[PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

>From 989d894a2af7ceadf2574f455d9e68779f4ae674 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@canonical.com>
Date: Mon, 11 Jul 2011 17:04:31 +0800
Subject: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

We found this type(0c45:6437) of Microdia camera does not
work(no stream packets sent out from camera any longer) after
resume from sleep, but unbind/bind driver will work again.

So introduce the quirk of UVC_QUIRK_FIX_SUSPEND_RESUME to
fix the problem for this type of Microdia camera.
---
 drivers/media/video/uvc/uvc_driver.c |  146 +++++++++++++++++++---------------
 drivers/media/video/uvc/uvcvideo.h   |    1 +
 2 files changed, 84 insertions(+), 63 deletions(-)

diff --git a/drivers/media/video/uvc/uvc_driver.c b/drivers/media/video/uvc/uvc_driver.c
index b6eae48..2b356c3 100644
--- a/drivers/media/video/uvc/uvc_driver.c
+++ b/drivers/media/video/uvc/uvc_driver.c
@@ -1787,6 +1787,68 @@ static int uvc_register_chains(struct uvc_device *dev)
 /* ------------------------------------------------------------------------
  * USB probe, disconnect, suspend and resume
  */
+static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct uvc_device *dev = usb_get_intfdata(intf);
+	struct uvc_streaming *stream;
+
+	uvc_trace(UVC_TRACE_SUSPEND, "Suspending interface %u\n",
+		intf->cur_altsetting->desc.bInterfaceNumber);
+
+	/* Controls are cached on the fly so they don't need to be saved. */
+	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
+	    UVC_SC_VIDEOCONTROL)
+		return uvc_status_suspend(dev);
+
+	list_for_each_entry(stream, &dev->streams, list) {
+		if (stream->intf == intf)
+			return uvc_video_suspend(stream);
+	}
+
+	uvc_trace(UVC_TRACE_SUSPEND, "Suspend: video streaming USB interface "
+			"mismatch.\n");
+	return -EINVAL;
+}
+
+static int __uvc_resume(struct usb_interface *intf, int reset)
+{
+	struct uvc_device *dev = usb_get_intfdata(intf);
+	struct uvc_streaming *stream;
+
+	uvc_trace(UVC_TRACE_SUSPEND, "Resuming interface %u\n",
+		intf->cur_altsetting->desc.bInterfaceNumber);
+
+	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
+	    UVC_SC_VIDEOCONTROL) {
+		if (reset) {
+			int ret = uvc_ctrl_resume_device(dev);
+
+			if (ret < 0)
+				return ret;
+		}
+
+		return uvc_status_resume(dev);
+	}
+
+	list_for_each_entry(stream, &dev->streams, list) {
+		if (stream->intf == intf)
+			return uvc_video_resume(stream);
+	}
+
+	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "
+			"mismatch.\n");
+	return -EINVAL;
+}
+
+static int uvc_resume(struct usb_interface *intf)
+{
+	return __uvc_resume(intf, 0);
+}
+
+static int uvc_reset_resume(struct usb_interface *intf)
+{
+	return __uvc_resume(intf, 1);
+}
 
 static int uvc_probe(struct usb_interface *intf,
 		     const struct usb_device_id *id)
@@ -1888,6 +1950,18 @@ static int uvc_probe(struct usb_interface *intf,
 			"supported.\n", ret);
 	}
 
+	/* For some buggy cameras, they will not work after wakeup, so
+	 * do unbind in .usb_suspend and do rebind in .usb_resume to
+	 * make it work again.
+	 * */
+	if (dev->quirks & UVC_QUIRK_FIX_SUSPEND_RESUME) {
+		uvc_driver.driver.suspend = NULL;
+		uvc_driver.driver.resume = NULL;
+	} else {
+		uvc_driver.driver.suspend = uvc_suspend;
+		uvc_driver.driver.resume = uvc_resume;
+	}
+
 	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
 	usb_enable_autosuspend(udev);
 	return 0;
@@ -1915,69 +1989,6 @@ static void uvc_disconnect(struct usb_interface *intf)
 	uvc_unregister_video(dev);
 }
 
