On Tue, Jun 26, 2012 at 1:40 AM, Tomasz Stanislawski
[off-list ref] wrote:

Hi Dima Zavin,
Thank you for the patch and for a ping remainder :).

You are right. The unmap is missing in __vb2_queue_cancel.
I will apply your fix into next version of V4L2 support for dmabuf.

Please refer to some comments below.

On 06/20/2012 08:12 AM, Dima Zavin wrote:

quoted

Tomasz,

I've encountered an issue with this patch when userspace does several
stream_on/stream_off cycles. When the user tries to qbuf a buffer
after doing stream_off, we trigger the "dmabuf already pinned" warning
since we didn't unmap the buffer as dqbuf was never called.

The below patch adds calls to unmap in queue_cancel, but my feeling is that we
probably should be calling detach too (i.e. put_dmabuf).

Thoughts?

--Dima

Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event

Currently, if the user issues a STREAM_OFF request and then
tries to re-enqueue buffers, it will trigger a warning in
the vb2 allocators as the buffer would still be mapped
from before STREAM_OFF was called. The current expectation
is that buffers will be unmapped in dqbuf, but that will never
be called on the mapped buffers after a STREAM_OFF event.

Cc: Sumit Semwal <redacted>
Cc: Tomasz Stanislawski <redacted>
Signed-off-by: Dima Zavin <redacted>
---
 drivers/media/video/videobuf2-core.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index b431dc6..e2a8f12 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c

@@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q)

      /*
       * Reinitialize all buffers for next use.
       */
-     for (i = 0; i < q->num_buffers; ++i)
-             q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED;
+     for (i = 0; i < q->num_buffers; ++i) {
+             struct vb2_buffer *vb = q->bufs[i];
+             int plane;
+
+             vb->state = VB2_BUF_STATE_DEQUEUED;
+
+             if (q->memory != V4L2_MEMORY_DMABUF)
+                     continue;
+
+             for (plane = 0; plane < vb->num_planes; ++plane) {
+                     struct vb2_plane *p = &vb->planes[plane];
+
+                     if (!p->mem_priv)
+                             continue;

is the check above really needed? No check like this is done in
vb2_dqbuf.

I added it because queue_cancel gets called in release, so you never
know what the state of the buffers will be. If we called req_bufs to
allocate the buffer descriptors and then call release, then won't we
have the vb2_buffer structs but nothing in mem_priv since we haven't
attached a dmabuf yet?

quoted

+                     if (p->dbuf_mapped) {

If a buffer is queued then it is also mapped, so dbuf_mapped
should be always be true here (at least in theory).

This loop doesn't check for what the buffer status was, it just
iterates over the entire queue. The buffer may have been put into
STATE_ERROR, and then you don't know if it was ever mapped, etc. It
should always be safe to just follow the flag that says you mapped it
and unmap it. If you think that this should always be true, we can
change it to:

    if (!WARN_ON(!p->dbuf_mapped))

or something like that.

Thanks!

--Dima

quoted

+                             call_memop(q, unmap_dmabuf, p->mem_priv);
+                             p->dbuf_mapped = 0;
+                     }
+             }
+     }
 }

 /**

Regards,
Tomasz Stanislawski