RE: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Kaukab, Yousaf]

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: Wednesday, September 9, 2015 6:30 PM
> To: Laurent Pinchart
> Cc: Hans de Goede; Kaukab, Yousaf; linux-media@vger.kernel.org;
> mche...@osg.samsung.com; linux-...@vger.kernel.org
> Subject: Re: [PATCH v1] media: uvcvideo: handle urb completion in a work
> queue
> 
> On Wed, 9 Sep 2015, Laurent Pinchart wrote:
> 
> > > > Instead of fixing the issue in the uvcvideo driver, would it then
> > > > make more sense to fix it in the remaining hcd drivers ?
> > >
> > > Unfortunately that's not so easy.  It involves some subtle changes
> > > related to the way isochronous endpoints are handled.  I wouldn't
> > > know what to change in any of the HCDs, except the ones that I maintain.
> >
> > I'm not saying it would be easy, but I'm wondering whether it makes
> > change to move individual USB device drivers to work queues when the
> > long term goal is to use tasklets for URB completion anyway.
> 
> I'm not sure that this is a long-term goal for every HCD.  For instance, there
> probably isn't much incentive to convert a driver if its host controllers can 
> only
> run at low speed or full speed.
> 

I can convert this patch to use tasklets instead and only schedule the
tasklet if urb->complete is called in interrupt context. This way, hcd
using tasklet or not, uvc driver behavior will be almost same. Only
difference is that local interrupts will still be enabled when tasklet 
scheduled from uvc driver is executing the completion function.

This patch can be dropped once all hcd's start using tasklets for the
completion callback (if that ever happens).

BR,
Yousaf
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Laurent Pinchart]

On Wednesday 09 September 2015 10:30:12 Hans de Goede wrote:
> On 08-09-15 16:36, Alan Stern wrote:
> > On Tue, 8 Sep 2015, Hans de Goede wrote:
> >> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:
> >>> urb completion callback is executed in host controllers interrupt
> >>> context. To keep preempt disable time short, add urbs to a list on
> >>> completion and schedule work to process the list.
> >>> 
> >>> Moreover, save timestamp and sof number in the urb completion callback
> >>> to avoid any delays.
> >> 
> >> Erm, I thought that we had already moved to using threaded interrupt
> >> handling for the urb completion a while (1-2 years ?) back. Is this then
> >> still needed ?
> > 
> > We moved to handling URB completions in a tasklet, not a threaded
> > handler.
> 
> Right.
> 
> > (Similar idea, though.)  And the change was made in only one
> > or two HCDs, not in all of them.
> 
> Ah, I was under the impression this was a core change, not a per
> hcd change.

Instead of fixing the issue in the uvcvideo driver, would it then make more 
sense to fix it in the remaining hcd drivers ?

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Hans de Goede]

Hi,

On 08-09-15 16:36, Alan Stern wrote:

On Tue, 8 Sep 2015, Hans de Goede wrote:


Hi,

On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:

urb completion callback is executed in host controllers interrupt
context. To keep preempt disable time short, add urbs to a list on
completion and schedule work to process the list.

Moreover, save timestamp and sof number in the urb completion callback
to avoid any delays.


Erm, I thought that we had already moved to using threaded interrupt
handling for the urb completion a while (1-2 years ?) back. Is this then
still needed ?


We moved to handling URB completions in a tasklet, not a threaded
handler.


Right.


(Similar idea, though.)  And the change was made in only one
or two HCDs, not in all of them.


Ah, I was under the impression this was a core change, not a per
hcd change.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Alan Stern]

On Wed, 9 Sep 2015, Laurent Pinchart wrote:

> On Wednesday 09 September 2015 10:30:12 Hans de Goede wrote:
> > On 08-09-15 16:36, Alan Stern wrote:
> > > On Tue, 8 Sep 2015, Hans de Goede wrote:
> > >> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:
> > >>> urb completion callback is executed in host controllers interrupt
> > >>> context. To keep preempt disable time short, add urbs to a list on
> > >>> completion and schedule work to process the list.
> > >>> 
> > >>> Moreover, save timestamp and sof number in the urb completion callback
> > >>> to avoid any delays.
> > >> 
> > >> Erm, I thought that we had already moved to using threaded interrupt
> > >> handling for the urb completion a while (1-2 years ?) back. Is this then
> > >> still needed ?
> > > 
> > > We moved to handling URB completions in a tasklet, not a threaded
> > > handler.
> > 
> > Right.
> > 
> > > (Similar idea, though.)  And the change was made in only one
> > > or two HCDs, not in all of them.
> > 
> > Ah, I was under the impression this was a core change, not a per
> > hcd change.
> 
> Instead of fixing the issue in the uvcvideo driver, would it then make more 
> sense to fix it in the remaining hcd drivers ?

Unfortunately that's not so easy.  It involves some subtle changes 
related to the way isochronous endpoints are handled.  I wouldn't know 
what to change in any of the HCDs, except the ones that I maintain.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Laurent Pinchart]

Hi Alan,