-static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
-{
-	struct uvc_device *dev = usb_get_intfdata(intf);
-	struct uvc_streaming *stream;
-
-	uvc_trace(UVC_TRACE_SUSPEND, "Suspending interface %u\n",
-		intf->cur_altsetting->desc.bInterfaceNumber);
-
-	/* Controls are cached on the fly so they don't need to be saved. */
-	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
-	    UVC_SC_VIDEOCONTROL)
-		return uvc_status_suspend(dev);
-
-	list_for_each_entry(stream, &dev->streams, list) {
-		if (stream->intf == intf)
-			return uvc_video_suspend(stream);
-	}
-
-	uvc_trace(UVC_TRACE_SUSPEND, "Suspend: video streaming USB interface "
-			"mismatch.\n");
-	return -EINVAL;
-}
-
-static int __uvc_resume(struct usb_interface *intf, int reset)
-{
-	struct uvc_device *dev = usb_get_intfdata(intf);
-	struct uvc_streaming *stream;
-
-	uvc_trace(UVC_TRACE_SUSPEND, "Resuming interface %u\n",
-		intf->cur_altsetting->desc.bInterfaceNumber);
-
-	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
-	    UVC_SC_VIDEOCONTROL) {
-		if (reset) {
-			int ret = uvc_ctrl_resume_device(dev);
-
-			if (ret < 0)
-				return ret;
-		}
-
-		return uvc_status_resume(dev);
-	}
-
-	list_for_each_entry(stream, &dev->streams, list) {
-		if (stream->intf == intf)
-			return uvc_video_resume(stream);
-	}
-
-	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "
-			"mismatch.\n");
-	return -EINVAL;
-}
-
-static int uvc_resume(struct usb_interface *intf)
-{
-	return __uvc_resume(intf, 0);
-}
-
-static int uvc_reset_resume(struct usb_interface *intf)
-{
-	return __uvc_resume(intf, 1);
-}
-
 /* ------------------------------------------------------------------------
  * Module parameters
  */
@@ -2175,6 +2186,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_QUIRK_FIX_BANDWIDTH },
+	/* Microdia camera */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x0c45,
+	  .idProduct		= 0x6437,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FIX_SUSPEND_RESUME },
 	/* MT6227 */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/video/uvc/uvcvideo.h b/drivers/media/video/uvc/uvcvideo.h
index 20107fd..3c13b04 100644
--- a/drivers/media/video/uvc/uvcvideo.h
+++ b/drivers/media/video/uvc/uvcvideo.h
@@ -211,6 +211,7 @@ struct uvc_xu_control {
 #define UVC_QUIRK_FIX_BANDWIDTH		0x00000080
 #define UVC_QUIRK_PROBE_DEF		0x00000100
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
+#define UVC_QUIRK_FIX_SUSPEND_RESUME	0x00000400
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
1.7.4.1



-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Laurent Pinchart]

Hi,

On Monday 11 July 2011 11:48:11 Ming Lei wrote:
> From 989d894a2af7ceadf2574f455d9e68779f4ae674 Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@canonical.com>
> Date: Mon, 11 Jul 2011 17:04:31 +0800
> Subject: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
> 
> We found this type(0c45:6437) of Microdia camera does not
> work(no stream packets sent out from camera any longer) after
> resume from sleep, but unbind/bind driver will work again.
> 
> So introduce the quirk of UVC_QUIRK_FIX_SUSPEND_RESUME to
> fix the problem for this type of Microdia camera.

Thank you for the patch.

[snip]

> +	/* For some buggy cameras, they will not work after wakeup, so
> +	 * do unbind in .usb_suspend and do rebind in .usb_resume to
> +	 * make it work again.
> +	 * */
> +	if (dev->quirks & UVC_QUIRK_FIX_SUSPEND_RESUME) {
> +		uvc_driver.driver.suspend = NULL;
> +		uvc_driver.driver.resume = NULL;
> +	} else {
> +		uvc_driver.driver.suspend = uvc_suspend;
> +		uvc_driver.driver.resume = uvc_resume;
> +	}
> +

That's unfortunately not acceptable as-is. If two cameras are connected to the 
system, and only one of them doesn't support suspend/resume, the other will be 
affected by your patch.

Have you tried to investigate why suspend/resume fails for the above-mentioned 
camera, instead of working around the problem ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Sergei Shtylyov]

Hello.

On 11-07-2011 13:48, Ming Lei wrote:

>  From 989d894a2af7ceadf2574f455d9e68779f4ae674 Mon Sep 17 00:00:00 2001
> From: Ming Lei<ming.lei@canonical.com>
> Date: Mon, 11 Jul 2011 17:04:31 +0800
> Subject: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

    Please omit this header when sending patches.

> We found this type(0c45:6437) of Microdia camera does not
> work(no stream packets sent out from camera any longer) after
> resume from sleep, but unbind/bind driver will work again.

