Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585669 > unrolled thread

[PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt Thibault Saunier <thibault.saunier@osg.samsung.com> - 2017-02-21 20:30 +0100
    Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set  'v4l2_pix_format:field' field in try_fmt Andrzej Hajda <a.hajda@samsung.com> - 2017-02-22 10:40 +0100
      Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set  'v4l2_pix_format:field' field in try_fmt Thibault Saunier <thibault.saunier@osg.samsung.com> - 2017-02-22 14:20 +0100
        Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set  'v4l2_pix_format:field' field in try_fmt Nicolas Dufresne <nicolas.dufresne@collabora.com> - 2017-02-22 15:50 +0100
        Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set  'v4l2_pix_format:field' field in try_fmt Andrzej Hajda <a.hajda@samsung.com> - 2017-02-23 12:20 +0100

#1585669 — [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

It is required by the standard that the field order is set by the
driver.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v5:
- Just adapt the field and never error out.

Changes in v4: None
Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 0976c3e0a5ce..44ed2afe0780 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct s5p_mfc_fmt *fmt;
 
+	if (f->fmt.pix.field == V4L2_FIELD_ANY)
+		f->fmt.pix.field = V4L2_FIELD_NONE;
+
 	mfc_debug(2, "Type is %d\n", f->type);
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		fmt = find_format(f, MFC_FMT_DEC);
-- 
2.11.1

[toc] | [next] | [standalone]

#1586021 — Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

On 21.02.2017 20:20, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver.
>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>
> ---
>
> Changes in v5:
> - Just adapt the field and never error out.
>
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
>
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 0976c3e0a5ce..44ed2afe0780 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +

In previous version the only supported field type was NONE, here you
support everything.
If the only supported format is none you should set 'field'
unconditionally to NONE, nothing more.

Regards
Andrzej

>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);

[toc] | [prev] | [next] | [standalone]

#1586138 — Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

Hello,

On 02/22/2017 06:29 AM, Andrzej Hajda wrote:
> On 21.02.2017 20:20, Thibault Saunier wrote:
>> It is required by the standard that the field order is set by the
>> driver.
>>
>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>
>> ---
>>
>> Changes in v5:
>> - Just adapt the field and never error out.
>>
>> Changes in v4: None
>> Changes in v3:
>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>
>> Changes in v2:
>> - Fix a silly build error that slipped in while rebasing the patches
>>
>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> index 0976c3e0a5ce..44ed2afe0780 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>>   	struct s5p_mfc_fmt *fmt;
>>   
>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>> +
> In previous version the only supported field type was NONE, here you
> support everything.
> If the only supported format is none you should set 'field'
> unconditionally to NONE, nothing more.
Afaict we  support 2 things:

   1. NONE
   2. INTERLACE

Until now we were not checking what was supported or not and basically 
ignoring that info, this patch
keeps the old behaviour making sure to be compliant.

I had a doubt and was pondering doing:

``` diff

+	if (f->fmt.pix.field != V4L2_FIELD_INTERLACED)
+		f->fmt.pix.field = V4L2_FIELD_NONE;
+

```

instead, it is probably more correct, do you think it is what should be 
done here?

Regards,

Thibault

>
> Regards
> Andrzej
>
>>   	mfc_debug(2, "Type is %d\n", f->type);
>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		fmt = find_format(f, MFC_FMT_DEC);
>

[toc] | [prev] | [next] | [standalone]

#1586221 — Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

[Multipart message — attachments visible in raw view] — view raw

