Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf
From: Javier Martinez Canillas
Date: Wed Jul 20 2016 - 10:19:16 EST
On 07/20/2016 10:12 AM, Hans Verkuil wrote:
> On 07/20/2016 04:06 PM, Javier Martinez Canillas wrote:
>> Hello Sakari,
>> On 07/20/2016 09:20 AM, Sakari Ailus wrote:
>>> Hi Javier,
>>> On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
>>>> The buffer planes' dma-buf are currently mapped when buffers are queued
>>>> from userspace but it's more appropriate to do the mapping when buffers
>>>> are queued in the driver since that's when the actual DMA operation are
>>>> going to happen.
>>>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>>>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>>>> A side effect of this change is that if the dmabuf map fails for some
>>>> reasons (i.e: a driver using the DMA contig memory allocator but CMA
>>>> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
>>>> later (i.e: in VIDIOC_STREAMON).
>>>> I don't know if that's an issue though but I think is worth mentioning.
>>> I have the same question has Hans --- why?
>> Yes, sorry for missing this information. Nicolas already explained a little
>> bit but the context is that I want to add dma-buf fence support to videobuf2,
>> and currently the dma-buf is unmapped in VIDIOC_DQBUF.
>> But with dma-buf fence, the idea is to be able to dequeue a buffer even when
>> the driver has not yet finished processing the buffer. So the dma-buf needs to
>> be mapped until vb2_buffer_done() when the driver is done processing the vb2,
>> and is able to signal the pending fence.
>> Since the unmapping was going to be delayed to vb2_buffer_done(), I thought
>> it would make sense to also move the mapping closer to when is really going
>> to be used and that's why I moved it to __enqueue_in_driver() in this patch.
>> But I didn't know that user-space was using the dma-buf map as a way to know
>> if the dma-buf will be compatible and fallback to a different streaming I/O
>> method if that's not the case. So $SUBJECT is wrong if it prevents user-space
>> to recover gracefully from a dma-buf mapping failure.
>> In any case, only delaying the unmapping is needed to support fence and doing
>> the map early in VIDIOC_QBUF is not an issue.
> OK. I've rejected this patch. I understand the DQBUF part and I happily accept
> a patch for that. But the other side should be left as-is. The TODO comment
> should probably be dropped, now that I think about it.
I can post such a patch, do you want me to also add a comment about why is done
in QBUF instead of when the buffer is queued in the driver (e.g: that user-space
is able to recover in QBUF but no in STREAMON) or just removing it and mention
that in the commit message is enough?
Javier Martinez Canillas
Open Source Group
Samsung Research America