> So introduce the quirk of UVC_QUIRK_FIX_SUSPEND_RESUME to
> fix the problem for this type of Microdia camera.

    You didn't sign off.

> ---
>   drivers/media/video/uvc/uvc_driver.c |  146 +++++++++++++++++++---------------
>   drivers/media/video/uvc/uvcvideo.h   |    1 +
>   2 files changed, 84 insertions(+), 63 deletions(-)

> diff --git a/drivers/media/video/uvc/uvc_driver.c b/drivers/media/video/uvc/uvc_driver.c
> index b6eae48..2b356c3 100644
> --- a/drivers/media/video/uvc/uvc_driver.c
> +++ b/drivers/media/video/uvc/uvc_driver.c
> @@ -1787,6 +1787,68 @@ static int uvc_register_chains(struct uvc_device *dev)
[...]
> @@ -1888,6 +1950,18 @@ static int uvc_probe(struct usb_interface *intf,
>   			"supported.\n", ret);
>   	}
>
> +	/* For some buggy cameras, they will not work after wakeup, so
> +	 * do unbind in .usb_suspend and do rebind in .usb_resume to
> +	 * make it work again.
> +	 * */

    The preferred style is:

/*
  * bla
  * bla
  */

WBR, Sergei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

Thanks for your reply.

On Mon, Jul 11, 2011 at 6:44 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> That's unfortunately not acceptable as-is. If two cameras are connected to the
> system, and only one of them doesn't support suspend/resume, the other will be
> affected by your patch.

Yes, other cameras may be affected, but they still can work well, so
the patch still
makes sense.

>
> Have you tried to investigate why suspend/resume fails for the above-mentioned
> camera, instead of working around the problem ?

I have investigated the problem, and not found failures in the
suspend/resume path,
either .suspend or .resume callback return successful, but no stream packets are
received from camera any longer after resume from sleep. Once doing a unbind&
bind will make the camera work again.

Could you give any suggestions to help to find the root cause of the problem?



thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

Hi Laurent,

After resume from sleep,  all the ISO packets from camera are like below:

ffff880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600
-18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 <
ffff880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12
0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c0000 0000fa7e
012f1b05 00000000 00000000 00000000 00000000 00000000

All are headed with 0c8c0000, see attached usbmon captures.

thanks,
-- 
Ming Lei

[-- Attachment #2: usb.tar.gz --]
[-- Type: application/x-gzip, Size: 41548 bytes --]

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

On Tue, 12 Jul 2011, Ming Lei wrote:

> Hi Laurent,
> 
> After resume from sleep,  all the ISO packets from camera are like below:
> 
> ffff880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600
> -18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 <
> ffff880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12
> 0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c0000 0000fa7e
> 012f1b05 00000000 00000000 00000000 00000000 00000000
> 
> All are headed with 0c8c0000, see attached usbmon captures.

Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c.

Alan Stern

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Tue, Jul 12, 2011 at 11:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 12 Jul 2011, Ming Lei wrote:
>
>> Hi Laurent,
>>
>> After resume from sleep,  all the ISO packets from camera are like below:
>>
>> ffff880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600
>> -18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 <
>> ffff880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12
>> 0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c0000 0000fa7e
>> 012f1b05 00000000 00000000 00000000 00000000 00000000
>>
>> All are headed with 0c8c0000, see attached usbmon captures.
>
> Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c.

I will try it, but seems unbind&bind driver don't produce extra usb reset signal
to the device.

Also, the problem didn't happen in runtime pm case, just happen in
wakeup from system suspend case. uvcvideo has enabled auto suspend
already at default.

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Tue, Jul 12, 2011 at 11:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

> Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c.

RESET_RESUME quirk makes things worse, now stream data is not received from
the camera at all even in resume from runtime suspend case. So the quirk can
make the device totally useless, :-)

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Laurent Pinchart]

On Tuesday 12 July 2011 03:21:05 Ming Lei wrote:
> On Mon, Jul 11, 2011 at 6:44 PM, Laurent Pinchart wrote:
> > That's unfortunately not acceptable as-is. If two cameras are connected
> > to the system, and only one of them doesn't support suspend/resume, the
> > other will be affected by your patch.
> 
> Yes, other cameras may be affected, but they still can work well, so
> the patch still makes sense.

They can still work, but not optimally, as they will be reset instead of 
suspended/resumed. That's not acceptable.

> > Have you tried to investigate why suspend/resume fails for the
> > above-mentioned camera, instead of working around the problem ?
> 
> I have investigated the problem, and not found failures in the
> suspend/resume path,
> either .suspend or .resume callback return successful, but no stream
> packets are received from camera any longer after resume from sleep. Once
> doing a unbind& bind will make the camera work again.
> 
> Could you give any suggestions to help to find the root cause of the
> problem?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Wed, Jul 13, 2011 at 4:38 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> They can still work, but not optimally, as they will be reset instead of
> suspended/resumed. That's not acceptable.

If the "reset" you mentioned is usb bus reset signal, I think unbind&bind
will not produce the reset signal.

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Oliver Neukum]

Am Mittwoch, 13. Juli 2011, 10:51:11 schrieb Ming Lei:
> Hi,
> 
> On Wed, Jul 13, 2011 at 4:38 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
> > They can still work, but not optimally, as they will be reset instead of
> > suspended/resumed. That's not acceptable.
> 
> If the "reset" you mentioned is usb bus reset signal, I think unbind&bind
> will not produce the reset signal.

You are correct. It will not.

	Regards
		Oliver
-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Laurent Pinchart]

On Wednesday 13 July 2011 10:51:11 Ming Lei wrote:
> On Wed, Jul 13, 2011 at 4:38 PM, Laurent Pinchart wrote:
> > They can still work, but not optimally, as they will be reset instead of
> > suspended/resumed. That's not acceptable.
> 
> If the "reset" you mentioned is usb bus reset signal, I think unbind&bind
> will not produce the reset signal.

Sorry, I haven't been clear. If you remove the suspend/resume handlers from 
the driver, the USB core will unbind and rebind the driver instead of 
suspending/resuming the device properly. As this will affect other UVC devices 
as well, that's not a good solution.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Wed, Jul 13, 2011 at 4:59 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> Sorry, I haven't been clear. If you remove the suspend/resume handlers from
> the driver, the USB core will unbind and rebind the driver instead of
> suspending/resuming the device properly. As this will affect other UVC devices
> as well, that's not a good solution.

I agree with you this is not a good solution, but seems there are no
other solutions
for the buggy device.

You are uvc expert, could you give some suggestion about accepted solutions?

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1168 bytes --]

On Wed, 13 Jul 2011, Ming Lei wrote:

> Hi,
> 
> On Tue, Jul 12, 2011 at 11:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 12 Jul 2011, Ming Lei wrote:
> >
> >> Hi Laurent,
> >>
> >> After resume from sleep,  all the ISO packets from camera are like below:
> >>
> >> ffff880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600
> >> -18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 <
> >> ffff880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12
> >> 0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c0000 0000fa7e
> >> 012f1b05 00000000 00000000 00000000 00000000 00000000
> >>
> >> All are headed with 0c8c0000, see attached usbmon captures.
> >
> > Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c.
> 
> I will try it, but seems unbind&bind driver don't produce extra usb reset signal
> to the device.
> 
> Also, the problem didn't happen in runtime pm case, just happen in
> wakeup from system suspend case. uvcvideo has enabled auto suspend
> already at default.

Why should system suspend be different from runtime suspend?  Have you
compared usbmon traces for the two types of suspend?

Alan Stern

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Wed, Jul 13, 2011 at 11:20 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

> Why should system suspend be different from runtime suspend?  Have you

This is also my puzzle, :-)

> compared usbmon traces for the two types of suspend?

Almost same. If I add USB_QUIRK_RESET_RESUME quirk for the device,
the stream data will not be received from the device in runtime pm case,
same with that in system suspend.

Maybe buggy BIOS makes root hub send reset signal to the device during
system suspend time, not sure...

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 938 bytes --]

On Wed, 13 Jul 2011, Ming Lei wrote:

> Hi,
> 
> On Wed, Jul 13, 2011 at 11:20 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > Why should system suspend be different from runtime suspend?  Have you
> 
> This is also my puzzle, :-)
> 
> > compared usbmon traces for the two types of suspend?
> 
> Almost same.

Come on.  "Almost same" means they are different.  That difference is
clearly the important thing you need to track down.

>  If I add USB_QUIRK_RESET_RESUME quirk for the device,
> the stream data will not be received from the device in runtime pm case,
> same with that in system suspend.

uvcvideo should be able to reinitialize the device so that it works
correctly following a reset.  If the device doesn't work then uvcvideo
has a bug in its reset_resume handler.

> Maybe buggy BIOS makes root hub send reset signal to the device during
> system suspend time, not sure...

That's entirely possible.

Alan Stern

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Wed, Jul 13, 2011 at 11:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 13 Jul 2011, Ming Lei wrote:

