Re: [PATCH RESEND v2 5/7] media: uvcvideo: Fix handling on Bitmask controls

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

Hi Ricardo,

Thank you for the patch.

On Fri, Dec 02, 2022 at 06:21:39PM +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

s/uvc/UVC/

> 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>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 53 +++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7153ee5aabb1..526572044e82 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1145,6 +1145,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.

You can reflow to 80 columns.

> +	 */
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX)
> +		return mapping->get(mapping, UVC_GET_MAX,
> +				    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> +
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)
> +		return mapping->get(mapping, UVC_GET_RES,
> +				    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));

Should we start with GET_RES to avoid any regression in case of controls
that support both GET_RES and GET_MAX ?

> +
> +	return ~0;
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl,
>  	struct uvc_control_mapping *mapping,
> @@ -1219,6 +1238,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;
>  	}
> @@ -1320,19 +1345,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;
>  		}
> @@ -1815,6 +1835,18 @@ 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 = max(0, xctrl->value);

This would prevent bit 31 from being set. It doesn't matter for the
controls currently supported by the driver, but is it needed ?

> +		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;
> @@ -1829,17 +1861,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