| Message ID | 20220920-resend-v4l2-compliance-v2-0-b0ceb15353ac@chromium.org |
|---|---|
| Headers | show |
| Series | Follow-up patches for uvc v4l2-compliance | expand |
Hi Ricardo, Thank you for the patch. On Fri, Dec 02, 2022 at 06:21:40PM +0100, Ricardo Ribalda wrote: > Replace the count with a mask field that lets us choose not only the max > value, but also the minimum value and what values are valid in between. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 30 ++++++++++++++++++++---------- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > drivers/media/usb/uvc/uvcvideo.h | 2 +- > 4 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 526572044e82..df273b829961 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -6,6 +6,7 @@ > * Laurent Pinchart (laurent.pinchart@ideasonboard.com) > */ > > +#include <linux/bitops.h> > #include <linux/kernel.h> > #include <linux/list.h> > #include <linux/module.h> > @@ -524,7 +525,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = { > .v4l2_type = V4L2_CTRL_TYPE_MENU, > .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > .menu_info = exposure_auto_controls, > - .menu_count = ARRAY_SIZE(exposure_auto_controls), > + .menu_mask = BIT_MASK(ARRAY_SIZE(exposure_auto_controls)), > .slave_ids = { V4L2_CID_EXPOSURE_ABSOLUTE, }, > }, > { > @@ -730,7 +731,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc11[] = { > .v4l2_type = V4L2_CTRL_TYPE_MENU, > .data_type = UVC_CTRL_DATA_TYPE_ENUM, > .menu_info = power_line_frequency_controls, > - .menu_count = ARRAY_SIZE(power_line_frequency_controls) - 1, > + .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls) - 1), > }, > }; > > @@ -744,7 +745,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc15[] = { > .v4l2_type = V4L2_CTRL_TYPE_MENU, > .data_type = UVC_CTRL_DATA_TYPE_ENUM, > .menu_info = power_line_frequency_controls, > - .menu_count = ARRAY_SIZE(power_line_frequency_controls), > + .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls)), > }, > }; > > @@ -974,7 +975,9 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > const struct uvc_menu_info *menu = mapping->menu_info; > unsigned int i; > > - for (i = 0; i < mapping->menu_count; ++i, ++menu) { > + for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) { > + if (!test_bit(i, &mapping->menu_mask)) > + continue; > if (menu->value == value) { > value = i; > break; > @@ -1212,12 +1215,14 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > switch (mapping->v4l2_type) { > case V4L2_CTRL_TYPE_MENU: > - v4l2_ctrl->minimum = 0; > - v4l2_ctrl->maximum = mapping->menu_count - 1; > + v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1; > + v4l2_ctrl->maximum = fls(mapping->menu_mask) - 1; > v4l2_ctrl->step = 1; > > menu = mapping->menu_info; > - for (i = 0; i < mapping->menu_count; ++i, ++menu) { > + for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) { > + if (!test_bit(i, &mapping->menu_mask)) > + continue; > if (menu->value == v4l2_ctrl->default_value) { > v4l2_ctrl->default_value = i; > break; > @@ -1338,7 +1343,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain, > goto done; > } > > - if (query_menu->index >= mapping->menu_count) { > + if (!test_bit(query_menu->index, &mapping->menu_mask)) { > ret = -EINVAL; > goto done; > } > @@ -1853,8 +1858,13 @@ int uvc_ctrl_set(struct uvc_fh *handle, > break; > > case V4L2_CTRL_TYPE_MENU: > - if (xctrl->value < 0 || xctrl->value >= mapping->menu_count) > + if (xctrl->value < (ffs(mapping->menu_mask) - 1) || > + xctrl->value > (fls(mapping->menu_mask) - 1)) > return -ERANGE; > + > + if (!test_bit(xctrl->value, &mapping->menu_mask)) > + return -EINVAL; > + > value = mapping->menu_info[xctrl->value].value; > > /* > @@ -2301,7 +2311,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > INIT_LIST_HEAD(&map->ev_subs); > > - size = sizeof(*mapping->menu_info) * mapping->menu_count; > + size = sizeof(*mapping->menu_info) * fls(mapping->menu_mask); > map->menu_info = kmemdup(mapping->menu_info, size, GFP_KERNEL); > if (map->menu_info == NULL) { > kfree(map->name); > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 9c05776f11d1..abdb9ca7eed6 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2675,7 +2675,7 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = { > .v4l2_type = V4L2_CTRL_TYPE_MENU, > .data_type = UVC_CTRL_DATA_TYPE_ENUM, > .menu_info = power_line_frequency_controls_limited, > - .menu_count = ARRAY_SIZE(power_line_frequency_controls_limited), > + .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls_limited)), Let's include linux/bits.h. Same in the next file. I'll fix this when applying the patch if there's no other need to submit a new version of the series. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > }; > > static const struct uvc_device_info uvc_ctrl_power_line_limited = { > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index d95168cdc2d1..e6792fd46bf5 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -80,7 +80,7 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain, > goto free_map; > } > > - map->menu_count = xmap->menu_count; > + map->menu_mask = BIT_MASK(xmap->menu_count); > break; > > default: > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 644d5fcf2eef..7e2339fc256e 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -255,7 +255,7 @@ struct uvc_control_mapping { > u32 data_type; > > const struct uvc_menu_info *menu_info; > - u32 menu_count; > + unsigned long menu_mask; > > u32 master_id; > s32 master_manual; >
Hi Ricardo, Thank you for the patch. On Fri, Dec 02, 2022 at 06:21:37PM +0100, Ricardo Ribalda wrote: > For error 2 (Wrong state) return -EACCES instead of -EILSEQ. > EACCES is a much more appropriate error code. EILSEQ will return > "Invalid or incomplete multibyte or wide character." in strerror(), > which is a *very* confusing message. Unless there's an objection, I'd like to use the following text to replace the commit message to provide more information: Error 2 is defined by UVC as Wrong State: The device is in a state that disallows the specific request. The device will remain in this state until a specific action from the host or the user is completed. This is documented as happening happen when attempting to set the value of a manual control when the device is in auto mode. While V4L2 allows this, the closest error code defined by VIDIOC_S_CTRL is indeed EACCES: EACCES Attempt to set a read-only control or to get a write-only control. Or if there is an attempt to set an inactive control and the driver is not capable of caching the new value until the control is active again. Replace EILSEQ with EACCESS. > Suggested-by: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 2cf7f692c0bb..497073a50194 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -108,7 +108,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > case 1: /* Not ready */ > return -EBUSY; > case 2: /* Wrong state */ > - return -EILSEQ; > + return -EACCES; > case 3: /* Power */ > return -EREMOTE; > case 4: /* Out of range */ >
Hi Ricardo, On Fri, Dec 02, 2022 at 06:21:34PM +0100, Ricardo Ribalda wrote: > This patchset contains the fixes for the comments on "v10 of Fix > v4l2-compliance errors series". In particular to the patches > > -uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE > -uvcvideo: improve error handling in uvc_query_ctrl() > > And the patch: > -uvcvideo: Fix handling on Bitmask controls Patches 1/7, 3/7, 4/7 and 6/7 are fine (possibly with changes mentioned in my review comments that I can handle when applying). I can apply them to my tree already (with a minor conflict resolution between 2/7 and 3/7), but it may be easier for you to send a v3 of the whole series. Please let me know what you'd prefer. > To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > To: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: linux-media@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > Changes in v2: > - Include "Get menu names from framework series" > https://lore.kernel.org/r/20220920-standard-menues-v2-0-a35af3243c2f@chromium.org > - Link to v1: https://lore.kernel.org/r/20220920-resend-v4l2-compliance-v1-0-81364c15229b@chromium.org > > --- > Hans Verkuil (2): > media: uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE > media: uvcvideo: improve error logging in uvc_query_ctrl() > > Ricardo Ribalda (5): > media: uvcvideo: Return -EACCES for Wrong state error > media: uvcvideo: Do not return positive errors in uvc_query_ctrl() > media: uvcvideo: Fix handling on Bitmask controls > media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU > media: uvcvideo: Use standard names for menus > > drivers/media/usb/uvc/uvc_ctrl.c | 230 ++++++++++++++++++++++++++++--------- > drivers/media/usb/uvc/uvc_driver.c | 9 +- > drivers/media/usb/uvc/uvc_v4l2.c | 85 ++++++++++---- > drivers/media/usb/uvc/uvc_video.c | 15 +-- > drivers/media/usb/uvc/uvcvideo.h | 8 +- > include/uapi/linux/uvcvideo.h | 3 +- > 6 files changed, 258 insertions(+), 92 deletions(-) > --- > base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6 > change-id: 20220920-resend-v4l2-compliance-4fdbe4fbd7b5
This patchset contains the fixes for the comments on "v10 of Fix v4l2-compliance errors series". In particular to the patches -uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE -uvcvideo: improve error handling in uvc_query_ctrl() And the patch: -uvcvideo: Fix handling on Bitmask controls To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: linux-media@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Hans Verkuil <hans.verkuil@cisco.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v2: - Include "Get menu names from framework series" https://lore.kernel.org/r/20220920-standard-menues-v2-0-a35af3243c2f@chromium.org - Link to v1: https://lore.kernel.org/r/20220920-resend-v4l2-compliance-v1-0-81364c15229b@chromium.org --- Hans Verkuil (2): media: uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE media: uvcvideo: improve error logging in uvc_query_ctrl() Ricardo Ribalda (5): media: uvcvideo: Return -EACCES for Wrong state error media: uvcvideo: Do not return positive errors in uvc_query_ctrl() media: uvcvideo: Fix handling on Bitmask controls media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU media: uvcvideo: Use standard names for menus drivers/media/usb/uvc/uvc_ctrl.c | 230 ++++++++++++++++++++++++++++--------- drivers/media/usb/uvc/uvc_driver.c | 9 +- drivers/media/usb/uvc/uvc_v4l2.c | 85 ++++++++++---- drivers/media/usb/uvc/uvc_video.c | 15 +-- drivers/media/usb/uvc/uvcvideo.h | 8 +- include/uapi/linux/uvcvideo.h | 3 +- 6 files changed, 258 insertions(+), 92 deletions(-) --- base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6 change-id: 20220920-resend-v4l2-compliance-4fdbe4fbd7b5 Best regards,