>> Almost same.
>
> Come on.  "Almost same" means they are different.  That difference is
> clearly the important thing you need to track down.

I didn't say "entirely same" because we can't trace the packets via usbmon
during system resume, but we can do it during runtime resume.

In fact, except for above, the packets captured from interrupt ep and control ep
are completely same.  Also all functions in uvc (rpm, system)resume path return
successfully.

>
>>  If I add USB_QUIRK_RESET_RESUME quirk for the device,
>> the stream data will not be received from the device in runtime pm case,
>> same with that in system suspend.
>
> uvcvideo should be able to reinitialize the device so that it works
> correctly following a reset.  If the device doesn't work then uvcvideo
> has a bug in its reset_resume handler.

This also indicates the usb reset during resume does make the uvc device
broken.

>> Maybe buggy BIOS makes root hub send reset signal to the device during
>> system suspend time, not sure...
>
> That's entirely possible.

The below quirk  fixes the issue now.

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 81ce6a8..93c6fa2 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -82,6 +82,9 @@ static const struct usb_device_id usb_quirk_list[] = {
 	/* Broadcom BCM92035DGROM BT dongle */
 	{ USB_DEVICE(0x0a5c, 0x2021), .driver_info = USB_QUIRK_RESET_RESUME },

+	/* Microdia uvc camera */
+	{ USB_DEVICE(0x0c45, 0x6437), .driver_info = USB_QUIRK_RESET_MORPHS },
+
 	/* Action Semiconductor flash disk */
 	{ USB_DEVICE(0x10d6, 0x2200), .driver_info =
 			USB_QUIRK_STRING_FETCH_255 },


thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2446 bytes --]

On Thu, 14 Jul 2011, Ming Lei wrote:

> Hi,
> 
> On Wed, Jul 13, 2011 at 11:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 13 Jul 2011, Ming Lei wrote:
> 
> >> Almost same.
> >
> > Come on.  "Almost same" means they are different.  That difference is
> > clearly the important thing you need to track down.
> 
> I didn't say "entirely same" because we can't trace the packets via usbmon
> during system resume, but we can do it during runtime resume.
> 
> In fact, except for above, the packets captured from interrupt ep and control ep
> are completely same.  Also all functions in uvc (rpm, system)resume path return
> successfully.

All right; this tends to confirm your guess that the BIOS messes up the 
device by resetting it during system resume.

> >>  If I add USB_QUIRK_RESET_RESUME quirk for the device,
> >> the stream data will not be received from the device in runtime pm case,
> >> same with that in system suspend.
> >
> > uvcvideo should be able to reinitialize the device so that it works
> > correctly following a reset.  If the device doesn't work then uvcvideo
> > has a bug in its reset_resume handler.
> 
> This also indicates the usb reset during resume does make the uvc device
> broken.

Resetting the device doesn't actually _break_ it -- if it did then the 
device would _never_ work because the first thing that usbcore does to 
initialize a new device is reset it!

More likely, the reset erases some device setting that uvcvideo 
installed while binding.  Evidently uvcvideo does not re-install the 
setting during reset-resume; this is probably a bug in the driver.

> The below quirk  fixes the issue now.
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 81ce6a8..93c6fa2 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -82,6 +82,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>  	/* Broadcom BCM92035DGROM BT dongle */
>  	{ USB_DEVICE(0x0a5c, 0x2021), .driver_info = USB_QUIRK_RESET_RESUME },
> 
> +	/* Microdia uvc camera */
> +	{ USB_DEVICE(0x0c45, 0x6437), .driver_info = USB_QUIRK_RESET_MORPHS },
> +
>  	/* Action Semiconductor flash disk */
>  	{ USB_DEVICE(0x10d6, 0x2200), .driver_info =
>  			USB_QUIRK_STRING_FETCH_255 },

It would be better to fix uvcvideo, if you could figure out what it 
needs to do differently.  This quirk is only a workaround, because the 
device doesn't really morph.

Alan Stern

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Thu, Jul 14, 2011 at 11:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

> All right; this tends to confirm your guess that the BIOS messes up the
> device by resetting it during system resume.

Yes.  BIOS messes the device first, then usbcore has to reset the device
at the end of resume, so the device behaves badly: ISO transfer oddly

>> This also indicates the usb reset during resume does make the uvc device
>> broken.
>
> Resetting the device doesn't actually _break_ it -- if it did then the
> device would _never_ work because the first thing that usbcore does to
> initialize a new device is reset it!

I means the reset in resume breaks the device, not the reset in enumeration, :-)
(the only extra reset in rpm resume will make the device not work)

