Re: [PATCH 01/10] media: ioctl: Simulate v4l2_queryctrl with v4l2_query_ext_ctrl

[Ricardo Ribalda]

On Mon, 9 Dec 2024 at 21:02, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
>
> Hi Hans
>
> On Mon, 9 Dec 2024 at 20:34, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >
> > On 09/12/2024 20:25, Ricardo Ribalda wrote:
> > > v4l2_queryctrl is a subset of v4l2_query_ext_ctrl. If the driver does
> > > not implement v4l2_queryctrl we can implement it with
> > > v4l2_query_ext_ctrl.
> > >
> > > Suggested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-dev.c   |  3 ++-
> > >  drivers/media/v4l2-core/v4l2-ioctl.c | 22 +++++++++++++++++++++-
> > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > index 5bcaeeba4d09..252308a67fa8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -572,7 +572,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
> > >          and that can't be tested here. If the bit for these control ioctls
> > >          is set, then the ioctl is valid. But if it is 0, then it can still
> > >          be valid if the filehandle passed the control handler. */
> > > -     if (vdev->ctrl_handler || ops->vidioc_queryctrl)
> > > +     if (vdev->ctrl_handler || ops->vidioc_queryctrl ||
> > > +         ops->vidioc_query_ext_ctrl)
> > >               __set_bit(_IOC_NR(VIDIOC_QUERYCTRL), valid_ioctls);
> > >       if (vdev->ctrl_handler || ops->vidioc_query_ext_ctrl)
> > >               __set_bit(_IOC_NR(VIDIOC_QUERY_EXT_CTRL), valid_ioctls);
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index 0304daa8471d..a5562f2f1fc9 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -2284,9 +2284,11 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
> > >                               struct file *file, void *fh, void *arg)
> > >  {
> > >       struct video_device *vfd = video_devdata(file);
> > > +     struct v4l2_query_ext_ctrl qec;
> > >       struct v4l2_queryctrl *p = arg;
> > >       struct v4l2_fh *vfh =
> > >               test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> > > +     int ret;
> > >
> > >       if (vfh && vfh->ctrl_handler)
> > >               return v4l2_queryctrl(vfh->ctrl_handler, p);
> > > @@ -2294,7 +2296,25 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
> > >               return v4l2_queryctrl(vfd->ctrl_handler, p);
> > >       if (ops->vidioc_queryctrl)
> > >               return ops->vidioc_queryctrl(file, fh, p);
> > > -     return -ENOTTY;
> > > +     if (!ops->vidioc_query_ext_ctrl)
> > > +             return -ENOTTY;
> > > +
> > > +     /* Simulate query_ext_ctr using query_ctrl. */
> > > +     qec.id = p->id;
> > > +     ret = ops->vidioc_query_ext_ctrl(file, fh, &qec);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     p->id = qec.id;
> > > +     p->type = qec.type;
> > > +     strscpy(p->name, qec.name, sizeof(p->name));
> > > +     p->minimum = qec.minimum;
> > > +     p->maximum = qec.maximum;
> > > +     p->step = qec.step;
> > > +     p->default_value = qec.default_value;
> > > +     p->flags = qec.flags;
> >
> > That's not quite correct. See v4l2_queryctrl() in v4l2-ctrls-api.c
> > on how to do this: for types that VIDIOC_QUERYCTRL doesn't support,
> > some of these fields must be set to 0.
> >
> > In fact, once vidioc_queryctrl has been removed, then you can also
> > remove v4l2_queryctrl() and just rely on this code. Unless I missed
> > something.
>
> Thanks for the mega-fast review :)
>
> I do not think that we can easily remove v4l2_queryctrl(). It is still
> called by v4l2-subdev.c
>
> We could do something to remove the code duplication... but it will
> probably make the code more difficult to follow.
>
> I will send a new version with the fix that you proposed, as well as:
>
> -- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2290,10 +2290,6 @@ static int v4l_queryctrl(const struct
> v4l2_ioctl_ops *ops,
>                 test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>         int ret;
>
> -       if (vfh && vfh->ctrl_handler)
> -               return v4l2_queryctrl(vfh->ctrl_handler, p);
> -       if (vfd->ctrl_handler)
> -               return v4l2_queryctrl(vfd->ctrl_handler, p);
>         if (!ops->vidioc_query_ext_ctrl)
>                 return -ENOTTY;

Actually we cannot remove these four lines. I have a set ready with a helper....
https://gitlab.freedesktop.org/linux-media/users/ribalda/-/commits/queryctrl
Not sure if it is better with or without the helper.

Will send it tomorrow if I do not have more feedback.

Best rergards!


>
> >
> > Regards,
> >
> >         Hans
> >
> > > +
> > > +     return 0;
> > >  }
> > >
> > >  static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> > >
> >
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda