Hi Ricardo, Thank you for the patch. On Tue, Jan 03, 2023 at 03:36:23PM +0100, Ricardo Ribalda wrote: > Minimum and step values for V4L2_CTRL_TYPE_BITMASK controls should be 0. > There is no need to query the camera firmware about this and maybe get > invalid results. > > Also value should be masked to the max value advertised by the > hardware. > > Finally, handle UVC 1.5 mask controls that use MAX instead of RES to > describe the valid bits. > > Fixes v4l2-compliane: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(97): minimum must be 0 for a bitmask control > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 6165d6b8e855..7622c5b54b35 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1161,6 +1161,25 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) > return "Unknown Control"; > } > > +static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping) > +{ > + /* > + * Some controls, like CT_AE_MODE_CONTROL, use GET_RES to represent > + * the number of bits supported. Those controls do not list GET_MAX > + * as supported. > + */ > + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) > + return mapping->get(mapping, UVC_GET_RES, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); > + > + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) > + return mapping->get(mapping, UVC_GET_MAX, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); > + > + return ~0; > +} > + > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control *ctrl, > struct uvc_control_mapping *mapping, > @@ -1235,6 +1254,12 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > v4l2_ctrl->step = 0; > return 0; > > + case V4L2_CTRL_TYPE_BITMASK: > + v4l2_ctrl->minimum = 0; > + v4l2_ctrl->maximum = uvc_get_ctrl_bitmap(ctrl, mapping); > + v4l2_ctrl->step = 0; > + return 0; > + > default: > break; > } > @@ -1336,19 +1361,14 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain, > > menu_info = &mapping->menu_info[query_menu->index]; > > - if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK && > - (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)) { > - s32 bitmap; > - > + if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) { > if (!ctrl->cached) { > ret = uvc_ctrl_populate_cache(chain, ctrl); > if (ret < 0) > goto done; > } > > - bitmap = mapping->get(mapping, UVC_GET_RES, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); > - if (!(bitmap & menu_info->value)) { > + if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & menu_info->value)) { > ret = -EINVAL; > goto done; > } > @@ -1831,6 +1851,17 @@ int uvc_ctrl_set(struct uvc_fh *handle, > value = xctrl->value; > break; > > + case V4L2_CTRL_TYPE_BITMASK: > + if (!ctrl->cached) { > + ret = uvc_ctrl_populate_cache(chain, ctrl); > + if (ret < 0) > + return ret; > + } > + > + xctrl->value &= uvc_get_ctrl_bitmap(ctrl, mapping); > + value = xctrl->value; > + break; > + > case V4L2_CTRL_TYPE_BOOLEAN: > xctrl->value = clamp(xctrl->value, 0, 1); > value = xctrl->value; > @@ -1845,17 +1876,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > * Valid menu indices are reported by the GET_RES request for > * UVC controls that support it. > */ > - if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK && > - (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)) { > + if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) { > if (!ctrl->cached) { > ret = uvc_ctrl_populate_cache(chain, ctrl); > if (ret < 0) > return ret; > } > > - step = mapping->get(mapping, UVC_GET_RES, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); > - if (!(step & value)) > + if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & value)) > return -EINVAL; > } > > -- Regards, Laurent Pinchart