>
> More likely, the reset erases some device setting that uvcvideo
> installed while binding.  Evidently uvcvideo does not re-install the
> setting during reset-resume; this is probably a bug in the driver.

Yes, maybe some settings inside device have changed after the
reset signal, I don't know if it is a normal behaviour.

I have tried to add some code in .probe path to .resume path,
but still not make it work. Anyway, it is not easy thing, because we
have not the internal knowledge of uvc device implementation, and
have to try it by guess.

>> The below quirk  fixes the issue now.
>>
>> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
>> index 81ce6a8..93c6fa2 100644
>> --- a/drivers/usb/core/quirks.c
>> +++ b/drivers/usb/core/quirks.c
>> @@ -82,6 +82,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>>       /* Broadcom BCM92035DGROM BT dongle */
>>       { USB_DEVICE(0x0a5c, 0x2021), .driver_info = USB_QUIRK_RESET_RESUME },
>>
>> +     /* Microdia uvc camera */
>> +     { USB_DEVICE(0x0c45, 0x6437), .driver_info = USB_QUIRK_RESET_MORPHS },
>> +
>>       /* Action Semiconductor flash disk */
>>       { USB_DEVICE(0x10d6, 0x2200), .driver_info =
>>                       USB_QUIRK_STRING_FETCH_255 },
>
> It would be better to fix uvcvideo, if you could figure out what it
> needs to do differently.  This quirk is only a workaround, because the
> device doesn't really morph.

In fact we can understand the quirk is used to avoid reset in system resume,
which is one of its original purpose too.

I will do some tests to figure out solution in uvc driver, but I am
not sure I can
find it quickly because I debug it remotely and network is very
slowly. If I can't
find out the solution in uvc driver, could you accept the workaround of
USB_QUIRK_RESET_MORPHS first?

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Thu, Jul 14, 2011 at 11:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

> More likely, the reset erases some device setting that uvcvideo
> installed while binding.  Evidently uvcvideo does not re-install the
> setting during reset-resume; this is probably a bug in the driver.

Alan, you are right.

I think I have found the root cause. Given many devices can't
handle set_interface(0) if the interfaces were already in altsetting 0,
usb_reset_and_verify_device does not run set_interface(0). So we
need to do it in .reset_resume handler of uvc driver and it is always
safe for uvc devices.

I have tested the below patch, and it can make the uvc device work
well after rpm resume and system resume(reset resume), both in
streaming on and off case.

Alan, Laurent, if you have no objections, I will submit a formal one.

diff --git a/drivers/media/video/uvc/uvc_driver.c
b/drivers/media/video/uvc/uvc_driver.c
index b6eae48..4055dfc 100644
--- a/drivers/media/video/uvc/uvc_driver.c
+++ b/drivers/media/video/uvc/uvc_driver.c
@@ -1959,8 +1959,12 @@ static int __uvc_resume(struct usb_interface
*intf, int reset)
 	}

 	list_for_each_entry(stream, &dev->streams, list) {
-		if (stream->intf == intf)
+		if (stream->intf == intf) {
+			if (reset)
+				usb_set_interface(stream->dev->udev,
+					stream->intfnum, 0);
 			return uvc_video_resume(stream);
+		}
 	}

 	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "



thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1766 bytes --]

On Fri, 15 Jul 2011, Ming Lei wrote:

> Hi,
> 
> On Thu, Jul 14, 2011 at 11:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > More likely, the reset erases some device setting that uvcvideo
> > installed while binding.  Evidently uvcvideo does not re-install the
> > setting during reset-resume; this is probably a bug in the driver.
> 
> Alan, you are right.
> 
> I think I have found the root cause. Given many devices can't
> handle set_interface(0) if the interfaces were already in altsetting 0,
> usb_reset_and_verify_device does not run set_interface(0). So we
> need to do it in .reset_resume handler of uvc driver and it is always
> safe for uvc devices.
> 
> I have tested the below patch, and it can make the uvc device work
> well after rpm resume and system resume(reset resume), both in
> streaming on and off case.
> 
> Alan, Laurent, if you have no objections, I will submit a formal one.
> 
> diff --git a/drivers/media/video/uvc/uvc_driver.c
> b/drivers/media/video/uvc/uvc_driver.c
> index b6eae48..4055dfc 100644
> --- a/drivers/media/video/uvc/uvc_driver.c
> +++ b/drivers/media/video/uvc/uvc_driver.c
> @@ -1959,8 +1959,12 @@ static int __uvc_resume(struct usb_interface
> *intf, int reset)
>  	}
> 
>  	list_for_each_entry(stream, &dev->streams, list) {
> -		if (stream->intf == intf)
> +		if (stream->intf == intf) {
> +			if (reset)
> +				usb_set_interface(stream->dev->udev,
> +					stream->intfnum, 0);
>  			return uvc_video_resume(stream);
> +		}
>  	}
> 
>  	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "

