[PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
which allows the user to control the exposure time of the webcam,
essentially controlling the brightness of the received image.  By default
the webcam automatically adjusts the exposure time automatically but the
if you set the control "Exposure, Auto"="Manual Mode" the user can fix
the exposure time.

Unfortunately it seems that the Logitech C920 has a firmware bug where
it will forget that it's in manual mode temporarily during initialisation.
This means that the camera doesn't respect the exposure time that the user
requested if they request it before starting to stream video.  They end up
with a video stream which is either too bright or too dark and must reset
the controls after video starts streaming.

This patch works around this camera bug by re-uploading the cached
controls to the camera immediately after initialising the camera.

Signed-off-by: William Manley <will@williammanley.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 2 +-
 drivers/media/usb/uvc/uvc_driver.c | 2 +-
 drivers/media/usb/uvc/uvc_video.c  | 5 +++++
 drivers/media/usb/uvc/uvcvideo.h   | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a2f4501..f72d7eb 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1795,7 +1795,7 @@ done:
  * - Handle restore order (Auto-Exposure Mode should be restored before
  *   Exposure Time).
  */
-int uvc_ctrl_resume_device(struct uvc_device *dev)
+int uvc_ctrl_restore_values(struct uvc_device *dev)
 {
 	struct uvc_control *ctrl;
 	struct uvc_entity *entity;
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index c3bb250..9f8a87e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 		int ret = 0;
 
 		if (reset) {
-			ret = uvc_ctrl_resume_device(dev);
+			ret = uvc_ctrl_restore_values(dev);
 			if (ret < 0)
 				return ret;
 		}
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 3394c34..87cd57b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1660,6 +1660,11 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
 		}
 	}
 
+	/* The Logitech C920 temporarily forgets that it should not be
+	   adjusting Exposure Absolute during init so restore controls to
+	   stored values. */
+	uvc_ctrl_restore_values(stream->dev);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9e35982..3b365a3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -676,7 +676,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		const struct uvc_control_mapping *mapping);
 extern int uvc_ctrl_init_device(struct uvc_device *dev);
 extern void uvc_ctrl_cleanup_device(struct uvc_device *dev);
-extern int uvc_ctrl_resume_device(struct uvc_device *dev);
+extern int uvc_ctrl_restore_values(struct uvc_device *dev);
 
 extern int uvc_ctrl_begin(struct uvc_video_chain *chain);
 extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
-- 
1.9.0

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Laurent Pinchart]

Hi William,

Thank you for the patch.

First of all, could you please CC me in the future for uvcvideo patches ?

On Wednesday 12 March 2014 18:08:31 William Manley wrote:
> The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
> which allows the user to control the exposure time of the webcam,
> essentially controlling the brightness of the received image.  By default
> the webcam automatically adjusts the exposure time automatically but the
> if you set the control "Exposure, Auto"="Manual Mode" the user can fix
> the exposure time.
> 
> Unfortunately it seems that the Logitech C920 has a firmware bug where
> it will forget that it's in manual mode temporarily during initialisation.
> This means that the camera doesn't respect the exposure time that the user
> requested if they request it before starting to stream video.  They end up
> with a video stream which is either too bright or too dark and must reset
> the controls after video starts streaming.

I've asked Logitech whether they can confirm this is a known issue. I'm not 
sure when I'll have a reply though.

> This patch works around this camera bug by re-uploading the cached
> controls to the camera immediately after initialising the camera.

I'm a bit concerned about this. As you noticed UVC camera are often buggy, and 
small changes in the driver can fix problems with one model and break others. 
Sending a bunch of SET_CUR requests at once right after starting the stream is 
something that has the potential to crash firmwares (yes, they can be that 
fragile, unfortunately).

I would like to get a better understanding of the problem first. As I don't 
have a C920, could you please perform two tests for me ?

I would first like to know what the camera reports as its exposure time after 
starting the stream. If you get the exposure time at that point (by sending a 
GET_CUR request, bypassing the driver cache), do you get the value you had 
previously set (which, from your explanation, would be incorrect, as the 
exposure time has changed based on your findings), or a different value ? Does 
the camera change the exposure priority control autonomously as well, or just 
the exposure time ?

Then, I would like to know whether the camera sends a control update event 
when you start the stream, or if it just changes the exposure time without 
notifying the driver.

> Signed-off-by: William Manley <will@williammanley.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   | 2 +-
>  drivers/media/usb/uvc/uvc_driver.c | 2 +-
>  drivers/media/usb/uvc/uvc_video.c  | 5 +++++
>  drivers/media/usb/uvc/uvcvideo.h   | 2 +-
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index a2f4501..f72d7eb 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1795,7 +1795,7 @@ done:
>   * - Handle restore order (Auto-Exposure Mode should be restored before
>   *   Exposure Time).
>   */
> -int uvc_ctrl_resume_device(struct uvc_device *dev)
> +int uvc_ctrl_restore_values(struct uvc_device *dev)
>  {
>  	struct uvc_control *ctrl;
>  	struct uvc_entity *entity;
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index c3bb250..9f8a87e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf,
> int reset) int ret = 0;
> 
>  		if (reset) {
> -			ret = uvc_ctrl_resume_device(dev);
> +			ret = uvc_ctrl_restore_values(dev);
>  			if (ret < 0)
>  				return ret;
>  		}
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 3394c34..87cd57b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1660,6 +1660,11 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags) }
>  	}
> 
> +	/* The Logitech C920 temporarily forgets that it should not be
> +	   adjusting Exposure Absolute during init so restore controls to
> +	   stored values. */
> +	uvc_ctrl_restore_values(stream->dev);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 9e35982..3b365a3 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -676,7 +676,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain
> *chain, const struct uvc_control_mapping *mapping);
>  extern int uvc_ctrl_init_device(struct uvc_device *dev);
>  extern void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> -extern int uvc_ctrl_resume_device(struct uvc_device *dev);
> +extern int uvc_ctrl_restore_values(struct uvc_device *dev);
> 
>  extern int uvc_ctrl_begin(struct uvc_video_chain *chain);
>  extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Will Manley]

On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote:
> First of all, could you please CC me in the future for uvcvideo patches ?

Will do.

> On Wednesday 12 March 2014 18:08:31 William Manley wrote:
> > The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
> > which allows the user to control the exposure time of the webcam,
> > essentially controlling the brightness of the received image.  By default
> > the webcam automatically adjusts the exposure time automatically but the
> > if you set the control "Exposure, Auto"="Manual Mode" the user can fix
> > the exposure time.
> > 
> > Unfortunately it seems that the Logitech C920 has a firmware bug where
> > it will forget that it's in manual mode temporarily during initialisation.
> > This means that the camera doesn't respect the exposure time that the user
> > requested if they request it before starting to stream video.  They end up
> > with a video stream which is either too bright or too dark and must reset
> > the controls after video starts streaming.
> 
> I've asked Logitech whether they can confirm this is a known issue. I'm
> not 
> sure when I'll have a reply though.

Great :)

> > This patch works around this camera bug by re-uploading the cached
> > controls to the camera immediately after initialising the camera.
> 
> I'm a bit concerned about this. As you noticed UVC camera are often
> buggy, and 
> small changes in the driver can fix problems with one model and break
> others. 
> Sending a bunch of SET_CUR requests at once right after starting the
> stream is 
> something that has the potential to crash firmwares (yes, they can be
> that 
> fragile, unfortunately).

Good point.  I can add a quirk such that it only happens with the C920.

> I would like to get a better understanding of the problem first. As I
> don't 
> have a C920, could you please perform two tests for me ?
> 
> I would first like to know what the camera reports as its exposure time
> after 
> starting the stream. If you get the exposure time at that point (by
> sending a 
> GET_CUR request, bypassing the driver cache), do you get the value you
> had 
> previously set (which, from your explanation, would be incorrect, as the 
> exposure time has changed based on your findings), or a different value ?
> Does 
> the camera change the exposure priority control autonomously as well, or
> just 
> the exposure time ?

It's a bit of a strange behaviour.  I'd already tried littering the code
with (uncached) GET_CUR requests.  It seems that the value changes
sometime during the call to usb_set_interface in uvc_init_video. 
Strangely enough though calling uvc_ctrl_restore_values immediately
after uvc_init_video has no effect.  It must be put after the
usb_submit_urb loop to fix the problem.

> Then, I would like to know whether the camera sends a control update
> event 
> when you start the stream, or if it just changes the exposure time
> without 
> notifying the driver.

Wireshark tells me that it is sending a control update event, but it
seems like uvcvideo ignores it.  I had to add the flag
UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure
(Auto)" for the new value to be properly reported to userspace.

Thanks

Will

[PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
which allows the user to control the exposure time of the webcam,
essentially controlling the brightness of the received image.  By default
the webcam automatically adjusts the exposure time automatically but the
if you set the control "Exposure, Auto"="Manual Mode" the user can fix
the exposure time.

Unfortunately it seems that the Logitech C920 has a firmware bug where
it will forget that it's in manual mode temporarily during initialisation.
This means that the camera doesn't respect the exposure time that the user
requested if they request it before starting to stream video.  They end up
with a video stream which is either too bright or too dark and must reset
the controls after video starts streaming.

This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
causes the cached controls to be re-uploaded to the camera immediately
after initialising the camera.  This quirk is applied to the C920 to work
around this camera bug.

Changes since patch v1:
 * Introduce quirk so workaround is only applied to the C920.

Signed-off-by: William Manley <will@williammanley.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  2 +-
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++++++++-
 drivers/media/usb/uvc/uvc_video.c  |  6 ++++++
 drivers/media/usb/uvc/uvcvideo.h   |  3 ++-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a2f4501..f72d7eb 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1795,7 +1795,7 @@ done:
  * - Handle restore order (Auto-Exposure Mode should be restored before
  *   Exposure Time).
  */
-int uvc_ctrl_resume_device(struct uvc_device *dev)
+int uvc_ctrl_restore_values(struct uvc_device *dev)
 {
 	struct uvc_control *ctrl;
 	struct uvc_entity *entity;
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index c3bb250..d3a9c3b 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 		int ret = 0;
 
 		if (reset) {
-			ret = uvc_ctrl_resume_device(dev);
+			ret = uvc_ctrl_restore_values(dev);
 			if (ret < 0)
 				return ret;
 		}
@@ -2156,6 +2156,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* Logitech HD Pro Webcam C920 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x046d,
+	  .idProduct		= 0x082d,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_RESTORE_CTRLS_ON_INIT },
 	/* Chicony CNF7129 (Asus EEE 100HE) */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 3394c34..85ff6b8 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1660,6 +1660,12 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
 		}
 	}
 
+	/* The Logitech C920 temporarily forgets that it should not be
+	   adjusting Exposure Absolute during init so restore controls to
+	   stored values. */
+	if (stream->dev->quirks & UVC_QUIRK_RESTORE_CTRLS_ON_INIT)
+		uvc_ctrl_restore_values(stream->dev);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9e35982..0f54376 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -137,6 +137,7 @@
 #define UVC_QUIRK_FIX_BANDWIDTH		0x00000080
 #define UVC_QUIRK_PROBE_DEF		0x00000100
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
+#define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
@@ -676,7 +677,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		const struct uvc_control_mapping *mapping);
 extern int uvc_ctrl_init_device(struct uvc_device *dev);
 extern void uvc_ctrl_cleanup_device(struct uvc_device *dev);
-extern int uvc_ctrl_resume_device(struct uvc_device *dev);
+extern int uvc_ctrl_restore_values(struct uvc_device *dev);
 
 extern int uvc_ctrl_begin(struct uvc_video_chain *chain);
 extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
-- 
1.9.0

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Laurent Pinchart]

Hi Will,

