| Message ID | 20210317164511.39967-1-ribalda@chromium.org |
|---|---|
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
Hi Ricardo, On 17/03/2021 17:44, Ricardo Ribalda wrote: > Drivers that do not use the ctrl-framework use this function instead. > > - Do not check for multiple classes when getting the DEF_VAL. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Can you merge patches 1-4 into a single patch? It's really one big fix since this code was never updated when new 'which' values were added. So keeping it together is, for once, actually preferred. You can add my: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> after these 4 patches are merged. It looks much nicer now. Regards, Hans > > Cc: stable@vger.kernel.org > Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > 1 file changed, 27 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 31d1342e61e8..403f957a1012 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > pr_cont("driver-specific ioctl\n"); > } > > -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > { > __u32 i; > > @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > for (i = 0; i < c->count; i++) > c->controls[i].reserved2[0] = 0; > > - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > - when using extended controls. > - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > - is it allowed for backwards compatibility. > - */ > - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > - return 0; > - if (!c->which) > - return 1; > + switch (c->which) { > + case V4L2_CID_PRIVATE_BASE: > + /* > + * V4L2_CID_PRIVATE_BASE cannot be used as control class > + * when using extended controls. > + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > + * is it allowed for backwards compatibility. > + */ > + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > + return false; > + break; > + case V4L2_CTRL_WHICH_DEF_VAL: > + case V4L2_CTRL_WHICH_CUR_VAL: > + return true; > + } > + > /* Check that all controls are from the same control class. */ > for (i = 0; i < c->count; i++) { > if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > c->error_idx = i; > - return 0; > + return false; > } > } > - return 1; > + return true; > } > > static int check_fmt(struct file *file, enum v4l2_buf_type type) > @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > ctrls.controls = &ctrl; > ctrl.id = p->id; > ctrl.value = p->value; > - if (check_ext_ctrls(&ctrls, 1)) { > + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > > if (ret == 0) > @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > ctrls.controls = &ctrl; > ctrl.id = p->id; > ctrl.value = p->value; > - if (check_ext_ctrls(&ctrls, 1)) > + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > return -EINVAL; > } > @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > vfd, vfd->v4l2_dev->mdev, p); > if (ops->vidioc_g_ext_ctrls == NULL) > return -ENOTTY; > - return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > } > > static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > vfd, vfd->v4l2_dev->mdev, p); > if (ops->vidioc_s_ext_ctrls == NULL) > return -ENOTTY; > - return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > } > > static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > vfd, vfd->v4l2_dev->mdev, p); > if (ops->vidioc_try_ext_ctrls == NULL) > return -ENOTTY; > - return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > } > > /* >
Hi Hans Can I merge 1-3, but leave 4 as a separate one? It helps to tell a story for 5 and 6. Thanks On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Ricardo, > > On 17/03/2021 17:44, Ricardo Ribalda wrote: > > Drivers that do not use the ctrl-framework use this function instead. > > > > - Do not check for multiple classes when getting the DEF_VAL. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Can you merge patches 1-4 into a single patch? It's really one big fix since > this code was never updated when new 'which' values were added. So keeping it > together is, for once, actually preferred. > > You can add my: > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > after these 4 patches are merged. It looks much nicer now. > > Regards, > > Hans > > > > > Cc: stable@vger.kernel.org > > Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > > 1 file changed, 27 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 31d1342e61e8..403f957a1012 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > > pr_cont("driver-specific ioctl\n"); > > } > > > > -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > > +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > > { > > __u32 i; > > > > @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > > for (i = 0; i < c->count; i++) > > c->controls[i].reserved2[0] = 0; > > > > - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > > - when using extended controls. > > - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > > - is it allowed for backwards compatibility. > > - */ > > - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > > - return 0; > > - if (!c->which) > > - return 1; > > + switch (c->which) { > > + case V4L2_CID_PRIVATE_BASE: > > + /* > > + * V4L2_CID_PRIVATE_BASE cannot be used as control class > > + * when using extended controls. > > + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > > + * is it allowed for backwards compatibility. > > + */ > > + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > > + return false; > > + break; > > + case V4L2_CTRL_WHICH_DEF_VAL: > > + case V4L2_CTRL_WHICH_CUR_VAL: > > + return true; > > + } > > + > > /* Check that all controls are from the same control class. */ > > for (i = 0; i < c->count; i++) { > > if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > > c->error_idx = i; > > - return 0; > > + return false; > > } > > } > > - return 1; > > + return true; > > } > > > > static int check_fmt(struct file *file, enum v4l2_buf_type type) > > @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > > ctrls.controls = &ctrl; > > ctrl.id = p->id; > > ctrl.value = p->value; > > - if (check_ext_ctrls(&ctrls, 1)) { > > + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > > int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > > > > if (ret == 0) > > @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > > ctrls.controls = &ctrl; > > ctrl.id = p->id; > > ctrl.value = p->value; > > - if (check_ext_ctrls(&ctrls, 1)) > > + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > > return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > > return -EINVAL; > > } > > @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > vfd, vfd->v4l2_dev->mdev, p); > > if (ops->vidioc_g_ext_ctrls == NULL) > > return -ENOTTY; > > - return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > > + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > vfd, vfd->v4l2_dev->mdev, p); > > if (ops->vidioc_s_ext_ctrls == NULL) > > return -ENOTTY; > > - return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > > + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > vfd, vfd->v4l2_dev->mdev, p); > > if (ops->vidioc_try_ext_ctrls == NULL) > > return -ENOTTY; > > - return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > > + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > /* > > >
On 17/03/2021 17:45, Ricardo Ribalda wrote: > Pass the chain instead of the device. We want to keed the reference to keed -> keep With that typo fixed you can add my: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > the chain that controls belong to. > > We need to delay the initialization of the controls after the chains > have been initialized. > > This is a cleanup needed for the next patches. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++---------- > drivers/media/usb/uvc/uvc_driver.c | 8 +++--- > 2 files changed, 32 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index b3dde98499f4..b75da65115ef 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -2057,7 +2057,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > /* > * Add a control mapping to a given control. > */ > -static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) > { > struct uvc_control_mapping *map; > @@ -2086,7 +2086,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > map->set = uvc_set_le_value; > > list_add_tail(&map->list, &ctrl->info.mappings); > - uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > + uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > map->name, ctrl->info.entity, ctrl->info.selector); > > return 0; > @@ -2168,7 +2168,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > goto done; > } > > - ret = __uvc_ctrl_add_mapping(dev, ctrl, mapping); > + ret = __uvc_ctrl_add_mapping(chain, ctrl, mapping); > if (ret < 0) > atomic_dec(&dev->nmappings); > > @@ -2244,7 +2244,8 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev, > * Add control information and hardcoded stock control mappings to the given > * device. > */ > -static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) > +static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, > + struct uvc_control *ctrl) > { > const struct uvc_control_info *info = uvc_ctrls; > const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls); > @@ -2263,14 +2264,14 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) > for (; info < iend; ++info) { > if (uvc_entity_match_guid(ctrl->entity, info->entity) && > ctrl->index == info->index) { > - uvc_ctrl_add_info(dev, ctrl, info); > + uvc_ctrl_add_info(chain->dev, ctrl, info); > /* > * Retrieve control flags from the device. Ignore errors > * and work with default flag values from the uvc_ctrl > * array when the device doesn't properly implement > * GET_INFO on standard controls. > */ > - uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); > + uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info); > break; > } > } > @@ -2281,22 +2282,20 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) > for (; mapping < mend; ++mapping) { > if (uvc_entity_match_guid(ctrl->entity, mapping->entity) && > ctrl->info.selector == mapping->selector) > - __uvc_ctrl_add_mapping(dev, ctrl, mapping); > + __uvc_ctrl_add_mapping(chain, ctrl, mapping); > } > } > > /* > * Initialize device controls. > */ > -int uvc_ctrl_init_device(struct uvc_device *dev) > +static int uvc_ctrl_init_chain(struct uvc_video_chain *chain) > { > struct uvc_entity *entity; > unsigned int i; > > - INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work); > - > /* Walk the entities list and instantiate controls */ > - list_for_each_entry(entity, &dev->entities, list) { > + list_for_each_entry(entity, &chain->entities, chain) { > struct uvc_control *ctrl; > unsigned int bControlSize = 0, ncontrols; > u8 *bmControls = NULL; > @@ -2316,7 +2315,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > } > > /* Remove bogus/blacklisted controls */ > - uvc_ctrl_prune_entity(dev, entity); > + uvc_ctrl_prune_entity(chain->dev, entity); > > /* Count supported controls and allocate the controls array */ > ncontrols = memweight(bmControls, bControlSize); > @@ -2338,7 +2337,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > ctrl->entity = entity; > ctrl->index = i; > > - uvc_ctrl_init_ctrl(dev, ctrl); > + uvc_ctrl_init_ctrl(chain, ctrl); > ctrl++; > } > } > @@ -2346,6 +2345,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > return 0; > } > > +int uvc_ctrl_init_device(struct uvc_device *dev) > +{ > + struct uvc_video_chain *chain; > + int ret; > + > + INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work); > + > + list_for_each_entry(chain, &dev->chains, list) { > + ret = uvc_ctrl_init_chain(chain); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > /* > * Cleanup device controls. > */ > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 30ef2a3110f7..35873cf2773d 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2423,14 +2423,14 @@ static int uvc_probe(struct usb_interface *intf, > if (v4l2_device_register(&intf->dev, &dev->vdev) < 0) > goto error; > > - /* Initialize controls. */ > - if (uvc_ctrl_init_device(dev) < 0) > - goto error; > - > /* Scan the device for video chains. */ > if (uvc_scan_device(dev) < 0) > goto error; > > + /* Initialize controls. */ > + if (uvc_ctrl_init_device(dev) < 0) > + goto error; > + > /* Register video device nodes. */ > if (uvc_register_chains(dev) < 0) > goto error; >
On 18/03/2021 08:17, Ricardo Ribalda wrote: > Hi Hans > > Can I merge 1-3, but leave 4 as a separate one? It helps to tell a > story for 5 and 6. I really prefer it as a single patch. All four patches are basically a single big fix for v4l2-ioctl.c where the code for drivers that do not use the control framework had become very outdated. Fixing it in a single patch helps backporting to stable, and it is easier to review and see everything that had to be done to fix this. In this case I wondered when I was reviewing patch 1 why V4L2_CTRL_WHICH_DEF_VAL was just accepted without checking for S/TRY_EXT_CTRLS. Basically patch 1 is a broken fix w.r.t. DEF_VAL until patch 4, which really fixes it. Just do it all in a single patch, splitting it up doesn't work in this particular case. Regards, Hans > > Thanks > > On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Hi Ricardo, >> >> On 17/03/2021 17:44, Ricardo Ribalda wrote: >>> Drivers that do not use the ctrl-framework use this function instead. >>> >>> - Do not check for multiple classes when getting the DEF_VAL. >>> >>> Fixes v4l2-compliance: >>> Control ioctls (Input 0): >>> fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) >>> test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL >> >> Can you merge patches 1-4 into a single patch? It's really one big fix since >> this code was never updated when new 'which' values were added. So keeping it >> together is, for once, actually preferred. >> >> You can add my: >> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> after these 4 patches are merged. It looks much nicer now. >> >> Regards, >> >> Hans >> >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ >>> 1 file changed, 27 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>> index 31d1342e61e8..403f957a1012 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) >>> pr_cont("driver-specific ioctl\n"); >>> } >>> >>> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) >>> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) >>> { >>> __u32 i; >>> >>> @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) >>> for (i = 0; i < c->count; i++) >>> c->controls[i].reserved2[0] = 0; >>> >>> - /* V4L2_CID_PRIVATE_BASE cannot be used as control class >>> - when using extended controls. >>> - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL >>> - is it allowed for backwards compatibility. >>> - */ >>> - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) >>> - return 0; >>> - if (!c->which) >>> - return 1; >>> + switch (c->which) { >>> + case V4L2_CID_PRIVATE_BASE: >>> + /* >>> + * V4L2_CID_PRIVATE_BASE cannot be used as control class >>> + * when using extended controls. >>> + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL >>> + * is it allowed for backwards compatibility. >>> + */ >>> + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) >>> + return false; >>> + break; >>> + case V4L2_CTRL_WHICH_DEF_VAL: >>> + case V4L2_CTRL_WHICH_CUR_VAL: >>> + return true; >>> + } >>> + >>> /* Check that all controls are from the same control class. */ >>> for (i = 0; i < c->count; i++) { >>> if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { >>> c->error_idx = i; >>> - return 0; >>> + return false; >>> } >>> } >>> - return 1; >>> + return true; >>> } >>> >>> static int check_fmt(struct file *file, enum v4l2_buf_type type) >>> @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, >>> ctrls.controls = &ctrl; >>> ctrl.id = p->id; >>> ctrl.value = p->value; >>> - if (check_ext_ctrls(&ctrls, 1)) { >>> + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { >>> int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); >>> >>> if (ret == 0) >>> @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, >>> ctrls.controls = &ctrl; >>> ctrl.id = p->id; >>> ctrl.value = p->value; >>> - if (check_ext_ctrls(&ctrls, 1)) >>> + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) >>> return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); >>> return -EINVAL; >>> } >>> @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> vfd, vfd->v4l2_dev->mdev, p); >>> if (ops->vidioc_g_ext_ctrls == NULL) >>> return -ENOTTY; >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? >>> + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> vfd, vfd->v4l2_dev->mdev, p); >>> if (ops->vidioc_s_ext_ctrls == NULL) >>> return -ENOTTY; >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? >>> + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> vfd, vfd->v4l2_dev->mdev, p); >>> if (ops->vidioc_try_ext_ctrls == NULL) >>> return -ENOTTY; >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? >>> + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> /* >>> >> > >
Hi Hans On Thu, Mar 18, 2021 at 8:48 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 18/03/2021 08:17, Ricardo Ribalda wrote: > > Hi Hans > > > > Can I merge 1-3, but leave 4 as a separate one? It helps to tell a > > story for 5 and 6. > > I really prefer it as a single patch. All four patches are basically a single big fix > for v4l2-ioctl.c where the code for drivers that do not use the control framework had > become very outdated. Fixing it in a single patch helps backporting to stable, and > it is easier to review and see everything that had to be done to fix this. > > In this case I wondered when I was reviewing patch 1 why V4L2_CTRL_WHICH_DEF_VAL was > just accepted without checking for S/TRY_EXT_CTRLS. Basically patch 1 is a broken fix > w.r.t. DEF_VAL until patch 4, which really fixes it. > > Just do it all in a single patch, splitting it up doesn't work in this particular case. Ok, thanks for the clarification :) > > Regards, > > Hans > > > > > Thanks > > > > On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> Hi Ricardo, > >> > >> On 17/03/2021 17:44, Ricardo Ribalda wrote: > >>> Drivers that do not use the ctrl-framework use this function instead. > >>> > >>> - Do not check for multiple classes when getting the DEF_VAL. > >>> > >>> Fixes v4l2-compliance: > >>> Control ioctls (Input 0): > >>> fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > >>> test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > >> > >> Can you merge patches 1-4 into a single patch? It's really one big fix since > >> this code was never updated when new 'which' values were added. So keeping it > >> together is, for once, actually preferred. > >> > >> You can add my: > >> > >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >> after these 4 patches are merged. It looks much nicer now. > >> > >> Regards, > >> > >> Hans > >> > >>> > >>> Cc: stable@vger.kernel.org > >>> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > >>> 1 file changed, 27 insertions(+), 20 deletions(-) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> index 31d1342e61e8..403f957a1012 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > >>> pr_cont("driver-specific ioctl\n"); > >>> } > >>> > >>> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > >>> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > >>> { > >>> __u32 i; > >>> > >>> @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > >>> for (i = 0; i < c->count; i++) > >>> c->controls[i].reserved2[0] = 0; > >>> > >>> - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > >>> - when using extended controls. > >>> - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > >>> - is it allowed for backwards compatibility. > >>> - */ > >>> - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > >>> - return 0; > >>> - if (!c->which) > >>> - return 1; > >>> + switch (c->which) { > >>> + case V4L2_CID_PRIVATE_BASE: > >>> + /* > >>> + * V4L2_CID_PRIVATE_BASE cannot be used as control class > >>> + * when using extended controls. > >>> + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > >>> + * is it allowed for backwards compatibility. > >>> + */ > >>> + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > >>> + return false; > >>> + break; > >>> + case V4L2_CTRL_WHICH_DEF_VAL: > >>> + case V4L2_CTRL_WHICH_CUR_VAL: > >>> + return true; > >>> + } > >>> + > >>> /* Check that all controls are from the same control class. */ > >>> for (i = 0; i < c->count; i++) { > >>> if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > >>> c->error_idx = i; > >>> - return 0; > >>> + return false; > >>> } > >>> } > >>> - return 1; > >>> + return true; > >>> } > >>> > >>> static int check_fmt(struct file *file, enum v4l2_buf_type type) > >>> @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > >>> ctrls.controls = &ctrl; > >>> ctrl.id = p->id; > >>> ctrl.value = p->value; > >>> - if (check_ext_ctrls(&ctrls, 1)) { > >>> + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > >>> int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > >>> > >>> if (ret == 0) > >>> @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > >>> ctrls.controls = &ctrl; > >>> ctrl.id = p->id; > >>> ctrl.value = p->value; > >>> - if (check_ext_ctrls(&ctrls, 1)) > >>> + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > >>> return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > >>> return -EINVAL; > >>> } > >>> @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> vfd, vfd->v4l2_dev->mdev, p); > >>> if (ops->vidioc_g_ext_ctrls == NULL) > >>> return -ENOTTY; > >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > >>> + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> vfd, vfd->v4l2_dev->mdev, p); > >>> if (ops->vidioc_s_ext_ctrls == NULL) > >>> return -ENOTTY; > >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > >>> + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> vfd, vfd->v4l2_dev->mdev, p); > >>> if (ops->vidioc_try_ext_ctrls == NULL) > >>> return -ENOTTY; > >>> - return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > >>> + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> /* > >>> > >> > > > > > -- Ricardo Ribalda
On 17/03/2021 17:44, Ricardo Ribalda wrote: > v4l2-compliance -m /dev/media0 -a -f > Total for uvcvideo device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0 > Total for uvcvideo device /dev/video0: 54, Succeeded: 50, Failed: 4, Warnings: 2 > Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 > Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 102, > Failed: 6, Warnings: 2 > > After fixing all of them we go down to: > > Total for uvcvideo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 > Total for uvcvideo device /dev/video0: 54, Succeeded: 54, Failed: 0, Warnings: 0 > Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 > Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 108, > Failed: 0, Warnings: 0 > > YES, NO MORE WARNINGS :) > > Note that we depend on: > https://patchwork.linuxtv.org/project/linux-media/patch/20210315172531.101694-1-ribalda@chromium.org/ > > With Hans patch we can also pass v4l2-compliance -s. There is one remaining issue: if uvc_ioctl_s_try_ext_ctrls() calls uvc_ctrl_commit and the 'commit' fails for a certain control, then you want error_idx to be the index of the failing control. This is obviously something that v4l2-compliance cannot test since it would rely on a buggy uvc HW implementation. But I can test it by dropping patch 16/17: that should force the commit to fail and then I can verify error_idx. Regards, Hans > > Changelog from v5 (Thanks to Hans) > - Move more checks to framework > - Rewrite the framework check_ext_ctrls > - Rewrite ctrl_is_inactive > - Add function ctrl_is_accessible > - Use ioctl name instead of boolean values > > Hans Verkuil (1): > uvc: use vb2 ioctl and fop helpers > > Ricardo Ribalda (16): > media: v4l2-ioctl: check_ext_ctrls: Fix multiclass > V4L2_CTRL_WHICH_DEF_VAL > media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on > V4L2_CTRL_WHICH_REQUEST_VAL > media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx > media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL > media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL > media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL > media: uvcvideo: Set capability in s_param > media: uvcvideo: Return -EIO for control errors > media: uvcvideo: refactor __uvc_ctrl_add_mapping > media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS > media: uvcvideo: Use dev->name for querycap() > media: uvcvideo: Set unique vdev name based in type > media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE > media: uvcvideo: Use control names from framework > media: uvcvideo: Check controls flags before accessing them > media: uvcvideo: Return -EACCES to inactive controls > > drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 - > drivers/media/usb/uvc/uvc_ctrl.c | 284 ++++++++++++++---- > drivers/media/usb/uvc/uvc_driver.c | 22 +- > drivers/media/usb/uvc/uvc_metadata.c | 8 +- > drivers/media/usb/uvc/uvc_queue.c | 143 --------- > drivers/media/usb/uvc/uvc_v4l2.c | 352 +++++------------------ > drivers/media/usb/uvc/uvc_video.c | 13 +- > drivers/media/usb/uvc/uvcvideo.h | 43 +-- > drivers/media/v4l2-core/v4l2-ioctl.c | 59 ++-- > 9 files changed, 366 insertions(+), 562 deletions(-) >
v4l2-compliance -m /dev/media0 -a -f Total for uvcvideo device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0 Total for uvcvideo device /dev/video0: 54, Succeeded: 50, Failed: 4, Warnings: 2 Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 102, Failed: 6, Warnings: 2 After fixing all of them we go down to: Total for uvcvideo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 Total for uvcvideo device /dev/video0: 54, Succeeded: 54, Failed: 0, Warnings: 0 Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 108, Failed: 0, Warnings: 0 YES, NO MORE WARNINGS :) Note that we depend on: https://patchwork.linuxtv.org/project/linux-media/patch/20210315172531.101694-1-ribalda@chromium.org/ With Hans patch we can also pass v4l2-compliance -s. Changelog from v5 (Thanks to Hans) - Move more checks to framework - Rewrite the framework check_ext_ctrls - Rewrite ctrl_is_inactive - Add function ctrl_is_accessible - Use ioctl name instead of boolean values Hans Verkuil (1): uvc: use vb2 ioctl and fop helpers Ricardo Ribalda (16): media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on V4L2_CTRL_WHICH_REQUEST_VAL media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL media: uvcvideo: Set capability in s_param media: uvcvideo: Return -EIO for control errors media: uvcvideo: refactor __uvc_ctrl_add_mapping media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS media: uvcvideo: Use dev->name for querycap() media: uvcvideo: Set unique vdev name based in type media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE media: uvcvideo: Use control names from framework media: uvcvideo: Check controls flags before accessing them media: uvcvideo: Return -EACCES to inactive controls drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 - drivers/media/usb/uvc/uvc_ctrl.c | 284 ++++++++++++++---- drivers/media/usb/uvc/uvc_driver.c | 22 +- drivers/media/usb/uvc/uvc_metadata.c | 8 +- drivers/media/usb/uvc/uvc_queue.c | 143 --------- drivers/media/usb/uvc/uvc_v4l2.c | 352 +++++------------------ drivers/media/usb/uvc/uvc_video.c | 13 +- drivers/media/usb/uvc/uvcvideo.h | 43 +-- drivers/media/v4l2-core/v4l2-ioctl.c | 59 ++-- 9 files changed, 366 insertions(+), 562 deletions(-)