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