On Thursday 13 March 2014 10:48:20 Will Manley wrote:
> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote:
> > First of all, could you please CC me in the future for uvcvideo patches ?
> 
> Will do.

Thank you.

> > On Wednesday 12 March 2014 18:08:31 William Manley wrote:
> > > The uvcvideo webcam driver exposes the v4l2 control "Exposure
> > > (Absolute)" which allows the user to control the exposure time of the
> > > webcam, essentially controlling the brightness of the received image. 
> > > By default the webcam automatically adjusts the exposure time
> > > automatically but the if you set the control "Exposure, Auto"="Manual
> > > Mode" the user can fix the exposure time.
> > > 
> > > Unfortunately it seems that the Logitech C920 has a firmware bug where
> > > it will forget that it's in manual mode temporarily during
> > > initialisation. This means that the camera doesn't respect the exposure
> > > time that the user requested if they request it before starting to
> > > stream video. They end up with a video stream which is either too bright
> > > or too dark and must reset the controls after video starts streaming.
> > 
> > I've asked Logitech whether they can confirm this is a known issue. I'm
> > not sure when I'll have a reply though.
> 
> Great :)
> 
> > > This patch works around this camera bug by re-uploading the cached
> > > controls to the camera immediately after initialising the camera.
> > 
> > I'm a bit concerned about this. As you noticed UVC camera are often buggy,
> > and small changes in the driver can fix problems with one model and break
> > others. Sending a bunch of SET_CUR requests at once right after starting
> > the stream is something that has the potential to crash firmwares (yes,
> > they can be that fragile, unfortunately).
> 
> Good point.  I can add a quirk such that it only happens with the C920.
> 
> > I would like to get a better understanding of the problem first. As I
> > don't have a C920, could you please perform two tests for me ?
> > 
> > I would first like to know what the camera reports as its exposure time
> > after starting the stream. If you get the exposure time at that point (by
> > sending a GET_CUR request, bypassing the driver cache), do you get the
> > value you had previously set (which, from your explanation, would be
> > incorrect, as the exposure time has changed based on your findings), or a
> > different value ? Does the camera change the exposure priority control
> > autonomously as well, or just the exposure time ?
> 
> It's a bit of a strange behaviour. I'd already tried littering the code with
> (uncached) GET_CUR requests. It seems that the value changes sometime during
> the call to usb_set_interface in uvc_init_video.

I'll assume this means that the camera reports the updated exposure time in 
response to the GET_CUR request. Does the value of other controls (such as the 
exposure priority control for instance) change as well ?

> Strangely enough though calling uvc_ctrl_restore_values immediately after
> uvc_init_video has no effect. It must be put after the usb_submit_urb loop
> to fix the problem.
> 
> > Then, I would like to know whether the camera sends a control update
> > event when you start the stream, or if it just changes the exposure time
> > without notifying the driver.
> 
> Wireshark tells me that it is sending a control update event, but it seems
> like uvcvideo ignores it. I had to add the flag UVC_CTRL_FLAG_AUTO_UPDATE to
> the uvc_control_info entry for "Exposure (Auto)" for the new value to be
> properly reported to userspace.

Could you send me the USB trace ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

On 13/03/14 17:03, Laurent Pinchart wrote:
> On Thursday 13 March 2014 10:48:20 Will Manley wrote:
>> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote:
>>> On Wednesday 12 March 2014 18:08:31 William Manley wrote:
>>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure
>>>> (Absolute)" which allows the user to control the exposure time of the
>>>> webcam, essentially controlling the brightness of the received image. 
>>>> By default the webcam automatically adjusts the exposure time
>>>> automatically but the if you set the control "Exposure, Auto"="Manual
>>>> Mode" the user can fix the exposure time.
>>>>
>>>> Unfortunately it seems that the Logitech C920 has a firmware bug where
>>>> it will forget that it's in manual mode temporarily during
>>>> initialisation. This means that the camera doesn't respect the exposure
>>>> time that the user requested if they request it before starting to
>>>> stream video. They end up with a video stream which is either too bright
>>>> or too dark and must reset the controls after video starts streaming.
>>>
>>> I would like to get a better understanding of the problem first. As I
>>> don't have a C920, could you please perform two tests for me ?
>>>
>>> I would first like to know what the camera reports as its exposure time
>>> after starting the stream. If you get the exposure time at that point (by
>>> sending a GET_CUR request, bypassing the driver cache), do you get the
>>> value you had previously set (which, from your explanation, would be
>>> incorrect, as the exposure time has changed based on your findings), or a
>>> different value ? Does the camera change the exposure priority control
>>> autonomously as well, or just the exposure time ?
>>
>> It's a bit of a strange behaviour. I'd already tried littering the code with
>> (uncached) GET_CUR requests. It seems that the value changes sometime during
>> the call to usb_set_interface in uvc_init_video.
> 
> I'll assume this means that the camera reports the updated exposure time in 
> response to the GET_CUR request.

That's right

> Does the value of other controls (such as the 
> exposure priority control for instance) change as well ?

No, I've not seen any of the other controls change.

>> Strangely enough though calling uvc_ctrl_restore_values immediately after
>> uvc_init_video has no effect. It must be put after the usb_submit_urb loop
>> to fix the problem.
>>
>>> Then, I would like to know whether the camera sends a control update
>>> event when you start the stream, or if it just changes the exposure time
>>> without notifying the driver.
>>
>> Wireshark tells me that it is sending a control update event, but it seems
>> like uvcvideo ignores it. I had to add the flag UVC_CTRL_FLAG_AUTO_UPDATE to
>> the uvc_control_info entry for "Exposure (Auto)" for the new value to be
>> properly reported to userspace.
> 
> Could you send me the USB trace ?

I've uploaded 3 traces: one with no patch (kernel 3.12) one with my
"PATCH v2" applied and one with no patch but caching of gets disabled.
You can get them here:

    http://williammanley.net/C920-no-patch.pcapng
    http://williammanley.net/C920-with-patch.pcapng
    http://williammanley.net/C920-no-caching.pcapng

