Re: [PATCH v3 5/8] media: uvcvideo: Fix handling on Bitmask controls

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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