diff mbox

[v10,08/16] v4l: mark unordered formats

Message ID 20180521165946.11778-9-ezequiel@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia May 21, 2018, 4:59 p.m. UTC
From: Gustavo Padovan <gustavo.padovan@collabora.com>

Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
mark the appropriate formats.

v2: Set unordered flag before calling the driver callback.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 74 +++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 17 deletions(-)

Comments

Hans Verkuil May 22, 2018, 11:55 a.m. UTC | #1
On 21/05/18 18:59, Ezequiel Garcia wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
> mark the appropriate formats.
> 
> v2: Set unordered flag before calling the driver callback.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 74 +++++++++++++++++++++++++++---------
>  1 file changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index f48c505550e0..2135ac235a96 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1102,6 +1102,27 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> +static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
> +{
> +	switch (fmt->pixelformat) {
> +	case V4L2_PIX_FMT_MPEG:
> +	case V4L2_PIX_FMT_H264:
> +	case V4L2_PIX_FMT_H264_NO_SC:
> +	case V4L2_PIX_FMT_H264_MVC:
> +	case V4L2_PIX_FMT_H263:
> +	case V4L2_PIX_FMT_MPEG1:
> +	case V4L2_PIX_FMT_MPEG2:
> +	case V4L2_PIX_FMT_MPEG4:
> +	case V4L2_PIX_FMT_XVID:
> +	case V4L2_PIX_FMT_VC1_ANNEX_G:
> +	case V4L2_PIX_FMT_VC1_ANNEX_L:
> +	case V4L2_PIX_FMT_VP8:
> +	case V4L2_PIX_FMT_VP9:
> +	case V4L2_PIX_FMT_HEVC:
> +		fmt->flags |= V4L2_FMT_FLAG_UNORDERED;

Please add a break here. This prevents potential future errors if new cases
are added below this line.

> +	}
> +}
> +
>  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  {
>  	const unsigned sz = sizeof(fmt->description);
> @@ -1310,61 +1331,80 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  
>  	if (descr)
>  		WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
> -	fmt->flags = flags;
> +	fmt->flags |= flags;
>  }
>  
> -static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> -				struct file *file, void *fh, void *arg)
> -{
> -	struct v4l2_fmtdesc *p = arg;
> -	int ret = check_fmt(file, p->type);
>  
> -	if (ret)
> -		return ret;
> -	ret = -EINVAL;
> +static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
> +			     struct v4l2_fmtdesc *p,
> +			     struct file *file, void *fh)
> +{
> +	int ret = 0;
>  
>  	switch (p->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
>  			break;
> -		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
>  			break;
> -		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>  		if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
>  			break;
> -		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		if (unlikely(!ops->vidioc_enum_fmt_vid_out))
>  			break;
> -		ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>  		if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
>  			break;
> -		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
>  		if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
>  			break;
> -		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_SDR_OUTPUT:
>  		if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
>  			break;
> -		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
>  		break;
>  	case V4L2_BUF_TYPE_META_CAPTURE:
>  		if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
>  			break;
> -		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, arg);
> +		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, p);
>  		break;
>  	}
> +	return ret;
> +}
> +
> +static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_fmtdesc *p = arg;
> +	int ret = check_fmt(file, p->type);
> +
> +	if (ret)
> +		return ret;
> +	ret = -EINVAL;

Why set ret when it is overwritten below?

> +
> +	ret = __vidioc_enum_fmt(ops, p, file, fh);
> +	if (ret)
> +		return ret;

Huh? Why call the driver twice? As far as I can see you can just drop
these three lines above.

Regards,

	Hans