The process to generate them was:

    sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng &

    # Plug USB cable in here

    v4l2-ctl -d
/dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1

-cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balance_temperature_auto=0

    ./streamon

The relevant output seems to be (here from C920-no-patch.pcapng):

> 10446 23.095874000 21:14:16.313257000         host -> 32.0         USB 64 SET INTERFACE Request
> 10447 23.109379000 21:14:16.326762000         32.3 -> host         USBVIDEO 70 URB_INTERRUPT in
> 10448 23.109389000 21:14:16.326772000         host -> 32.3         USB 64 URB_INTERRUPT in
> 10449 23.189448000 21:14:16.406831000         32.3 -> host         USBVIDEO 73 URB_INTERRUPT in
> 10450 23.189486000 21:14:16.406869000         host -> 32.3         USB 64 URB_INTERRUPT in
> 10451 23.243314000 21:14:16.460697000         32.0 -> host         USB 64 SET INTERFACE Response

Taking a closer look at packet 10449:

> Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on interface 0
>     Interface id: 0
>     Encapsulation type: USB packets with Linux header and padding (115)
>     Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT
>     [Time shift for this packet: 0.000000000 seconds]
>     Epoch Time: 1394745256.406831000 seconds
>     [Time delta from previous captured frame: 0.080059000 seconds]
>     [Time delta from previous displayed frame: 0.080059000 seconds]
>     [Time since reference or first frame: 23.189448000 seconds]
>     Frame Number: 10449
>     Frame Length: 73 bytes (584 bits)
>     Capture Length: 73 bytes (584 bits)
>     [Frame is marked: False]
>     [Frame is ignored: False]
>     [Protocols in frame: usb:usbvideo]
> USB URB
>     URB id: 0xffff88081bf95240
>     URB type: URB_COMPLETE ('C')
>     URB transfer type: URB_INTERRUPT (0x01)
>     Endpoint: 0x83, Direction: IN
>         1... .... = Direction: IN (1)
>         .000 0011 = Endpoint value: 3
>     Device: 32
>     URB bus id: 1
>     Device setup request: not relevant ('-')
>     Data: present (0)
>     URB sec: 1394745256
>     URB usec: 406831
>     URB status: Success (0)
>     URB length [bytes]: 9
>     Data length [bytes]: 9
>     [Request in: 10448]
>     [Time from request: 0.080059000 seconds]
>     [bInterfaceClass: Video (0x0e)]
> .... 0001 = Status Type: VideoControl Interface (0x01)
> Originator: 1
> Event: Control Change (0x00)
> Control Selector: Exposure Time (Absolute) (0x04)
> Change Type: Value (0x00)
> Current value: 333 (0x0000014d)

This is the notification of the change of the value.  As you can see it
happens between "SET INTERFACE Request" and "SET INTERFACE Response".

The sources for "streamon" look like:

     #include <linux/videodev2.h>
     #include <stdio.h>
     #include <stdlib.h>
     #include <fcntl.h>
     #include <errno.h>
     #include <string.h>
     #include <unistd.h>
     #include <sys/ioctl.h>

     #define DEVICE
"/dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1"

     int main()
     {
             int fd, type;

             fd = open(DEVICE, O_RDWR | O_CLOEXEC);
             if (fd < 0) {
                     perror("Failed to open " DEVICE "\n");
                     return 1;
             }

             struct v4l2_requestbuffers reqbuf = {
                     .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
                     .memory = V4L2_MEMORY_MMAP,
                     .count = 1,
             };

             if (-1 == ioctl (fd, VIDIOC_REQBUFS, &reqbuf)) {
                     perror("VIDIOC_REQBUFS");
                     return 1;
             }

             type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
             if (ioctl (fd, VIDIOC_STREAMON, &type) != 0) {
                     perror("VIDIOC_STREAMON");
                     return 1;
             }

             printf("VIDIOC_STREAMON\n");

             usleep(100000);

             return 0;
     }

Thanks

Will

Re: [PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

Prod...

Is this acceptable to go in?

Thanks

Will

On 13/03/14 12:38, William Manley wrote:
> The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
> which allows the user to control the exposure time of the webcam,
> essentially controlling the brightness of the received image.  By default
> the webcam automatically adjusts the exposure time automatically but the
> if you set the control "Exposure, Auto"="Manual Mode" the user can fix
> the exposure time.
> 
> Unfortunately it seems that the Logitech C920 has a firmware bug where
> it will forget that it's in manual mode temporarily during initialisation.
> This means that the camera doesn't respect the exposure time that the user
> requested if they request it before starting to stream video.  They end up
> with a video stream which is either too bright or too dark and must reset
> the controls after video starts streaming.
> 
> This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
> causes the cached controls to be re-uploaded to the camera immediately
> after initialising the camera.  This quirk is applied to the C920 to work
> around this camera bug.
> 
> Changes since patch v1:
>  * Introduce quirk so workaround is only applied to the C920.
> 
> Signed-off-by: William Manley <will@williammanley.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  2 +-
>  drivers/media/usb/uvc/uvc_driver.c | 11 ++++++++++-
>  drivers/media/usb/uvc/uvc_video.c  |  6 ++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  3 ++-
>  4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index a2f4501..f72d7eb 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1795,7 +1795,7 @@ done:
>   * - Handle restore order (Auto-Exposure Mode should be restored before
>   *   Exposure Time).
>   */
> -int uvc_ctrl_resume_device(struct uvc_device *dev)
> +int uvc_ctrl_restore_values(struct uvc_device *dev)
>  {
>  	struct uvc_control *ctrl;
>  	struct uvc_entity *entity;
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index c3bb250..d3a9c3b 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
>  		int ret = 0;
>  
>  		if (reset) {
> -			ret = uvc_ctrl_resume_device(dev);
> +			ret = uvc_ctrl_restore_values(dev);
>  			if (ret < 0)
>  				return ret;
>  		}
> @@ -2156,6 +2156,15 @@ static struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0 },
> +	/* Logitech HD Pro Webcam C920 */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x046d,
> +	  .idProduct		= 0x082d,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_QUIRK_RESTORE_CTRLS_ON_INIT },
>  	/* Chicony CNF7129 (Asus EEE 100HE) */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 3394c34..85ff6b8 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1660,6 +1660,12 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
>  		}
>  	}
>  
> +	/* The Logitech C920 temporarily forgets that it should not be
> +	   adjusting Exposure Absolute during init so restore controls to
> +	   stored values. */
> +	if (stream->dev->quirks & UVC_QUIRK_RESTORE_CTRLS_ON_INIT)
> +		uvc_ctrl_restore_values(stream->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9e35982..0f54376 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -137,6 +137,7 @@
>  #define UVC_QUIRK_FIX_BANDWIDTH		0x00000080
>  #define UVC_QUIRK_PROBE_DEF		0x00000100
>  #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
> +#define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> @@ -676,7 +677,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  		const struct uvc_control_mapping *mapping);
>  extern int uvc_ctrl_init_device(struct uvc_device *dev);
>  extern void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> -extern int uvc_ctrl_resume_device(struct uvc_device *dev);
> +extern int uvc_ctrl_restore_values(struct uvc_device *dev);
>  
>  extern int uvc_ctrl_begin(struct uvc_video_chain *chain);
>  extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> 

Re: [PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

On 13/03/14 12:38, William Manley wrote:
> The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
> which allows the user to control the exposure time of the webcam,
> essentially controlling the brightness of the received image.  By default
> the webcam automatically adjusts the exposure time automatically but the
> if you set the control "Exposure, Auto"="Manual Mode" the user can fix
> the exposure time.
> 
> Unfortunately it seems that the Logitech C920 has a firmware bug where
> it will forget that it's in manual mode temporarily during initialisation.
> This means that the camera doesn't respect the exposure time that the user
> requested if they request it before starting to stream video.  They end up
> with a video stream which is either too bright or too dark and must reset
> the controls after video starts streaming.
> 
> This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
> causes the cached controls to be re-uploaded to the camera immediately
> after initialising the camera.  This quirk is applied to the C920 to work
> around this camera bug.
> 
> Changes since patch v1:
>  * Introduce quirk so workaround is only applied to the C920.
> 
> Signed-off-by: William Manley <will@williammanley.net>

Bump?

Re: [PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

On 25/03/14 23:03, Laurent Pinchart wrote:
> Hi William,
> 
> On Tuesday 25 March 2014 22:56:33 William Manley wrote:
>> On 13/03/14 12:38, William Manley wrote:
>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
>>> which allows the user to control the exposure time of the webcam,
>>> essentially controlling the brightness of the received image.  By default
>>> the webcam automatically adjusts the exposure time automatically but the
>>> if you set the control "Exposure, Auto"="Manual Mode" the user can fix
>>> the exposure time.
>>>
>>> Unfortunately it seems that the Logitech C920 has a firmware bug where
>>> it will forget that it's in manual mode temporarily during initialisation.
>>> This means that the camera doesn't respect the exposure time that the user
>>> requested if they request it before starting to stream video.  They end up
>>> with a video stream which is either too bright or too dark and must reset
>>> the controls after video starts streaming.
>>>
>>> This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
>>> causes the cached controls to be re-uploaded to the camera immediately
>>> after initialising the camera.  This quirk is applied to the C920 to work
>>> around this camera bug.
>>>
>>> Changes since patch v1:
>>>  * Introduce quirk so workaround is only applied to the C920.
>>>
>>> Signed-off-by: William Manley <will@williammanley.net>
>>
>> Bump?
> 
> Sorry, I haven't had the time to handle your patch yet. I'll try to do so on 
> Thursday or Friday.

Thanks.  Apologies for the nagging, just making sure that it hasn't been
forgotten about :)

Thanks

Will

Re: [PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[Laurent Pinchart]

Hi William,

On Tuesday 25 March 2014 22:56:33 William Manley wrote:
> On 13/03/14 12:38, William Manley wrote:
> > The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
> > which allows the user to control the exposure time of the webcam,
> > essentially controlling the brightness of the received image.  By default
> > the webcam automatically adjusts the exposure time automatically but the
> > if you set the control "Exposure, Auto"="Manual Mode" the user can fix
> > the exposure time.
> > 
> > Unfortunately it seems that the Logitech C920 has a firmware bug where
> > it will forget that it's in manual mode temporarily during initialisation.
> > This means that the camera doesn't respect the exposure time that the user
> > requested if they request it before starting to stream video.  They end up
> > with a video stream which is either too bright or too dark and must reset
> > the controls after video starts streaming.
> > 
> > This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
> > causes the cached controls to be re-uploaded to the camera immediately
> > after initialising the camera.  This quirk is applied to the C920 to work
> > around this camera bug.
> > 
> > Changes since patch v1:
> >  * Introduce quirk so workaround is only applied to the C920.
> > 
> > Signed-off-by: William Manley <will@williammanley.net>
> 
> Bump?

Sorry, I haven't had the time to handle your patch yet. I'll try to do so on 
Thursday or Friday.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] uvcvideo: Work around buggy Logitech C920 firmware

[William Manley]

On 25/03/14 23:03, Laurent Pinchart wrote:
> Hi William,
> 
> On Tuesday 25 March 2014 22:56:33 William Manley wrote:
>> On 13/03/14 12:38, William Manley wrote:
>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)"
>>> which allows the user to control the exposure time of the webcam,
>>> essentially controlling the brightness of the received image.  By default
>>> the webcam automatically adjusts the exposure time automatically but the
>>> if you set the control "Exposure, Auto"="Manual Mode" the user can fix
>>> the exposure time.
>>>
>>> Unfortunately it seems that the Logitech C920 has a firmware bug where
>>> it will forget that it's in manual mode temporarily during initialisation.
>>> This means that the camera doesn't respect the exposure time that the user
>>> requested if they request it before starting to stream video.  They end up
>>> with a video stream which is either too bright or too dark and must reset
>>> the controls after video starts streaming.
>>>
>>> This patch introduces the quirk UVC_QUIRK_RESTORE_CTRLS_ON_INIT which
>>> causes the cached controls to be re-uploaded to the camera immediately
>>> after initialising the camera.  This quirk is applied to the C920 to work
>>> around this camera bug.
>>>
>>> Changes since patch v1:
>>>  * Introduce quirk so workaround is only applied to the C920.
>>>
>>> Signed-off-by: William Manley <will@williammanley.net>
>>
>> Bump?
> 
> Sorry, I haven't had the time to handle your patch yet. I'll try to do so on 
> Thursday or Friday.

Apologies for the nagging: What's the current status?

Thanks

Will

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Laurent Pinchart]

Hi William,

On Thursday 13 March 2014 22:08:36 William Manley wrote:
> On 13/03/14 17:03, Laurent Pinchart wrote:
> > On Thursday 13 March 2014 10:48:20 Will Manley wrote:
> >> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote:
> >>> On Wednesday 12 March 2014 18:08:31 William Manley wrote:
> >>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure
> >>>> (Absolute)" which allows the user to control the exposure time of the
> >>>> webcam, essentially controlling the brightness of the received image.
> >>>> By default the webcam automatically adjusts the exposure time
> >>>> automatically but the if you set the control "Exposure, Auto"="Manual
> >>>> Mode" the user can fix the exposure time.
> >>>> 
> >>>> Unfortunately it seems that the Logitech C920 has a firmware bug where
> >>>> it will forget that it's in manual mode temporarily during
> >>>> initialisation. This means that the camera doesn't respect the exposure
> >>>> time that the user requested if they request it before starting to
> >>>> stream video. They end up with a video stream which is either too
> >>>> bright or too dark and must reset the controls after video starts
> >>>> streaming.
> >>> 
> >>> I would like to get a better understanding of the problem first. As I
> >>> don't have a C920, could you please perform two tests for me ?
> >>> 
> >>> I would first like to know what the camera reports as its exposure time
> >>> after starting the stream. If you get the exposure time at that point
> >>> (by sending a GET_CUR request, bypassing the driver cache), do you get
> >>> the value you had previously set (which, from your explanation, would be
> >>> incorrect, as the exposure time has changed based on your findings), or
> >>> a different value ? Does the camera change the exposure priority control
> >>> autonomously as well, or just the exposure time ?
> >> 
> >> It's a bit of a strange behaviour. I'd already tried littering the code
> >> with (uncached) GET_CUR requests. It seems that the value changes
> >> sometime during the call to usb_set_interface in uvc_init_video.
> > 
> > I'll assume this means that the camera reports the updated exposure time
> > in response to the GET_CUR request.
> 
> That's right
> 
> > Does the value of other controls (such as the
> > exposure priority control for instance) change as well ?
> 
> No, I've not seen any of the other controls change.
> 
> >> Strangely enough though calling uvc_ctrl_restore_values immediately after
> >> uvc_init_video has no effect. It must be put after the usb_submit_urb
> >> loop to fix the problem.
> >> 
> >>> Then, I would like to know whether the camera sends a control update
> >>> event when you start the stream, or if it just changes the exposure time
> >>> without notifying the driver.
> >> 
> >> Wireshark tells me that it is sending a control update event, but it
> >> seems like uvcvideo ignores it. I had to add the flag
> >> UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure
> >> (Auto)" for the new value to be properly reported to userspace.
> > 
> > Could you send me the USB trace ?
> 
> I've uploaded 3 traces: one with no patch (kernel 3.12) one with my
> "PATCH v2" applied and one with no patch but caching of gets disabled.
> You can get them here:
> 
>     http://williammanley.net/C920-no-patch.pcapng
>     http://williammanley.net/C920-with-patch.pcapng
>     http://williammanley.net/C920-no-caching.pcapng
> 
> The process to generate them was:
> 
>     sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng &
> 
>     # Plug USB cable in here
> 
>     v4l2-ctl -d
> /dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1
> 
> -cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balan
> ce_temperature_auto=0
> 
>     ./streamon
> 
> The relevant output seems to be (here from C920-no-patch.pcapng):
> > 10446 23.095874000 21:14:16.313257000         host -> 32.0         USB 64
> > SET INTERFACE Request 10447 23.109379000 21:14:16.326762000         32.3
> > -> host         USBVIDEO 70 URB_INTERRUPT in 10448 23.109389000
> > 21:14:16.326772000         host -> 32.3         USB 64 URB_INTERRUPT in
> > 10449 23.189448000 21:14:16.406831000         32.3 -> host        
> > USBVIDEO 73 URB_INTERRUPT in 10450 23.189486000 21:14:16.406869000       
> >  host -> 32.3         USB 64 URB_INTERRUPT in 10451 23.243314000
> > 21:14:16.460697000         32.0 -> host         USB 64 SET INTERFACE
> > Response
> Taking a closer look at packet 10449:
> > Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on
> > interface 0> 
> >     Interface id: 0
> >     Encapsulation type: USB packets with Linux header and padding (115)
> >     Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT
> >     [Time shift for this packet: 0.000000000 seconds]
> >     Epoch Time: 1394745256.406831000 seconds
> >     [Time delta from previous captured frame: 0.080059000 seconds]
> >     [Time delta from previous displayed frame: 0.080059000 seconds]
> >     [Time since reference or first frame: 23.189448000 seconds]
> >     Frame Number: 10449
> >     Frame Length: 73 bytes (584 bits)
> >     Capture Length: 73 bytes (584 bits)
> >     [Frame is marked: False]
> >     [Frame is ignored: False]
> >     [Protocols in frame: usb:usbvideo]
> > 
> > USB URB
> > 
> >     URB id: 0xffff88081bf95240
> >     URB type: URB_COMPLETE ('C')
> >     URB transfer type: URB_INTERRUPT (0x01)
> >     Endpoint: 0x83, Direction: IN
> >     
> >         1... .... = Direction: IN (1)
> >         .000 0011 = Endpoint value: 3
> >     
> >     Device: 32
> >     URB bus id: 1
> >     Device setup request: not relevant ('-')
> >     Data: present (0)
> >     URB sec: 1394745256
> >     URB usec: 406831
> >     URB status: Success (0)
> >     URB length [bytes]: 9
> >     Data length [bytes]: 9
> >     [Request in: 10448]
> >     [Time from request: 0.080059000 seconds]
> >     [bInterfaceClass: Video (0x0e)]
> > 
> > .... 0001 = Status Type: VideoControl Interface (0x01)
> > Originator: 1
> > Event: Control Change (0x00)
> > Control Selector: Exposure Time (Absolute) (0x04)
> > Change Type: Value (0x00)
> > Current value: 333 (0x0000014d)
> 
> This is the notification of the change of the value.  As you can see it
> happens between "SET INTERFACE Request" and "SET INTERFACE Response".

Thank you for investigating this, and sorry for the late reply.

I still haven't heard back from Logitech on this issue. I've pinged them, they 
might be busy at the moment.

Given that the device notifies us that the control value changes, one possibly 
more clever fix would be to handle that even and set the old control value 
back when the auto control is disabled. However, that's probably an 
overengineered solution.

I've still been wondering whether the quirk shouldn't restore only the 
control(s) that are known to be erroneously changed by the camera instead of 
restoring them all. Feel free to disagree, what's your opinion about that ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Will Manley]

On Mon, 14 Apr 2014, at 1:34, Laurent Pinchart wrote:
> On Thursday 13 March 2014 22:08:36 William Manley wrote:
> > On 13/03/14 17:03, Laurent Pinchart wrote:
> > > On Thursday 13 March 2014 10:48:20 Will Manley wrote:
> > >> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote:
> > >>> On Wednesday 12 March 2014 18:08:31 William Manley wrote:
> > >>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure
> > >>>> (Absolute)" which allows the user to control the exposure time of the
> > >>>> webcam, essentially controlling the brightness of the received image.
> > >>>> By default the webcam automatically adjusts the exposure time
> > >>>> automatically but the if you set the control "Exposure, Auto"="Manual
> > >>>> Mode" the user can fix the exposure time.
> > >>>> 
> > >>>> Unfortunately it seems that the Logitech C920 has a firmware bug where
> > >>>> it will forget that it's in manual mode temporarily during
> > >>>> initialisation. This means that the camera doesn't respect the exposure
> > >>>> time that the user requested if they request it before starting to
> > >>>> stream video. They end up with a video stream which is either too
> > >>>> bright or too dark and must reset the controls after video starts
> > >>>> streaming.
> > >>> 
> > >>> I would like to get a better understanding of the problem first. As I
> > >>> don't have a C920, could you please perform two tests for me ?
> > >>> 
> > >>> I would first like to know what the camera reports as its exposure time
> > >>> after starting the stream. If you get the exposure time at that point
> > >>> (by sending a GET_CUR request, bypassing the driver cache), do you get
> > >>> the value you had previously set (which, from your explanation, would be
> > >>> incorrect, as the exposure time has changed based on your findings), or
> > >>> a different value ? Does the camera change the exposure priority control
> > >>> autonomously as well, or just the exposure time ?
> > >> 
> > >> It's a bit of a strange behaviour. I'd already tried littering the code
> > >> with (uncached) GET_CUR requests. It seems that the value changes
> > >> sometime during the call to usb_set_interface in uvc_init_video.
> > > 
> > > I'll assume this means that the camera reports the updated exposure time
> > > in response to the GET_CUR request.
> > 
> > That's right
> > 
> > > Does the value of other controls (such as the
> > > exposure priority control for instance) change as well ?
> > 
> > No, I've not seen any of the other controls change.
> > 
> > >> Strangely enough though calling uvc_ctrl_restore_values immediately after
> > >> uvc_init_video has no effect. It must be put after the usb_submit_urb
> > >> loop to fix the problem.
> > >> 
> > >>> Then, I would like to know whether the camera sends a control update
> > >>> event when you start the stream, or if it just changes the exposure time
> > >>> without notifying the driver.
> > >> 
> > >> Wireshark tells me that it is sending a control update event, but it
> > >> seems like uvcvideo ignores it. I had to add the flag
> > >> UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure
> > >> (Auto)" for the new value to be properly reported to userspace.
> > > 
> > > Could you send me the USB trace ?
> > 
> > I've uploaded 3 traces: one with no patch (kernel 3.12) one with my
> > "PATCH v2" applied and one with no patch but caching of gets disabled.
> > You can get them here:
> > 
> >     http://williammanley.net/C920-no-patch.pcapng
> >     http://williammanley.net/C920-with-patch.pcapng
> >     http://williammanley.net/C920-no-caching.pcapng
> > 
> > The process to generate them was:
> > 
> >     sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng &
> > 
> >     # Plug USB cable in here
> > 
> >     v4l2-ctl -d
> > /dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1
> > 
> > -cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balan
> > ce_temperature_auto=0
> > 
> >     ./streamon
> > 
> > The relevant output seems to be (here from C920-no-patch.pcapng):
> > > 10446 23.095874000 21:14:16.313257000         host -> 32.0         USB 64
> > > SET INTERFACE Request 10447 23.109379000 21:14:16.326762000         32.3
> > > -> host         USBVIDEO 70 URB_INTERRUPT in 10448 23.109389000
> > > 21:14:16.326772000         host -> 32.3         USB 64 URB_INTERRUPT in
> > > 10449 23.189448000 21:14:16.406831000         32.3 -> host        
> > > USBVIDEO 73 URB_INTERRUPT in 10450 23.189486000 21:14:16.406869000       
> > >  host -> 32.3         USB 64 URB_INTERRUPT in 10451 23.243314000
> > > 21:14:16.460697000         32.0 -> host         USB 64 SET INTERFACE
> > > Response
> > Taking a closer look at packet 10449:
> > > Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on
> > > interface 0> 
> > >     Interface id: 0
> > >     Encapsulation type: USB packets with Linux header and padding (115)
> > >     Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT
> > >     [Time shift for this packet: 0.000000000 seconds]
> > >     Epoch Time: 1394745256.406831000 seconds
> > >     [Time delta from previous captured frame: 0.080059000 seconds]
> > >     [Time delta from previous displayed frame: 0.080059000 seconds]
> > >     [Time since reference or first frame: 23.189448000 seconds]
> > >     Frame Number: 10449
> > >     Frame Length: 73 bytes (584 bits)
> > >     Capture Length: 73 bytes (584 bits)
> > >     [Frame is marked: False]
> > >     [Frame is ignored: False]
> > >     [Protocols in frame: usb:usbvideo]
> > > 
> > > USB URB
> > > 
> > >     URB id: 0xffff88081bf95240
> > >     URB type: URB_COMPLETE ('C')
> > >     URB transfer type: URB_INTERRUPT (0x01)
> > >     Endpoint: 0x83, Direction: IN
> > >     
> > >         1... .... = Direction: IN (1)
> > >         .000 0011 = Endpoint value: 3
> > >     
> > >     Device: 32
> > >     URB bus id: 1
> > >     Device setup request: not relevant ('-')
> > >     Data: present (0)
> > >     URB sec: 1394745256
> > >     URB usec: 406831
> > >     URB status: Success (0)
> > >     URB length [bytes]: 9
> > >     Data length [bytes]: 9
> > >     [Request in: 10448]
> > >     [Time from request: 0.080059000 seconds]
> > >     [bInterfaceClass: Video (0x0e)]
> > > 
> > > .... 0001 = Status Type: VideoControl Interface (0x01)
> > > Originator: 1
> > > Event: Control Change (0x00)
> > > Control Selector: Exposure Time (Absolute) (0x04)
> > > Change Type: Value (0x00)
> > > Current value: 333 (0x0000014d)
> > 
> > This is the notification of the change of the value.  As you can see it
> > happens between "SET INTERFACE Request" and "SET INTERFACE Response".
> 
> Thank you for investigating this, and sorry for the late reply.
> 
> I still haven't heard back from Logitech on this issue. I've pinged them,
> they 
> might be busy at the moment.

Thanks for looking at my patch :).

> Given that the device notifies us that the control value changes, one
> possibly 
> more clever fix would be to handle that even and set the old control
> value 
> back when the auto control is disabled. However, that's probably an 
> overengineered solution.
> 
> I've still been wondering whether the quirk shouldn't restore only the 
> control(s) that are known to be erroneously changed by the camera instead
> of 
> restoring them all. Feel free to disagree, what's your opinion about that
> ?

So that was my initial intention, but when I got into it it seemed like
it was going to add a whole bunch of additional complexity (and lines of
code) for questionable benefit.  While uploading all the values is a bit
of a sledgehammer it has the advantage that it's simple and dumb and
exercises code that's already in use for suspend/resume.  OTOH you could
argue that a patch which explicitly contains code like:

    if (strcmp(param.name, "Exposure (Absolute)") == 0) {
        blah, blah, blah
    }

or similar documents the quirk a little more explicitly.  I still didn't
think it was worth the extra complexity.  I'm quite willing to be
convinced otherwise though :).

Another more marginal advantage is that the quirk may be more applicable
to other hardware.  Of course this is entirely theoretical at this point
so probably can be discounted.

Thanks

Will

Re: [PATCH] uvcvideo: Work around buggy Logitech C920 firmware

[Laurent Pinchart]

Hi Will,

On Monday 14 April 2014 11:27:00 Will Manley wrote:
> On Mon, 14 Apr 2014, at 1:34, Laurent Pinchart wrote:

[snip]

> > Thank you for investigating this, and sorry for the late reply.
> > 
> > I still haven't heard back from Logitech on this issue. I've pinged them,
> > they might be busy at the moment.
> 
> Thanks for looking at my patch :).

You're welcome.

> > Given that the device notifies us that the control value changes, one
> > possibly more clever fix would be to handle that even and set the old
> > control value back when the auto control is disabled. However, that's
> > probably an overengineered solution.
> > 
> > I've still been wondering whether the quirk shouldn't restore only the
> > control(s) that are known to be erroneously changed by the camera instead
> > of restoring them all. Feel free to disagree, what's your opinion about
> > that ?
> 
> So that was my initial intention, but when I got into it it seemed like
> it was going to add a whole bunch of additional complexity (and lines of
> code) for questionable benefit.  While uploading all the values is a bit
> of a sledgehammer it has the advantage that it's simple and dumb and
> exercises code that's already in use for suspend/resume.  OTOH you could
> argue that a patch which explicitly contains code like:
> 
>     if (strcmp(param.name, "Exposure (Absolute)") == 0) {
>         blah, blah, blah
>     }
> 
> or similar documents the quirk a little more explicitly.  I still didn't
> think it was worth the extra complexity.  I'm quite willing to be
> convinced otherwise though :).
> 
> Another more marginal advantage is that the quirk may be more applicable
> to other hardware.  Of course this is entirely theoretical at this point
> so probably can be discounted.

One of the things that bother me with restoring all controls right after 
starting the stream is that it might actually result in different unexpected 
behaviors. For instance, this would write the value of all manual controls 
even when the corresponding auto control is turned on. Some cameras might not 
be happy, and this could have an adverse effect from temporary glitches in the 
video to complete crashes. Of course the quirk should not be enabled for 
cameras that would then crash, so we could consider that it's good as-is for 
the C920.

I'd like to hear from Logitech on this issue before taking a decision. I've 
pinged them, let's wait one more week if that's fine with you.

In the meantime, there's another question that crossed my mind, have you 
checked whether the camera has a similar buggy behaviour for other auto 
controls (auto white balance for instance), or if it's limited to auto-
exposure ?

-- 
Regards,

Laurent Pinchart