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;
>
> Regards,
>
> Hans
>
> > +
> > + return 0;
> > }
> >
> > static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> >
>
--
Ricardo Ribalda
