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@xxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxx>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
>>> ---
>>> 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
>
