| Message ID | 20241209-queryctrl-v1-1-deff7acfcdcb@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: Remove vidioc_g/s_ctrl and vidioc_queryctrl callbacks | expand |
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@redhat.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > 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. Regards, Hans > + > + return 0; > } > > static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops, >
Hi Hans On Mon, 9 Dec 2024 at 20:34, Hans Verkuil <hverkuil@xs4all.nl> 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@redhat.com> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > 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, > > >
On Mon, 9 Dec 2024 at 21:02, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Hans > > On Mon, 9 Dec 2024 at 20:34, Hans Verkuil <hverkuil@xs4all.nl> 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@redhat.com> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > 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
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; + + return 0; } static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
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@redhat.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-dev.c | 3 ++- drivers/media/v4l2-core/v4l2-ioctl.c | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-)