lkml.org 
[lkml]   [2022]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RESEND v2 5/7] media: uvcvideo: Fix handling on Bitmask controls
    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

    \
     
     \ /
      Last update: 2023-03-26 23:23    [W:4.177 / U:0.584 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site