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 <[email protected]>
Cc: Laurent Pinchart <[email protected]>
---
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
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 <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> ---
> 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
Hi Laurent,
Thanks for the review.
On Thu, Nov 27, 2014 at 9:32 PM, Laurent Pinchart
<[email protected]> 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
>
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 <[email protected]>
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
Hi Laurent,
On Sat, Nov 29, 2014 at 6:11 PM, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>
> 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
>