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