> +	/*
> +	 * Set the unordered flag and call the driver
> +	 * again so it has the chance to clear the flag.
> +	 */
> +	v4l_fill_unordered_fmtdesc(p);
> +	ret = __vidioc_enum_fmt(ops, p, file, fh);
>  	if (ret == 0)
>  		v4l_fill_fmtdesc(p);
>  	return ret;
>
Ezequiel Garcia May 23, 2018, 10:30 a.m. UTC | #2
On Tue, 2018-05-22 at 13:55 +0200, Hans Verkuil wrote:
> On 21/05/18 18:59, Ezequiel Garcia wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
> > mark the appropriate formats.
> > 
> > v2: Set unordered flag before calling the driver callback.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 74 +++++++++++++++++++++++++++---------
> >  1 file changed, 57 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index f48c505550e0..2135ac235a96 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1102,6 +1102,27 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
> >  	return ops->vidioc_enum_output(file, fh, p);
> >  }
> >  
> > +static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
> > +{
> > +	switch (fmt->pixelformat) {
> > +	case V4L2_PIX_FMT_MPEG:
> > +	case V4L2_PIX_FMT_H264:
> > +	case V4L2_PIX_FMT_H264_NO_SC:
> > +	case V4L2_PIX_FMT_H264_MVC:
> > +	case V4L2_PIX_FMT_H263:
> > +	case V4L2_PIX_FMT_MPEG1:
> > +	case V4L2_PIX_FMT_MPEG2:
> > +	case V4L2_PIX_FMT_MPEG4:
> > +	case V4L2_PIX_FMT_XVID:
> > +	case V4L2_PIX_FMT_VC1_ANNEX_G:
> > +	case V4L2_PIX_FMT_VC1_ANNEX_L:
> > +	case V4L2_PIX_FMT_VP8:
> > +	case V4L2_PIX_FMT_VP9:
> > +	case V4L2_PIX_FMT_HEVC:
> > +		fmt->flags |= V4L2_FMT_FLAG_UNORDERED;
> 
> Please add a break here. This prevents potential future errors if new cases
> are added below this line.
> 

Sure.

> > +	}
> > +}
> > +
> >  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >  {
> >  	const unsigned sz = sizeof(fmt->description);
> > @@ -1310,61 +1331,80 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >  
> >  	if (descr)
> >  		WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
> > -	fmt->flags = flags;
> > +	fmt->flags |= flags;
> >  }
> >  
> > -static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > -				struct file *file, void *fh, void *arg)
> > -{
> > -	struct v4l2_fmtdesc *p = arg;
> > -	int ret = check_fmt(file, p->type);
> >  
> > -	if (ret)
> > -		return ret;
> > -	ret = -EINVAL;
> > +static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > +			     struct v4l2_fmtdesc *p,
> > +			     struct file *file, void *fh)
> > +{
> > +	int ret = 0;
> >  
> >  	switch (p->type) {
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >  		if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >  		if (unlikely(!ops->vidioc_enum_fmt_vid_out))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >  		if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> >  		if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_SDR_OUTPUT:
> >  		if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
> >  		break;
> >  	case V4L2_BUF_TYPE_META_CAPTURE:
> >  		if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
> >  			break;
> > -		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, arg);
> > +		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, p);
> >  		break;
> >  	}
> > +	return ret;
> > +}
> > +
> > +static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > +				struct file *file, void *fh, void *arg)
> > +{
> > +	struct v4l2_fmtdesc *p = arg;
> > +	int ret = check_fmt(file, p->type);
> > +
> > +	if (ret)
> > +		return ret;
> > +	ret = -EINVAL;
> 
> Why set ret when it is overwritten below?
> 

Oops, seems to be some leftover code.

> > +
> > +	ret = __vidioc_enum_fmt(ops, p, file, fh);
> > +	if (ret)
> > +		return ret;
> 
> Huh? Why call the driver twice? As far as I can see you can just drop
> these three lines above.
> 
> 

Well, because I thought this was the outcome of v9 [1]. Let me quote you:

""
I realized that this is a problem since this function is called *after*
the driver. So the driver has no chance to clear this flag if it knows
that the queue is always ordered.
""

So, we first call the driver to get struct v4l2_fmtdesc pixelformat field
filled by the driver, then we call v4l_fill_unordered_fmtdesc()
to set FLAG_UNORDERED if needed, and finally we call the driver again
so it can clear the FLAG_UNORDERED.

Does that make sense?

[1] https://lkml.org/lkml/2018/5/7/349
Hans Verkuil May 23, 2018, 11:29 a.m. UTC | #3
On 23/05/18 12:30, Ezequiel Garcia wrote:
> On Tue, 2018-05-22 at 13:55 +0200, Hans Verkuil wrote:
>> On 21/05/18 18:59, Ezequiel Garcia wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
>>> mark the appropriate formats.
>>>
>>> v2: Set unordered flag before calling the driver callback.
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 74 +++++++++++++++++++++++++++---------
>>>  1 file changed, 57 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index f48c505550e0..2135ac235a96 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1102,6 +1102,27 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>>  	return ops->vidioc_enum_output(file, fh, p);
>>>  }
>>>  
>>> +static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
>>> +{
>>> +	switch (fmt->pixelformat) {
>>> +	case V4L2_PIX_FMT_MPEG:
>>> +	case V4L2_PIX_FMT_H264:
>>> +	case V4L2_PIX_FMT_H264_NO_SC:
>>> +	case V4L2_PIX_FMT_H264_MVC:
>>> +	case V4L2_PIX_FMT_H263:
>>> +	case V4L2_PIX_FMT_MPEG1:
>>> +	case V4L2_PIX_FMT_MPEG2:
>>> +	case V4L2_PIX_FMT_MPEG4:
>>> +	case V4L2_PIX_FMT_XVID:
>>> +	case V4L2_PIX_FMT_VC1_ANNEX_G:
>>> +	case V4L2_PIX_FMT_VC1_ANNEX_L:
>>> +	case V4L2_PIX_FMT_VP8:
>>> +	case V4L2_PIX_FMT_VP9:
>>> +	case V4L2_PIX_FMT_HEVC:
>>> +		fmt->flags |= V4L2_FMT_FLAG_UNORDERED;
>>
>> Please add a break here. This prevents potential future errors if new cases
>> are added below this line.
>>
> 
> Sure.
> 
>>> +	}
>>> +}
>>> +
>>>  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>>  {
>>>  	const unsigned sz = sizeof(fmt->description);
>>> @@ -1310,61 +1331,80 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>>  
>>>  	if (descr)
>>>  		WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
>>> -	fmt->flags = flags;
>>> +	fmt->flags |= flags;
>>>  }
>>>  
>>> -static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>> -				struct file *file, void *fh, void *arg)
>>> -{
>>> -	struct v4l2_fmtdesc *p = arg;
>>> -	int ret = check_fmt(file, p->type);
>>>  
>>> -	if (ret)
>>> -		return ret;
>>> -	ret = -EINVAL;
>>> +static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>> +			     struct v4l2_fmtdesc *p,
>>> +			     struct file *file, void *fh)
>>> +{
>>> +	int ret = 0;
>>>  
>>>  	switch (p->type) {
>>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_vid_out))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_SDR_OUTPUT:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
>>>  		break;
>>>  	case V4L2_BUF_TYPE_META_CAPTURE:
>>>  		if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
>>>  			break;
>>> -		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, arg);
>>> +		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, p);
>>>  		break;
>>>  	}
>>> +	return ret;
>>> +}
>>> +
>>> +static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>> +				struct file *file, void *fh, void *arg)
>>> +{
>>> +	struct v4l2_fmtdesc *p = arg;
>>> +	int ret = check_fmt(file, p->type);
>>> +
>>> +	if (ret)
>>> +		return ret;
>>> +	ret = -EINVAL;
>>
>> Why set ret when it is overwritten below?
>>
> 
> Oops, seems to be some leftover code.
> 
>>> +
>>> +	ret = __vidioc_enum_fmt(ops, p, file, fh);
>>> +	if (ret)
>>> +		return ret;
>>
>> Huh? Why call the driver twice? As far as I can see you can just drop
>> these three lines above.
>>
>>
> 
> Well, because I thought this was the outcome of v9 [1]. Let me quote you:
> 
> ""
> I realized that this is a problem since this function is called *after*
> the driver. So the driver has no chance to clear this flag if it knows
> that the queue is always ordered.
> ""
> 
> So, we first call the driver to get struct v4l2_fmtdesc pixelformat field
> filled by the driver, then we call v4l_fill_unordered_fmtdesc()
> to set FLAG_UNORDERED if needed, and finally we call the driver again
> so it can clear the FLAG_UNORDERED.
> 
> Does that make sense?

Not what I meant. v4l_fill_unordered_fmtdesc() should be called first, then
the driver (which can now fiddle with the flag if it wants to) and finally
v4l_fill_fmtdesc(p) is called.

Drivers that support these compressed formats need to be checked though:
they shouldn't overwrite flags unconditionally.

Regards,

	Hans

> 
> [1] https://lkml.org/lkml/2018/5/7/349
>
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index f48c505550e0..2135ac235a96 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1102,6 +1102,27 @@  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
 	return ops->vidioc_enum_output(file, fh, p);
 }
 
