Re: [PATCH v4 6/6] media: uvcvideo: Move decode processing to process context

[Laurent Pinchart]

Hi Tomasz,

On Tuesday, 7 August 2018 12:54:02 EEST Tomasz Figa wrote:
> On Wed, Mar 28, 2018 at 1:47 AM Kieran Bingham wrote:
> [snip]
> 
> > @@ -1544,25 +1594,29 @@ static int uvc_alloc_urb_buffers(struct
> > uvc_streaming *stream,
> >   */
> >  
> >  static void uvc_uninit_video(struct uvc_streaming *stream, int
> >  free_buffers)
> >  {
> > -       struct urb *urb;
> > -       unsigned int i;
> > +       struct uvc_urb *uvc_urb;
> > 
> >         uvc_video_stats_stop(stream);
> > 
> > -       for (i = 0; i < UVC_URBS; ++i) {
> > -               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
> > +       /*
> > +        * We must poison the URBs rather than kill them to ensure that
> > even
> > +        * after the completion handler returns, any asynchronous
> > workqueues
> > +        * will be prevented from resubmitting the URBs
> > +        */
> > +       for_each_uvc_urb(uvc_urb, stream)
> > +               usb_poison_urb(uvc_urb->urb);
> > 
> > -               urb = uvc_urb->urb;
> > -               if (urb == NULL)
> > -                       continue;
> > +       flush_workqueue(stream->async_wq);
> > 
> > -               usb_kill_urb(urb);
> > -               usb_free_urb(urb);
> > +       for_each_uvc_urb(uvc_urb, stream) {
> > +               usb_free_urb(uvc_urb->urb);
> >                 uvc_urb->urb = NULL;
> >         }
> >         
> >         if (free_buffers)
> >                 uvc_free_urb_buffers(stream);
> > +
> > +       destroy_workqueue(stream->async_wq);
> 
> In our testing, this function ends up being called twice, if before
> suspend the camera is streaming and if the camera disconnects between
> suspend and resume. This is because uvc_video_suspend() calls this
> function (with free_buffers = 0), but uvc_video_resume() wouldn't call
> uvc_init_video() due to an earlier failure and uvc_v4l2_release()
> would end up calling this function again, while the workqueue is
> already destroyed.

This makes me wonder, as stated in my review, whether we shouldn't keep the 
work queue alive for the whole lifetime of the device instead of creating and 
destroying it at stream start and stop.

> The following diff seems to take care of it:
> 
> 8<~~~
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index c5e0ab564b1a..6fb890c8ba67 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1493,10 +1493,11 @@ static void uvc_uninit_video(struct
> uvc_streaming *stream, int free_buffers)
>                uvc_urb->urb = NULL;
>        }
> 
> -       if (free_buffers)
> +       if (free_buffers) {
>                uvc_free_urb_buffers(stream);
> -
> -       destroy_workqueue(stream->async_wq);
> +               destroy_workqueue(stream->async_wq);
> +               stream->async_wq = NULL;
> +       }
> }
> 
> /*
> @@ -1648,10 +1649,12 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags)
> 
>        uvc_video_stats_start(stream);
> 
> -       stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND |
> WQ_HIGHPRI, -                       0);
> -       if (!stream->async_wq)
> -               return -ENOMEM;
> +       if (!stream->async_wq) {
> +               stream->async_wq = alloc_workqueue("uvcvideo",
> +                                                  WQ_UNBOUND | WQ_HIGHPRI,
> 0); +               if (!stream->async_wq)
> +                       return -ENOMEM;
> +       }
> 
>        if (intf->num_altsetting > 1) {
>                struct usb_host_endpoint *best_ep = NULL;
> ~~~>8

-- 
Regards,

Laurent Pinchart