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
