Message-ID: <20120109102726.GD3723@phenom.ffwll.local>
Date:	Mon, 9 Jan 2012 11:27:26 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	InKi Dae <daeinki@...il.com>
Cc:	Sumit Semwal <sumit.semwal@...com>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
	linaro-mm-sig@...ts.linaro.org, dri-devel@...ts.freedesktop.org,
	linux-media@...r.kernel.org, linux@....linux.org.uk, arnd@...db.de,
	jesse.barker@...aro.org, m.szyprowski@...sung.com, rob@...com,
	t.stanislaws@...sung.com, Sumit Semwal <sumit.semwal@...aro.org>,
	daniel@...ll.ch
Subject: Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism

On Mon, Jan 09, 2012 at 07:10:25PM +0900, InKi Dae wrote:
> 2012/1/9 Daniel Vetter <daniel@...ll.ch>:
> > On Mon, Jan 09, 2012 at 03:20:48PM +0900, InKi Dae wrote:
> >> I has test dmabuf based drm gem module for exynos and I found one problem.
> >> you can refer to this test repository:
> >> http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/exynos-drm-dmabuf
> >>
> >> at this repository, I added some exception codes for resource release
> >> in addition to Dave's patch sets.
> >>
> >> let's suppose we use dmabuf based vb2 and drm gem with physically
> >> continuous memory(no IOMMU) and we try to share allocated buffer
> >> between them(v4l2 and drm driver).
> >>
> >> 1. request memory allocation through drm gem interface.
> >> 2. request DRM_SET_PRIME ioctl with the gem handle to get a fd to the
> >> gem object.
> >> - internally, private gem based dmabuf moudle calls drm_buf_export()
> >> to register allocated gem object to fd.
> >> 3. request qbuf with the fd(got from 2) and DMABUF type to set the
> >> buffer to v4l2 based device.
> >> - internally, vb2 plug in module gets a buffer to the fd and then
> >> calls dmabuf->ops->map_dmabuf() callback to get the sg table
> >> containing physical memory info to the gem object. and then the
> >> physical memory info would be copied to vb2_xx_buf object.
> >> for DMABUF feature for v4l2 and videobuf2 framework, you can refer to
> >> this repository:
> >> git://github.com/robclark/kernel-omap4.git drmplane-dmabuf
> >>
> >> after that, if v4l2 driver want to release vb2_xx_buf object with
> >> allocated memory region by user request, how should we do?. refcount
> >> to vb2_xx_buf is dependent on videobuf2 framework. so when vb2_xx_buf
> >> object is released videobuf2 framework don't know who is using the
> >> physical memory region. so this physical memory region is released and
> >> when drm driver tries to access the region or to release it also, a
> >> problem would be induced.
> >>
> >> for this problem, I added get_shared_cnt() callback to dma-buf.h but
> >> I'm not sure that this is good way. maybe there may be better way.
> >> if there is any missing point, please let me know.
> >
> > The dma_buf object needs to hold a reference on the underlying
> > (necessarily reference-counted) buffer object when the exporter creates
> > the dma_buf handle. This reference should then get dropped in the
> > exporters dma_buf->ops->release() function, which is only getting called
> > when the last reference to the dma_buf disappears.
> >
> 
> when the exporter creates the dma_buf handle(for example, gem -> fd),
> I think the refcount of gem object should be increased at this point,
> and decreased by dma_buf->ops->release() again because when the
> dma_buf is created and dma_buf_export() is called, this dma_buf refers
> to the gem object one time. and in case of inporter(fd -> gem),
> file->f_count of the dma_buf is increased and then when this gem
> object is released by user request such as drm close or
> drn_gem_close_ioctl, dma_buf_put() should be called by
> dma_buf->ops->detach() to decrease file->f_count again because the gem
> object refers to the dma_buf. for this, you can refer to my test
> repository I mentioned above. but the problem is that when a buffer is
> released by one side, another can't know whether the buffer already
> was released or not.

Nope, dma_buf_put should not be called by ->detach. The importer gets his
reference when importing the dma_buf and needs to drop that reference
himself when it's done using the buffer by calling dma_buf_put (i.e. after
the last ->detach call). I think adding separate reference counting to
->attach and ->detach is a waste of time and only papers over buggy
importers.

Additionally the importer does _not_ control the lifetime of an dma_buf
object and it's underlying backing storage. It hence may _never_ free the
backing storage itself, that's the job of the exporter.

With that cleared up, referencing the exporters underlying buffer object
from the dma_buf will just do the right thing.

> note : in case of sharing a buffer between v4l2 and drm driver, the
> memory info would be copied vb2_xx_buf to xx_gem or xx_gem to
> vb2_xx_buf through sg table. in this case, only memory info is used to
> share, not some objects.

Hm, maybe I need to take a look at the currently proposed v4l dma_buf
patches ;-) atm I don't have an idea what exactly you're talking about.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@...ll.ch
Mobile: +41 (0)79 365 57 48
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists