[RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

[daeinki@gmail.com (InKi Dae)]

2012/1/10 InKi Dae <daeinki@gmail.com>:
> 2012/1/10 Semwal, Sumit <sumit.semwal@ti.com>:
>> On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark <rob@ti.com> wrote:
>>> On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae <daeinki@gmail.com> wrote:
>>>> 2012/1/10 Rob Clark <rob@ti.com>:
>>>> at least with no IOMMU, the memory information(containing physical
>>>> memory address) would be copied to vb2_xx_buf object if drm gem
>>>> exported its own buffer and vb2 wants to use that buffer at this time,
>>>> sg table is used to share that buffer. and the problem I pointed out
>>>> is that this buffer(also physical memory region) could be released by
>>>> vb2 framework(as you know, vb2_xx_buf object and the memory region for
>>>> buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that
>>>> so some problems would be induced once drm gem tries to release or
>>>> access that buffer. and I have tried to resolve this issue adding
>>>> get_shared_cnt() callback to dma-buf.h but I'm not sure that this is
>>>> good way. maybe there would be better way.
>> Hi Inki,
>> As also mentioned in the documentation patch, importer (the user of
>> the buffer) - in this case for current RFC patches on
>> v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory
>> of the buffer directly - it should only use the dma-buf callbacks in
>> the right sequence to let the exporter know that it is done using this
>> buffer, so the exporter can release it if allowed and needed.
>
> thank you for your comments.:) and below are some tables about dmabuf
> operations with ideal use and these tables indicate reference count of
> when buffer is created, shared and released. so if there are any
> problems, please let me know. P.S. these are just simple cases so
> there would be others.
>
>
> in case of using only drm gem and dmabuf,
>
> operations ? ? ? ? ? ? ? ? ? ? ? gem refcount ? ?file f_count ? buf refcount
> ------------------------------------------------------------------------------------------------
> 1. gem create ? ? ? ? ? ? ? ? ? A:1 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? A:0
> 2. export(handle A -> fd) ? ?A:2 ? ? ? ? ? ? ? ?A:1 ? ? ? ? ? ? ?A:0
> 3. import(fd -> handle B) ? ?A:2, B:1 ? ? ? ? A:2 ? ? ? ? ? ? ?A:1
> 4. file close(A) ? ? ? ? ? ? ? ? ?A:2, B:1 ? ? ? ? A:1 ? ? ? ? ? ? ?A:1
> 5. gem close(A) ? ? ? ? ? ? ? ?A:1, B:1 ? ? ? ? A:1 ? ? ? ? ? ? ?A:1
> 6. gem close(B) ? ? ? ? ? ? ? ?A:1, B:0 ? ? ? ? A:1 ? ? ? ? ? ? ?A:0
> 7. file close(A) ? ? ? ? ? ? ? ? ?A:0 ? ? ? ? ? ? ? ?A:0
> -----------------------------------------------------------------------------------------------
> 3. handle B shares the buf of handle A.
> 6. release handle B but its buf.
> 7. release gem handle A and dmabuf of file A and also physical memory region.
>
>
> and in case of using drm gem, vb2 and dmabuf,
>
> operations ? ? ? ? ? ? ? ? ?gem, vb2 refcount ? ?file f_count ? buf refcount
> ------------------------------------------------------------------------------------------------
> 1. gem create ? ? ? ? ? ? ? ? ? A:1 ? ? ? ? ? ? ? ? A:0
> ? (GEM side)
> 2. export(handle A -> fd) ? ?A:2 ? ? ? ? ? ? ? ? A:1 ? ? ? ? ? ? ?A:0
> ? (GEM side)
> 3. import(fd -> handle B) ? ?A:2, B:1 ? ? ? ? ?A:2 ? ? ? ? ? ? ?A:1
> ? (VB2 side)
> 4. file close(A) ? ? ? ? ? ? ? ? ?A:2, B:1 ? ? ? ? ?A:1 ? ? ? ? ? ? ?A:1
> ? (VB2 side)
> 5. vb2 close(B) ? ? ? ? ? ? ? ? A:2, B:0 ? ? ? ? ?A:1 ? ? ? ? ? ? ?A:0
> ? (VB2 side)
> 6. gem close(A) ? ? ? ? ? ? ? ?A:1 ? ? ? ? ? ? ? ?A:1 ? ? ? ? ? ? ?A:0
> ? (GEM side)
> 7. file close(A) ? ? ? ? ? ? ? ? ?A:0 ? ? ? ? ? ? ? ?A:0
> ? (GEM side)
> ------------------------------------------------------------------------------------------------
> 3. vb2 handle B is shared with the buf of gem handle A.
> 5. release vb2 handle B and decrease refcount of the buf pointed by it.
> 7. release gem handle A and dmabuf of file A and also physical memory region.
>

Ah, sorry, it seems that file close shouldn't be called because
file->f_count of the file would be dropped by Importer and if f_count
is 0 then the file would be released by fput() so I'm not sure but
again:

in case of using only drm gem and dmabuf,

operations                       gem refcount      file f_count   buf refcount
--------------------------------------------------------------------------------------------------
1. gem create(A)               A:1                                     A:0
2. export(handle A -> fd)    A:2                  A:1              A:0
3. import(fd -> handle B)    A:2, B:1           A:2              A:1
4. gem close(B)                A:2, B:release  A:1              A:0
5. gem close(A)                A:1,                 A:1              A:0
6. gem close(A)                A:release         A:release     A:release
-------------------------------------------------------------------------------------------------

and in case of using drm gem, vb2 and dmabuf,

operations                  gem, vb2 refcount    file f_count   buf refcount
------------------------------------------------------------------------------------------------
1. gem create                   A:1                 A:0              A:0
  (GEM side)
2. export(handle A -> fd)    A:2                 A:1              A:0
  (GEM side)
3. import(fd -> handle B)    A:2, B:1          A:2              A:1
  (VB2 side)
5. vb2 close(B)                 A:2, B:release  A:1              A:0
  (VB2 side)
6. gem close(A)                A:1                A:1              A:0
  (GEM side)
6. gem close(A)                A:release       A:release      A:release
  (GEM side)
------------------------------------------------------------------------------------------------

>>>
>>> the exporter (in this case your driver's drm/gem bits) shouldn't
>>> release that mapping / sgtable until the importer (in this case v4l2)
>>> calls dma_buf_unmap fxn..
>>>
>>> It would be an error if the importer did a dma_buf_put() without first
>>> calling dma_buf_unmap_attachment() (if currently mapped) and then
>>> dma_buf_detach() (if currently attached). ?Perhaps somewhere there
>>> should be some sanity checking debug code which could be enabled to do
>>> a WARN_ON() if the importer does the wrong thing. ?It shouldn't really
>>> be part of the API, I don't think, but it actually does seem like a
>>> good thing, esp. as new drivers start trying to use dmabuf, to have
>>> some debug options which could be enabled.
>>>
>>> It is entirely possible that something was missed on the vb2 patches,
>>> but the way it is intended to work is like this:
>>> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-core.c#L92
>>>
>>> where it does a detach() before the dma_buf_put(), and the vb2-contig
>>> backend checks here that it is also unmapped():
>>> https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-dma-contig.c#L251
>>
>> The proposed RFC for V4L2 adaptation at [1] does exactly the same
>> thing; detach() before dma_buf_put(), and check in vb2-contig backend
>> for unmapped() as mentioned above.
>>
>>>
>>> BR,
>>> -R
>>>
>> BR,
>> Sumit.
>>
>> [1]: V4l2 as a dma-buf user RFC:
>> http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966