[PATCHv7 03/15] v4l: vb2: add support for shared buffer (dma_buf)

[Dima Zavin]

On Tue, Jun 26, 2012 at 1:40 AM, Tomasz Stanislawski
<t.stanislaws at samsung.com> 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:
>> 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 <sumit.semwal at ti.com>
>> Cc: Tomasz Stanislawski <t.stanislaws at samsung.com>
>> Signed-off-by: Dima Zavin <dima at android.com>
>> ---
>>  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?

>
>> +                     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

>
>> +                             call_memop(q, unmap_dmabuf, p->mem_priv);
>> +                             p->dbuf_mapped = 0;
>> +                     }
>> +             }
>> +     }
>>  }
>>
>>  /**
>
> Regards,
> Tomasz Stanislawski