diff mbox series

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

Message ID 20220920-resend-v4l2-compliance-v2-5-b0ceb15353ac@chromium.org
State New
Headers
Series Follow-up patches for uvc v4l2-compliance |

Commit Message

Ricardo Ribalda Dec. 2, 2022, 5:21 p.m. UTC 
  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>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 53 +++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Dec. 29, 2022, 9:25 p.m. UTC | #1 
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;
>  		}
>  
>
diff mbox series

Patch

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.
+	 */
+	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));
+
+	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);
+		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;
 		}