[PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[Lad, Prabhakar]

This patch drops driver specific wait_prepare() and
wait_finish() callbacks from vb2_ops and instead uses
the the helpers vb2_ops_wait_prepare/finish() provided
by the vb2 core, the lock member of the queue needs
to be initalized to a mutex so that vb2 helpers
vb2_ops_wait_prepare/finish() can make use of it.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_queue.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index cc96072..10c554e 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -143,20 +143,6 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
 		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
 }
 
-static void uvc_wait_prepare(struct vb2_queue *vq)
-{
-	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
-
-	mutex_unlock(&queue->mutex);
-}
-
-static void uvc_wait_finish(struct vb2_queue *vq)
-{
-	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
-
-	mutex_lock(&queue->mutex);
-}
-
 static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
@@ -195,8 +181,8 @@ static struct vb2_ops uvc_queue_qops = {
 	.buf_prepare = uvc_buffer_prepare,
 	.buf_queue = uvc_buffer_queue,
 	.buf_finish = uvc_buffer_finish,
-	.wait_prepare = uvc_wait_prepare,
-	.wait_finish = uvc_wait_finish,
+	.wait_prepare = vb2_ops_wait_prepare,
+	.wait_finish = vb2_ops_wait_finish,
 	.start_streaming = uvc_start_streaming,
 	.stop_streaming = uvc_stop_streaming,
 };
@@ -214,6 +200,7 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
 	queue->queue.mem_ops = &vb2_vmalloc_memops;
 	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
 		| V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
+	queue->queue.lock = &queue->mutex;
 	ret = vb2_queue_init(&queue->queue);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Wednesday 26 November 2014 23:25:44 Lad, Prabhakar wrote:
> This patch drops driver specific wait_prepare() and
> wait_finish() callbacks from vb2_ops and instead uses
> the the helpers vb2_ops_wait_prepare/finish() provided
> by the vb2 core, the lock member of the queue needs
> to be initalized to a mutex so that vb2 helpers
> vb2_ops_wait_prepare/finish() can make use of it.
> 
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index cc96072..10c554e 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -143,20 +143,6 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>  		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
>  }
> 
> -static void uvc_wait_prepare(struct vb2_queue *vq)
> -{
> -	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> -
> -	mutex_unlock(&queue->mutex);
> -}
> -
> -static void uvc_wait_finish(struct vb2_queue *vq)
> -{
> -	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> -
> -	mutex_lock(&queue->mutex);
> -}
> -
>  static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> @@ -195,8 +181,8 @@ static struct vb2_ops uvc_queue_qops = {
>  	.buf_prepare = uvc_buffer_prepare,
>  	.buf_queue = uvc_buffer_queue,
>  	.buf_finish = uvc_buffer_finish,
> -	.wait_prepare = uvc_wait_prepare,
> -	.wait_finish = uvc_wait_finish,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
>  	.start_streaming = uvc_start_streaming,
>  	.stop_streaming = uvc_stop_streaming,
>  };
> @@ -214,6 +200,7 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum
> v4l2_buf_type type, queue->queue.mem_ops = &vb2_vmalloc_memops;
>  	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> 
>  		| V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> 
> +	queue->queue.lock = &queue->mutex;

I'm a bit concerned that this would introduce future breakages. Setting the 
queue.lock pointer enables locking in all vb2_fop_* and vb2_ops_wait_* 
functions. The uvcvideo driver isn't ready for that, but doesn't use the 
vb2_fop_* functions yet, so that's not an issue. However, in the future, 
videobuf2 might use the lock in more places, including functions used by the 
uvcvideo driver. This could then cause breakages.

It would be better to completely convert the uvcvideo driver to the vb2_fop_* 
functions if we want to use vb2_ops_*. I'm not sure how complex that would be 
though, and whether it would be possible while still keeping the fine-grained 
locking implemented by the uvcvideo driver. Do you think it should be 
attempted ?

>  	ret = vb2_queue_init(&queue->queue);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[Prabhakar Lad]

Hi Laurent,

Thanks for the review.

On Thu, Nov 27, 2014 at 9:32 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
[Snip]
>>
>> +     queue->queue.lock = &queue->mutex;
>
> I'm a bit concerned that this would introduce future breakages. Setting the
> queue.lock pointer enables locking in all vb2_fop_* and vb2_ops_wait_*
> functions. The uvcvideo driver isn't ready for that, but doesn't use the
> vb2_fop_* functions yet, so that's not an issue. However, in the future,
> videobuf2 might use the lock in more places, including functions used by the
> uvcvideo driver. This could then cause breakages.
>
Even if in future if videobuf2 uses this lock it would be in helpers mostly,
so any way it doesn’t harm :)

