media: uvcvideo: Override default flags

Message ID 20240602065053.36850-1-dhs@frame.work (mailing list archive)
State Accepted
Delegated to: Laurent Pinchart
Headers
Series media: uvcvideo: Override default flags |

Commit Message

Daniel Schaefer June 2, 2024, 6:50 a.m. UTC
  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>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
  

Comments

Ricardo Ribalda June 3, 2024, 11:49 a.m. UTC | #1
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);
+       }


>
>         kfree(data);
>         return ret;
> --
> 2.43.0
>
>
  
Daniel Schaefer June 5, 2024, 7:12 a.m. UTC | #2
On Mon, Jun 3, 2024 at 7:50 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Daniel
>
> Thanks for the patch. Some minor nits.
>
> Feel free to ignore it if you prefer your style.
>

Sure, that's also a nice way to do it. I'm okay with either one,
whatever the media subsystem maintainers prefer.
  
Laurent Pinchart June 16, 2024, 11:20 p.m. UTC | #3
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;
  

Patch

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);
+	}
 
 	kfree(data);
 	return ret;