diff mbox series

[v2] media: uvcvideo: Fix handling on Bitmask controls

Message ID 20220215184228.2531386-1-ribalda@chromium.org
State New
Headers show
Series [v2] media: uvcvideo: Fix handling on Bitmask controls | expand

Commit Message

Ricardo Ribalda Feb. 15, 2022, 6:42 p.m. UTC 
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

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart March 24, 2022, 8:25 p.m. UTC | #1 
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)
Ricardo Ribalda March 24, 2022, 9:56 p.m. UTC | #2 
Hi Laurent

Thanks for your review :) :)

On Thu, 24 Mar 2022 at 21:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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.

I am particularly looking at bmAutoControls from CT_REGION_OF_INTEREST_CONTROL

The doc says that:
"""
 To detect if a device supports a particular Auto Control, use GET_MAX
which returns a mask indicating all supported Auto Controls.
"""

GET_RES does not seem to return the max_value accoring to the uvc
class spec, but I will try to validate tomorrow with real hardware,
maybe we are lucky.

And I definitely have to fix the clamp, thanks for pointing that out :)

>
> > 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
Laurent Pinchart March 24, 2022, 10:18 p.m. UTC | #3 
Hi Ricardo,

On Thu, Mar 24, 2022 at 10:56:37PM +0100, Ricardo Ribalda wrote:
> On Thu, 24 Mar 2022 at 21:25, Laurent Pinchart wrote:
> > 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.
> 
> I am particularly looking at bmAutoControls from CT_REGION_OF_INTEREST_CONTROL
> 
> The doc says that:
> """
>  To detect if a device supports a particular Auto Control, use GET_MAX
> which returns a mask indicating all supported Auto Controls.
> """

But if you look at CT_AE_MODE_CONTROL,

"A GET_RES request issued to this control will return a bitmap of the
modes supported by this control. A valid request to this control would
have only one bit set (a single mode selected)."

GET_MIN and GET_MAX are not listed as supported.

Maybe we need two types of UVC bitmap controls ? :-(

> GET_RES does not seem to return the max_value accoring to the uvc
> class spec, but I will try to validate tomorrow with real hardware,
> maybe we are lucky.
> 
> And I definitely have to fix the clamp, thanks for pointing that out :)
> 
> > > 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)
diff mbox series

Patch

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)