On Wednesday 09 September 2015 11:14:38 Alan Stern wrote:
> On Wed, 9 Sep 2015, Laurent Pinchart wrote:
> > On Wednesday 09 September 2015 10:30:12 Hans de Goede wrote:
> >> On 08-09-15 16:36, Alan Stern wrote:
> >>> On Tue, 8 Sep 2015, Hans de Goede wrote:
>  On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:
> > urb completion callback is executed in host controllers interrupt
> > context. To keep preempt disable time short, add urbs to a list on
> > completion and schedule work to process the list.
> > 
> > Moreover, save timestamp and sof number in the urb completion
> > callback to avoid any delays.
>  
>  Erm, I thought that we had already moved to using threaded interrupt
>  handling for the urb completion a while (1-2 years ?) back. Is this
>  then still needed ?
> >>> 
> >>> We moved to handling URB completions in a tasklet, not a threaded
> >>> handler.
> >> 
> >> Right.
> >> 
> >>> (Similar idea, though.)  And the change was made in only one
> >>> or two HCDs, not in all of them.
> >> 
> >> Ah, I was under the impression this was a core change, not a per
> >> hcd change.
> > 
> > Instead of fixing the issue in the uvcvideo driver, would it then make
> > more sense to fix it in the remaining hcd drivers ?
> 
> Unfortunately that's not so easy.  It involves some subtle changes
> related to the way isochronous endpoints are handled.  I wouldn't know
> what to change in any of the HCDs, except the ones that I maintain.

I'm not saying it would be easy, but I'm wondering whether it makes change to 
move individual USB device drivers to work queues when the long term goal is 
to use tasklets for URB completion anyway.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Alan Stern]

On Wed, 9 Sep 2015, Laurent Pinchart wrote:

> > > Instead of fixing the issue in the uvcvideo driver, would it then make
> > > more sense to fix it in the remaining hcd drivers ?
> > 
> > Unfortunately that's not so easy.  It involves some subtle changes
> > related to the way isochronous endpoints are handled.  I wouldn't know
> > what to change in any of the HCDs, except the ones that I maintain.
> 
> I'm not saying it would be easy, but I'm wondering whether it makes change to 
> move individual USB device drivers to work queues when the long term goal is 
> to use tasklets for URB completion anyway.

I'm not sure that this is a long-term goal for every HCD.  For
instance, there probably isn't much incentive to convert a driver if
its host controllers can only run at low speed or full speed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Alan Stern]

On Wed, 9 Sep 2015, Hans de Goede wrote:

> Hi,
> 
> On 08-09-15 16:36, Alan Stern wrote:
> > On Tue, 8 Sep 2015, Hans de Goede wrote:
> >
> >> Hi,
> >>
> >> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:
> >>> urb completion callback is executed in host controllers interrupt
> >>> context. To keep preempt disable time short, add urbs to a list on
> >>> completion and schedule work to process the list.
> >>>
> >>> Moreover, save timestamp and sof number in the urb completion callback
> >>> to avoid any delays.
> >>
> >> Erm, I thought that we had already moved to using threaded interrupt
> >> handling for the urb completion a while (1-2 years ?) back. Is this then
> >> still needed ?
> >
> > We moved to handling URB completions in a tasklet, not a threaded
> > handler.
> 
> Right.
> 
> > (Similar idea, though.)  And the change was made in only one
> > or two HCDs, not in all of them.
> 
> Ah, I was under the impression this was a core change, not a per
> hcd change.

In fact, both the core and the HCD needed to be changed.  For example,
see commits 94dfd7edfd5c (core) and 9118f9eb4f1e (ehci-hcd).  (There
was more to it than just those two commits, of course.)

In one respect the change still isn't complete: Since the completion
callback occurs in a tasklet, we should allow interrupts to remain
enabled while the callback runs.  But some class drivers still expect
interrupts to be disabled at that time, so the core has to disable
interrupts before invoking the callback and then re-enable them
afterward.

There was a proposal to fix up a number of drivers so that we could 
leave interrupts enabled the whole time.  But I don't think it ever got 
merged.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Oliver Neukum]

On Mon, 2015-09-07 at 18:23 +0200, Mian Yousaf Kaukab wrote:
> urb completion callback is executed in host controllers interrupt
> context. To keep preempt disable time short, add urbs to a list on
> completion and schedule work to process the list.
> 
> Moreover, save timestamp and sof number in the urb completion callback
> to avoid any delays.
> 
> Signed-off-by: Mian Yousaf Kaukab 
> ---
> History:
> v1:
>  - Use global work queue instead of creating ordered queue.

1. using a common queue for real-time work is probably not nice for
picture quality
2. it will deadlock under some conditions

The explanation is a bit long

Suppose we have a device with a camera and a storage device,
like an ordinary camera you can use as a video device which also
exports its memory card.

Now we assume that the storage part is suspended.

CPU A   CPU B
work item scheduled
entering uvc_uninit_video()
work item executed
work item allocates memory
write to storage interface
storage interface being resumed
flush_work() - waiting for CPU B
DEADLOCK


If you want to use flush_work() you must use a dedicated queue.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Hans de Goede]

Hi,