This is fine with me.  However, it is strange that the Set-Interface
request is necessary.  After being reset, the device should
automatically be in altsetting 0 for all interfaces.

Alan Stern

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Ming Lei]

Hi,

On Fri, Jul 15, 2011 at 10:27 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

> This is fine with me.  However, it is strange that the Set-Interface
> request is necessary.  After being reset, the device should
> automatically be in altsetting 0 for all interfaces.

For uvc devices, seems it is not strange, see uvc_video_init(), which
is called in .probe path and executes Set-Interface 0 first, then starts
to get/set video control.

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 784 bytes --]

On Fri, 15 Jul 2011, Ming Lei wrote:

> Hi,
> 
> On Fri, Jul 15, 2011 at 10:27 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This is fine with me.  However, it is strange that the Set-Interface
> > request is necessary.  After being reset, the device should
> > automatically be in altsetting 0 for all interfaces.
> 
> For uvc devices, seems it is not strange, see uvc_video_init(), which
> is called in .probe path and executes Set-Interface 0 first, then starts
> to get/set video control.

I see what you mean.  Apparently other UVC devices also need to receive
a Set-Interface(0) request before they will work right.  (It's still 
strange, though...)

Anyway, since the driver does this during probe, it makes sense to do 
it during reset-resume as well.

Alan Stern

[PATCH] uvcvideo: add SetInterface(0) in .reset_resume handler

[Ming Lei]

As commented in uvc_video_init,

	/* Alternate setting 0 should be the default, yet the XBox Live Vision
	 * Cam (and possibly other devices) crash or otherwise misbehave if
	 * they don't receive a SET_INTERFACE request before any other video
	 * control request.
	 */

so it does make sense to add the SetInterface(0) in .reset_resume
handler so that this kind of devices can work well if they are reseted
during resume from system or runtime suspend.

We have found, without the patch, Microdia camera(0c45:6437) can't send
stream data any longer after it is reseted during resume from
system suspend.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/media/video/uvc/uvc_driver.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/uvc/uvc_driver.c b/drivers/media/video/uvc/uvc_driver.c
index b6eae48..41c6d1a 100644
--- a/drivers/media/video/uvc/uvc_driver.c
+++ b/drivers/media/video/uvc/uvc_driver.c
@@ -1959,8 +1959,20 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 	}
 
 	list_for_each_entry(stream, &dev->streams, list) {
-		if (stream->intf == intf)
+		if (stream->intf == intf) {
+			/*
+			 * After usb bus reset, some devices may
+			 * misbehave if SetInterface(0) is not done, for
+			 * example, Microdia camera(0c45:6437) will stop
+			 * sending streaming data. I think XBox Live
+			 * Vision Cam needs it too, as commented in
+			 * uvc_video_init.
+			 */
+			if (reset)
+				usb_set_interface(stream->dev->udev,
+					stream->intfnum, 0);
 			return uvc_video_resume(stream);
+		}
 	}
 
 	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "
-- 
1.7.4.1




-- 
Ming Lei

Re: [PATCH] uvcvideo: add SetInterface(0) in .reset_resume handler

[Laurent Pinchart]

Hi Ming,

Thanks for the patch. I've queued it for v3.2 with a small modification (the 
usb_set_interface() call has been moved to uvc_video.c).

