[v1] media: uvcvideo: Fix power line control for a Chicony camera

[PATCH] media: uvcvideo: Fix power line control for a Chicony camera

Posted by Ricardo Ribalda 2 years, 4 months ago

The device does not implement the control properly.

Fixes vl2-compliance error:

info: checking control 'Power Line Frequency' (0x00980918)
fail: v4l2-test-controls.cpp(552): could not set valid menu item 3

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
This camera, like other Chicony devices, do not implement properly the
Power Line Frequency control.

This time, I do not have direct access to the device, just to the
report, but since other devices from the same family are showing the
same error, it is safe to assume that the same fix will work here.
---
 drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 08fcd2ffa727..db2556e95b72 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2592,6 +2592,15 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
+	/* Chicony Electronics Co., Ltd */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x04f2,
+	  .idProduct		= 0xb67c,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
 	/* Chicony EasyCamera */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,

---
base-commit: 4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6
change-id: 20230817-chicony-9c35f2046c6f

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH] media: uvcvideo: Fix power line control for a Chicony camera

Posted by Laurent Pinchart 2 years, 4 months ago

Hi Ricardo,

Thank you for the patch.

On Thu, Aug 17, 2023 at 12:38:04PM +0000, Ricardo Ribalda wrote:
> The device does not implement the control properly.
> 
> Fixes vl2-compliance error:
> 
> info: checking control 'Power Line Frequency' (0x00980918)
> fail: v4l2-test-controls.cpp(552): could not set valid menu item 3
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> This camera, like other Chicony devices, do not implement properly the
> Power Line Frequency control.
> 
> This time, I do not have direct access to the device, just to the
> report, but since other devices from the same family are showing the
> same error, it is safe to assume that the same fix will work here.

Why, ô why does UVC not provide a way to query this dynamically ? :-( Of
course, even if it did, I'm sure vendors would get it wrong... It sounds
like the Windows UVC compliance test suite must be a joke.

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 08fcd2ffa727..db2556e95b72 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2592,6 +2592,15 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> +	/* Chicony Electronics Co., Ltd */

I'll write

	/* Chicony Electronics Co., Ltd Integrated Camera */

as that's what the descriptors expose. Is this integrated in a
chromebook by any chance ? If so, could you share which model the camera
is found in, and can I add that to the comment ?

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

> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x04f2,
> +	  .idProduct		= 0xb67c,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
>  	/* Chicony EasyCamera */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> 
> ---
> base-commit: 4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6
> change-id: 20230817-chicony-9c35f2046c6f

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Fix power line control for a Chicony camera

Posted by Ricardo Ribalda 2 years, 4 months ago

Hi Laurent

On Fri, 18 Aug 2023 at 10:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Aug 17, 2023 at 12:38:04PM +0000, Ricardo Ribalda wrote:
> > The device does not implement the control properly.
> >
> > Fixes vl2-compliance error:
> >
> > info: checking control 'Power Line Frequency' (0x00980918)
> > fail: v4l2-test-controls.cpp(552): could not set valid menu item 3
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > This camera, like other Chicony devices, do not implement properly the
> > Power Line Frequency control.
> >
> > This time, I do not have direct access to the device, just to the
> > report, but since other devices from the same family are showing the
> > same error, it is safe to assume that the same fix will work here.
>
> Why, ô why does UVC not provide a way to query this dynamically ? :-( Of
> course, even if it did, I'm sure vendors would get it wrong... It sounds
> like the Windows UVC compliance test suite must be a joke.
>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 08fcd2ffa727..db2556e95b72 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2592,6 +2592,15 @@ static const struct usb_device_id uvc_ids[] = {
> >         .bInterfaceSubClass   = 1,
> >         .bInterfaceProtocol   = 0,
> >         .driver_info          = (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> > +     /* Chicony Electronics Co., Ltd */
>
> I'll write
>
>         /* Chicony Electronics Co., Ltd Integrated Camera */
>
> as that's what the descriptors expose. Is this integrated in a
> chromebook by any chance ? If so, could you share which model the camera
> is found in, and can I add that to the comment ?

It is a non-chromebook running ChromeOS Flex.
https://chromeenterprise.google/os/chromeosflex/

I believe it is a thinkpad, but I am not sure the exact model.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > +       .idVendor             = 0x04f2,
> > +       .idProduct            = 0xb67c,
> > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > +       .bInterfaceSubClass   = 1,
> > +       .bInterfaceProtocol   = 0,
> > +       .driver_info          = (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> >       /* Chicony EasyCamera */
> >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> >
> > ---
> > base-commit: 4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6
> > change-id: 20230817-chicony-9c35f2046c6f
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Fix power line control for a Chicony camera

Posted by Laurent Pinchart 2 years, 4 months ago

On Fri, Aug 18, 2023 at 11:15:47AM +0300, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Thu, Aug 17, 2023 at 12:38:04PM +0000, Ricardo Ribalda wrote:
> > The device does not implement the control properly.
> > 
> > Fixes vl2-compliance error:

I will also s/vl2/v4l2/ locally :-)

> > 
> > info: checking control 'Power Line Frequency' (0x00980918)
> > fail: v4l2-test-controls.cpp(552): could not set valid menu item 3
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > This camera, like other Chicony devices, do not implement properly the
> > Power Line Frequency control.
> > 
> > This time, I do not have direct access to the device, just to the
> > report, but since other devices from the same family are showing the
> > same error, it is safe to assume that the same fix will work here.
> 
> Why, ô why does UVC not provide a way to query this dynamically ? :-( Of
> course, even if it did, I'm sure vendors would get it wrong... It sounds
> like the Windows UVC compliance test suite must be a joke.
> 
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 08fcd2ffa727..db2556e95b72 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2592,6 +2592,15 @@ static const struct usb_device_id uvc_ids[] = {
> >  	  .bInterfaceSubClass	= 1,
> >  	  .bInterfaceProtocol	= 0,
> >  	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> > +	/* Chicony Electronics Co., Ltd */
> 
> I'll write
> 
> 	/* Chicony Electronics Co., Ltd Integrated Camera */
> 
> as that's what the descriptors expose. Is this integrated in a
> chromebook by any chance ? If so, could you share which model the camera
> is found in, and can I add that to the comment ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> > +				| USB_DEVICE_ID_MATCH_INT_INFO,
> > +	  .idVendor		= 0x04f2,
> > +	  .idProduct		= 0xb67c,
> > +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> > +	  .bInterfaceSubClass	= 1,
> > +	  .bInterfaceProtocol	= 0,
> > +	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> >  	/* Chicony EasyCamera */
> >  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> >  				| USB_DEVICE_ID_MATCH_INT_INFO,
> > 
> > ---
> > base-commit: 4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6
> > change-id: 20230817-chicony-9c35f2046c6f

-- 
Regards,

Laurent Pinchart