On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:

urb completion callback is executed in host controllers interrupt
context. To keep preempt disable time short, add urbs to a list on
completion and schedule work to process the list.

Moreover, save timestamp and sof number in the urb completion callback
to avoid any delays.


Erm, I thought that we had already moved to using threaded interrupt
handling for the urb completion a while (1-2 years ?) back. Is this then
still needed ?

Regards,

Hans




Signed-off-by: Mian Yousaf Kaukab 
---
History:
v1:
  - Use global work queue instead of creating ordered queue.
  - Add completed urbs to a list and process all on the list when work
is scheduled
  - Save timestamp and sof number in urb completion callback and use
in uvc_video_clock_decode() and uvc_video_decode_start()
  - Fix race between usb_submit_urb() from uvc_video_complete() and
usb_kill_urb() from uvc_uninit_video()

  drivers/media/usb/uvc/uvc_isight.c |   3 +-
  drivers/media/usb/uvc/uvc_video.c  | 132 +++--
  drivers/media/usb/uvc/uvcvideo.h   |  17 -
  3 files changed, 113 insertions(+), 39 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_isight.c 
b/drivers/media/usb/uvc/uvc_isight.c
index 8510e725..7f93d09 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -99,9 +99,10 @@ static int isight_decode(struct uvc_video_queue *queue, 
struct uvc_buffer *buf,
return 0;
  }

-void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
+void uvc_video_decode_isight(struct uvc_urb *uu, struct uvc_streaming *stream,
struct uvc_buffer *buf)
  {
+   struct urb *urb = uu->urb;
int ret, i;

for (i = 0; i < urb->number_of_packets; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_video.c 
b/drivers/media/usb/uvc/uvc_video.c
index f839654..71354ec 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -379,15 +379,14 @@ static inline void uvc_video_get_ts(struct timespec *ts)

  static void
  uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
-  const __u8 *data, int len)
+  const __u8 *data, int len, u16 host_sof,
+  struct timespec *ts)
  {
struct uvc_clock_sample *sample;
unsigned int header_size;
bool has_pts = false;
bool has_scr = false;
unsigned long flags;
-   struct timespec ts;
-   u16 host_sof;
u16 dev_sof;

switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
@@ -435,9 +434,6 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct 
uvc_buffer *buf,

stream->clock.last_sof = dev_sof;

-   host_sof = usb_get_current_frame_number(stream->dev->udev);
-   uvc_video_get_ts();
-
/* The UVC specification allows device implementations that can't obtain
 * the USB frame number to keep their own frame counters as long as they
 * match the size and frequency of the frame number associated with USB
@@ -473,7 +469,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct 
uvc_buffer *buf,
sample->dev_stc = get_unaligned_le32([header_size - 6]);
sample->dev_sof = dev_sof;
sample->host_sof = host_sof;
-   sample->host_ts = ts;
+   sample->host_ts = *ts;

/* Update the sliding window head and count. */
stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
@@ -964,7 +960,8 @@ static void uvc_video_stats_stop(struct uvc_streaming 
*stream)
   * uvc_video_decode_end will never be called with a NULL buffer.
   */
  static int uvc_video_decode_start(struct uvc_streaming *stream,
-   struct uvc_buffer *buf, const __u8 *data, int len)
+   struct uvc_buffer *buf, const __u8 *data, int len,
+   u16 host_sof, struct timespec *ts)
  {
__u8 fid;

@@ -989,7 +986,7 @@ static int uvc_video_decode_start(struct uvc_streaming 
*stream,
uvc_video_stats_update(stream);
}

-   uvc_video_clock_decode(stream, buf, data, len);
+   uvc_video_clock_decode(stream, buf, data, len, host_sof, ts);
uvc_video_stats_decode(stream, data, len);

/* Store the payload FID bit and return immediately when the buffer is
@@ -1016,8 +1013,6 @@ static int uvc_video_decode_start(struct uvc_streaming 
*stream,
 * when the EOF bit is set to force synchronisation on the next packet.
 */
if (buf->state != UVC_BUF_STATE_ACTIVE) {
-   struct timespec ts;
-
if (fid == stream->last_fid) {
uvc_trace(UVC_TRACE_FRAME, "Dropping payload (out of "
"sync).\n");
@@ -1027,13 +1022,11 @@ static int uvc_video_decode_start(struct uvc_streaming 
*stream,
return -ENODATA;
   

Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue

[Alan Stern]

On Tue, 8 Sep 2015, Hans de Goede wrote:

> Hi,
> 
> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote:
> > urb completion callback is executed in host controllers interrupt
> > context. To keep preempt disable time short, add urbs to a list on
> > completion and schedule work to process the list.
> >
> > Moreover, save timestamp and sof number in the urb completion callback
> > to avoid any delays.
> 
> Erm, I thought that we had already moved to using threaded interrupt
> handling for the urb completion a while (1-2 years ?) back. Is this then
> still needed ?

We moved to handling URB completions in a tasklet, not a threaded
handler.  (Similar idea, though.)  And the change was made in only one
or two HCDs, not in all of them.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html