2012/1/10 InKi Dae <daeinki@xxxxxxxxx>: > 2012/1/10 Semwal, Sumit <sumit.semwal@xxxxxx>: >> On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark <rob@xxxxxx> wrote: >>> On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae <daeinki@xxxxxxxxx> wrote: >>>> 2012/1/10 Rob Clark <rob@xxxxxx>: >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