Le mercredi 22 février 2017 à 10:10 -0300, Thibault Saunier a écrit :
> Hello,
> 
> On 02/22/2017 06:29 AM, Andrzej Hajda wrote:
> > On 21.02.2017 20:20, Thibault Saunier wrote:
> > > It is required by the standard that the field order is set by the
> > > driver.
> > > 
> > > Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com
> > > >
> > > 
> > > ---
> > > 
> > > Changes in v5:
> > > - Just adapt the field and never error out.
> > > 
> > > Changes in v4: None
> > > Changes in v3:
> > > - Do not check values in the g_fmt functions as Andrzej explained
> > > in previous review
> > > 
> > > Changes in v2:
> > > - Fix a silly build error that slipped in while rebasing the
> > > patches
> > > 
> > >   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > index 0976c3e0a5ce..44ed2afe0780 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file,
> > > void *priv, struct v4l2_format *f)
> > >   	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > >   	struct s5p_mfc_fmt *fmt;
> > >   
> > > +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> > > +		f->fmt.pix.field = V4L2_FIELD_NONE;
> > > +
> > 
> > In previous version the only supported field type was NONE, here
> > you
> > support everything.
> > If the only supported format is none you should set 'field'
> > unconditionally to NONE, nothing more.
> 
> Afaict we  support 2 things:
> 
>    1. NONE
>    2. INTERLACE
> 
> Until now we were not checking what was supported or not and
> basically 
> ignoring that info, this patch
> keeps the old behaviour making sure to be compliant.
> 
> I had a doubt and was pondering doing:
> 
> ``` diff
> 
> +	if (f->fmt.pix.field != V4L2_FIELD_INTERLACED)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
> 

This looks better to me.

> ```
> 
> instead, it is probably more correct, do you think it is what should
> be 
> done here?
> 
> Regards,
> 
> Thibault
> 
> > 
> > Regards
> > Andrzej
> > 
> > >   	mfc_debug(2, "Type is %d\n", f->type);
> > >   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > >   		fmt = find_format(f, MFC_FMT_DEC);
> 
> 

[toc] | [prev] | [next] | [standalone]

#1586825 — Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

On 22.02.2017 14:10, Thibault Saunier wrote:
> Hello,
>
> On 02/22/2017 06:29 AM, Andrzej Hajda wrote:
>> On 21.02.2017 20:20, Thibault Saunier wrote:
>>> It is required by the standard that the field order is set by the
>>> driver.
>>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>>
>>> ---
>>>
>>> Changes in v5:
>>> - Just adapt the field and never error out.
>>>
>>> Changes in v4: None
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>>
>>> Changes in v2:
>>> - Fix a silly build error that slipped in while rebasing the patches
>>>
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> index 0976c3e0a5ce..44ed2afe0780 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>>   	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>>>   	struct s5p_mfc_fmt *fmt;
>>>   
>>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>>> +
>> In previous version the only supported field type was NONE, here you
>> support everything.
>> If the only supported format is none you should set 'field'
>> unconditionally to NONE, nothing more.
> Afaict we  support 2 things:
>
>    1. NONE
>    2. INTERLACE
>
> Until now we were not checking what was supported or not and basically 
> ignoring that info, this patch
> keeps the old behaviour making sure to be compliant.
>
> I had a doubt and was pondering doing:
>
> ``` diff
>
> +	if (f->fmt.pix.field != V4L2_FIELD_INTERLACED)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>
> ```
>
> instead, it is probably more correct, do you think it is what should be 
> done here?

I have realized I have forgot that it is not simple mem2mem device, it
is decoder. It is feed with compressed data which is just byte stream,
the only valid field value on output side is V4L2_FIELD_NONE.
About possible interlacing we could only say in case of capture side.
And in this case there are multiple questions:
- how MFC decodes stream with interlaced content?
- is it possible to convince it to decode it as user asks?
- does it perform (de-)interlacing?
- what is implemented in the driver?

After answering above questions we will be able to say how fmt.pix.field
should be treated on capture side.
And similar question we can pose in case of encoder.

So simple patch and so many doubts.

Regards
Andrzej

>
> Regards,
>
> Thibault
>
>> Regards
>> Andrzej
>>
>>>   	mfc_debug(2, "Type is %d\n", f->type);
>>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		fmt = find_format(f, MFC_FMT_DEC);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web