+static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
+{
+	switch (fmt->pixelformat) {
+	case V4L2_PIX_FMT_MPEG:
+	case V4L2_PIX_FMT_H264:
+	case V4L2_PIX_FMT_H264_NO_SC:
+	case V4L2_PIX_FMT_H264_MVC:
+	case V4L2_PIX_FMT_H263:
+	case V4L2_PIX_FMT_MPEG1:
+	case V4L2_PIX_FMT_MPEG2:
+	case V4L2_PIX_FMT_MPEG4:
+	case V4L2_PIX_FMT_XVID:
+	case V4L2_PIX_FMT_VC1_ANNEX_G:
+	case V4L2_PIX_FMT_VC1_ANNEX_L:
+	case V4L2_PIX_FMT_VP8:
+	case V4L2_PIX_FMT_VP9:
+	case V4L2_PIX_FMT_HEVC:
+		fmt->flags |= V4L2_FMT_FLAG_UNORDERED;
+	}
+}
+
 static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 {
 	const unsigned sz = sizeof(fmt->description);
@@ -1310,61 +1331,80 @@  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 
 	if (descr)
 		WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
-	fmt->flags = flags;
+	fmt->flags |= flags;
 }
 
-static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
-				struct file *file, void *fh, void *arg)
-{
-	struct v4l2_fmtdesc *p = arg;
-	int ret = check_fmt(file, p->type);
 
-	if (ret)
-		return ret;
-	ret = -EINVAL;
+static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
+			     struct v4l2_fmtdesc *p,
+			     struct file *file, void *fh)
+{
+	int ret = 0;
 
 	switch (p->type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 		if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
 			break;
-		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 		if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
 			break;
-		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
 		if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
 			break;
-		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		if (unlikely(!ops->vidioc_enum_fmt_vid_out))
 			break;
-		ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
 			break;
-		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
 		if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
 			break;
-		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_SDR_OUTPUT:
 		if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
 			break;
-		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 		if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
 			break;
-		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, arg);
+		ret = ops->vidioc_enum_fmt_meta_cap(file, fh, p);
 		break;
 	}
+	return ret;
+}
+
+static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
+				struct file *file, void *fh, void *arg)
+{
+	struct v4l2_fmtdesc *p = arg;
+	int ret = check_fmt(file, p->type);
+
+	if (ret)
+		return ret;
+	ret = -EINVAL;
+
+	ret = __vidioc_enum_fmt(ops, p, file, fh);
+	if (ret)
+		return ret;
+	/*
+	 * Set the unordered flag and call the driver
+	 * again so it has the chance to clear the flag.
+	 */
+	v4l_fill_unordered_fmtdesc(p);
+	ret = __vidioc_enum_fmt(ops, p, file, fh);
 	if (ret == 0)
 		v4l_fill_fmtdesc(p);
 	return ret;