Re: [PATCH] media: uvcvideo: Override default flags

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

On Mon, Jun 03, 2024 at 01:49:51PM +0200, Ricardo Ribalda wrote:
> Hi Daniel
> 
> Thanks for the patch. Some minor nits.
> 
> Feel free to ignore it if you prefer your style.
> 
> On Sun, 2 Jun 2024 at 08:52, Daniel Schaefer <dhs@frame.work> wrote:
> >
> > When the UVC device has a control that is readonly it doesn't set the
> > SET_CUR flag. For example the privacy control has SET_CUR flag set in
> > the defaults in the `uvc_ctrls` variable. Even if the device does not
> > have it set, it's not cleared by uvc_ctrl_get_flags.
> >
> > Originally written with assignment in commit 859086ae3636 ("media:
> > uvcvideo: Apply flags from device to actual properties"). But changed to
> > |= in commit 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable
> > flags"). It would not clear the default flags.
> >
> > With this patch applied the correct flags are reported to user space.
> > Tested with:
> >
> > ```
> > > v4l2-ctl --list-ctrls | grep privacy
> > privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only
> > ```
> >
> > Cc: Edgar Thier <info@edgarthier.net>
> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Kieran Levin <ktl@frame.work>
> > Signed-off-by: Daniel Schaefer <dhs@frame.work>
> Fixes: 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable flags")
> 
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4b685f883e4d..f50542e26542 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2031,15 +2031,23 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> >         else
> >                 ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> >                                      dev->intfnum, info->selector, data, 1);
> > -       if (!ret)
> > -               info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> > -                               UVC_CTRL_FLAG_GET_CUR : 0)
> > -                           |  (data[0] & UVC_CONTROL_CAP_SET ?
> > -                               UVC_CTRL_FLAG_SET_CUR : 0)
> > -                           |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > -                               UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> > -                           |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> > -                               UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> > +       if (!ret) {
> > +               info->flags = (data[0] & UVC_CONTROL_CAP_GET)
> > +                       ? (info->flags | UVC_CTRL_FLAG_GET_CUR)
> > +                       : (info->flags & ~UVC_CTRL_FLAG_GET_CUR);
> > +
> > +               info->flags = (data[0] & UVC_CONTROL_CAP_SET)
> > +                       ? (info->flags | UVC_CTRL_FLAG_SET_CUR)
> > +                       : (info->flags & ~UVC_CTRL_FLAG_SET_CUR);
> > +
> > +               info->flags = (data[0] & UVC_CONTROL_CAP_AUTOUPDATE)
> > +                       ? (info->flags | UVC_CTRL_FLAG_AUTO_UPDATE)
> > +                       : (info->flags & ~UVC_CTRL_FLAG_AUTO_UPDATE);
> > +
> > +               info->flags = (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS)
> > +                       ? (info->flags | UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +                       : (info->flags & ~UVC_CTRL_FLAG_ASYNCHRONOUS);
> > +       }
> 
> nit: I would have done it as
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4b685f883e4d..c453a67e1407 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2031,7 +2031,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>         else
>                 ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
>                                      dev->intfnum, info->selector, data, 1);
> -       if (!ret)
> +       if (!ret) {
> +               info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
> +                               UVC_CTRL_FLAG_SET_CUR |
> +                               UVC_CTRL_FLAG_AUTO_UPDATE |
> +                               UVC_CTRL_FLAG_ASYNCHRONOUS);
> +
>                 info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
>                                 UVC_CTRL_FLAG_GET_CUR : 0)
>                             |  (data[0] & UVC_CONTROL_CAP_SET ?
> @@ -2040,6 +2045,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>                                 UVC_CTRL_FLAG_AUTO_UPDATE : 0)
>                             |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
>                                 UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> +       }

I prefer that slightly too.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll make the change in my tree, no need to send a v2.

> >
> >         kfree(data);
> >         return ret;

-- 
Regards,

Laurent Pinchart