On Saturday 16 July 2011 05:51:00 Ming Lei wrote:
> As commented in uvc_video_init,
> 
> 	/* Alternate setting 0 should be the default, yet the XBox Live Vision
> 	 * Cam (and possibly other devices) crash or otherwise misbehave if
> 	 * they don't receive a SET_INTERFACE request before any other video
> 	 * control request.
> 	 */
> 
> so it does make sense to add the SetInterface(0) in .reset_resume
> handler so that this kind of devices can work well if they are reseted
> during resume from system or runtime suspend.
> 
> We have found, without the patch, Microdia camera(0c45:6437) can't send
> stream data any longer after it is reseted during resume from
> system suspend.
> 
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/media/video/uvc/uvc_driver.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/uvc/uvc_driver.c
> b/drivers/media/video/uvc/uvc_driver.c index b6eae48..41c6d1a 100644
> --- a/drivers/media/video/uvc/uvc_driver.c
> +++ b/drivers/media/video/uvc/uvc_driver.c
> @@ -1959,8 +1959,20 @@ static int __uvc_resume(struct usb_interface *intf,
> int reset) }
> 
>  	list_for_each_entry(stream, &dev->streams, list) {
> -		if (stream->intf == intf)
> +		if (stream->intf == intf) {
> +			/*
> +			 * After usb bus reset, some devices may
> +			 * misbehave if SetInterface(0) is not done, for
> +			 * example, Microdia camera(0c45:6437) will stop
> +			 * sending streaming data. I think XBox Live
> +			 * Vision Cam needs it too, as commented in
> +			 * uvc_video_init.
> +			 */
> +			if (reset)
> +				usb_set_interface(stream->dev->udev,
> +					stream->intfnum, 0);
>  			return uvc_video_resume(stream);
> +		}
>  	}
> 
>  	uvc_trace(UVC_TRACE_SUSPEND, "Resume: video streaming USB interface "

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add SetInterface(0) in .reset_resume handler

[Ming Lei]

Hi,

On Sun, Jul 31, 2011 at 11:38 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ming,
>
> Thanks for the patch. I've queued it for v3.2 with a small modification (the
> usb_set_interface() call has been moved to uvc_video.c).

Thanks for queuing it.

Considered it is a fix patch, could you queue it for 3.1 -rcX as fix patch?
But anyway, it is up to you, :-)

thanks,
-- 
Ming Lei

Re: [PATCH] uvcvideo: add SetInterface(0) in .reset_resume handler

[Laurent Pinchart]

Hi Ming,

On Monday 01 August 2011 02:56:59 Ming Lei wrote:
> On Sun, Jul 31, 2011 at 11:38 PM, Laurent Pinchart wrote:
> > Hi Ming,
> > 
> > Thanks for the patch. I've queued it for v3.2 with a small modification
> > (the usb_set_interface() call has been moved to uvc_video.c).
> 
> Thanks for queuing it.
> 
> Considered it is a fix patch, could you queue it for 3.1 -rcX as fix patch?
> But anyway, it is up to you, :-)

It's not completely up to me :-) This patch falls in the "features that never 
worked" category. I've heard that Linus didn't want such non-regression fixes 
during the 3.0-rc phase. Mauro, is it still true for v3.1-rc ? Can I push this 
patch for 3.1, or does it need to wait until 3.2 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add SetInterface(0) in .reset_resume handler

[Mauro Carvalho Chehab]

Em 01-08-2011 08:26, Laurent Pinchart escreveu:
> Hi Ming,
> 
> On Monday 01 August 2011 02:56:59 Ming Lei wrote:
>> On Sun, Jul 31, 2011 at 11:38 PM, Laurent Pinchart wrote:
>>> Hi Ming,
>>>
>>> Thanks for the patch. I've queued it for v3.2 with a small modification
>>> (the usb_set_interface() call has been moved to uvc_video.c).
>>
>> Thanks for queuing it.
>>
>> Considered it is a fix patch, could you queue it for 3.1 -rcX as fix patch?
>> But anyway, it is up to you, :-)
> 
> It's not completely up to me :-) This patch falls in the "features that never 
> worked" category. I've heard that Linus didn't want such non-regression fixes 
> during the 3.0-rc phase. Mauro, is it still true for v3.1-rc ? Can I push this 
> patch for 3.1, or does it need to wait until 3.2 ?

Theoretically, we're still with the open window. Well, send it to me. It is
a fix. I'll likely queue it to 3.1 and send on a next pull request, together with
a few other fixes probably on the next weekend.

Thanks,
Mauro

[GIT PATCH FOR v3.1] uvcvideo: Set alternate setting 0 on resume if the bus has been reset

[Laurent Pinchart]

Hi Mauro,

The following changes since commit 46540f7ac646ada7f22912ea7ea9b761ff5c4718:

  [media] ir-mce_kbd-decoder: include module.h for its facilities (2011-07-29 
12:52:23 -0300)

are available in the git repository at:
  git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-stable

Ming Lei (1):
      uvcvideo: Set alternate setting 0 on resume if the bus has been reset

 drivers/media/video/uvc/uvc_driver.c |    2 +-
 drivers/media/video/uvc/uvc_video.c  |   10 +++++++++-
 drivers/media/video/uvc/uvcvideo.h   |    2 +-
 3 files changed, 11 insertions(+), 3 deletions(-)

-- 
Regards,

Laurent Pinchart