Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 24 Mar 2022 22:25:02 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>
Subject: Re: [PATCH v2] media: uvcvideo: Fix handling on Bitmask controls
Message-ID: <YjzTnlf92rY/X6Lv@pendragon.ideasonboard.com>
References: <20220215184228.2531386-1-ribalda@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220215184228.2531386-1-ribalda@chromium.org>
Precedence: bulk

Hi Ricardo,

Thank you for the patch.

On Tue, Feb 15, 2022 at 06:42:28PM +0000, 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 clamped to the min/max value advertised by the
> hardware.
> 
> 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

What bitmask control do you have ? The driver has no standard control
that use the V4L2_CTRL_TYPE_BITMASK type.

UVC doesn't formally define bitmask control type
(UVC_CTRL_DATA_TYPE_BITMASK). In UVC 1.1 only the UVC_CT_AE_MODE_CONTROL
control has a bitmap type, and only one bit can be set at a type. It
thus maps to a V4L2 menu control.

In UVC 1.5 there are other controls documented as bitmap controls,
which could map to the V4L2 bitmask control type. Those don't support
GET_MIN and GET_MAX, but use GET_RES to report the list of bits that can
be set. This should be mapped to the V4L2 control maximum value, which
isn't handled by this patch. The last hunk is also incorrect, as it
clamps the value to what is reported by GET_MIN and GET_MAX, instead of
[0, GET_RES], but more than that, it should not just clamp the value,
but check that all bits are valid.

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b4f6edf968bc0..d8b9ab5b7fb85 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1156,7 +1156,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  		break;
>  	}
>  
> -	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN &&
> +	    mapping->v4l2_type != V4L2_CTRL_TYPE_BITMASK)
>  		v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
>  				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
>  
> @@ -1164,7 +1165,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  		v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX,
>  				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
>  
> -	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES &&
> +	    mapping->v4l2_type != V4L2_CTRL_TYPE_BITMASK)
>  		v4l2_ctrl->step = mapping->get(mapping, UVC_GET_RES,
>  				  uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
>  
> @@ -1721,6 +1723,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>  	/* Clamp out of range values. */
>  	switch (mapping->v4l2_type) {
>  	case V4L2_CTRL_TYPE_INTEGER:
> +	case V4L2_CTRL_TYPE_BITMASK:
>  		if (!ctrl->cached) {
>  			ret = uvc_ctrl_populate_cache(chain, ctrl);
>  			if (ret < 0)

-- 
Regards,

Laurent Pinchart