| Message ID | 20110711174811.3c383595@tom-ThinkPad-T410 (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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,
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?
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,
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
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.
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,
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,
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
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(-)