> It would be better to completely convert the uvcvideo driver to the vb2_fop_*
> functions if we want to use vb2_ops_*. I'm not sure how complex that would be
> though, and whether it would be possible while still keeping the fine-grained
> locking implemented by the uvcvideo driver. Do you think it should be
> attempted ?
>
mmap & poll should be fairly simple, looks like open & release cannot be dropped
as it does some usb_autopm_get/put_interface() calls which I am not aware of.

Thanks,
--Prabhakar Lad

>>       ret = vb2_queue_init(&queue->queue);
>>       if (ret)
>>               return ret;
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[Laurent Pinchart]

Hi Prabhakar,

On Friday 28 November 2014 17:06:52 Prabhakar Lad wrote:
> On Thu, Nov 27, 2014 at 9:32 PM, Laurent Pinchart wrote:
> > Hi Prabhakar,
> 
> [Snip]
> 
> >> +     queue->queue.lock = &queue->mutex;
> > 
> > I'm a bit concerned that this would introduce future breakages. Setting
> > the queue.lock pointer enables locking in all vb2_fop_* and vb2_ops_wait_*
> > functions. The uvcvideo driver isn't ready for that, but doesn't use the
> > vb2_fop_* functions yet, so that's not an issue. However, in the future,
> > videobuf2 might use the lock in more places, including functions used by
> > the uvcvideo driver. This could then cause breakages.
> 
> Even if in future if videobuf2 uses this lock it would be in helpers mostly,
> so any way it doesn’t harm :)

My concern is about vb2 starting using the lock in existing helpers used by 
the uvcvideo driver. I suppose we can deal with it later.

> > It would be better to completely convert the uvcvideo driver to the
> > vb2_fop_* functions if we want to use vb2_ops_*. I'm not sure how complex
> > that would be though, and whether it would be possible while still
> > keeping the fine-grained locking implemented by the uvcvideo driver. Do
> > you think it should be attempted ?
> 
> mmap & poll should be fairly simple, looks like open & release cannot be
> dropped as it does some usb_autopm_get/put_interface() calls which I am not
> aware of.

I've looked at that, and there's a race condition in vb2_fop_poll() (for which 
I've already sent a patch) and possible in vb2_mmap() (raised the issue on 
#v4l today) as well that need to be fixed first.

Anyway, for this patch

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Have you tested it by the way ?

Should I take it in my tree or will you send a pull request for the whole 
series ?

> >>       ret = vb2_queue_init(&queue->queue);
> >>       if (ret)
> >>               return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] media: usb: uvc: use vb2_ops_wait_prepare/finish helper

[Prabhakar Lad]

Hi Laurent,

On Sat, Nov 29, 2014 at 6:11 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> On Friday 28 November 2014 17:06:52 Prabhakar Lad wrote:
>> On Thu, Nov 27, 2014 at 9:32 PM, Laurent Pinchart wrote:
>> > Hi Prabhakar,
>>
>> [Snip]
>>
>> >> +     queue->queue.lock = &queue->mutex;
>> >
>> > I'm a bit concerned that this would introduce future breakages. Setting
>> > the queue.lock pointer enables locking in all vb2_fop_* and vb2_ops_wait_*
>> > functions. The uvcvideo driver isn't ready for that, but doesn't use the
>> > vb2_fop_* functions yet, so that's not an issue. However, in the future,
>> > videobuf2 might use the lock in more places, including functions used by
>> > the uvcvideo driver. This could then cause breakages.
>>
>> Even if in future if videobuf2 uses this lock it would be in helpers mostly,
>> so any way it doesn’t harm :)
>
> My concern is about vb2 starting using the lock in existing helpers used by
> the uvcvideo driver. I suppose we can deal with it later.
>
>> > It would be better to completely convert the uvcvideo driver to the
>> > vb2_fop_* functions if we want to use vb2_ops_*. I'm not sure how complex
>> > that would be though, and whether it would be possible while still
>> > keeping the fine-grained locking implemented by the uvcvideo driver. Do
>> > you think it should be attempted ?
>>
>> mmap & poll should be fairly simple, looks like open & release cannot be
>> dropped as it does some usb_autopm_get/put_interface() calls which I am not
>> aware of.
>
> I've looked at that, and there's a race condition in vb2_fop_poll() (for which
> I've already sent a patch) and possible in vb2_mmap() (raised the issue on
> #v4l today) as well that need to be fixed first.
>
> Anyway, for this patch
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Have you tested it by the way ?
>
I have just compile tested it.

> Should I take it in my tree or will you send a pull request for the whole
> series ?
>
Probably you can pick this up via your tree.

Thanks,
--Prabhakar Lad

>> >>       ret = vb2_queue_init(&queue->queue);
>> >>       if (ret)
>> >>               return ret;
>
> --
> Regards,
>
> Laurent Pinchart
>