On Wed, Jun 27, 2012 at 09:44:15PM +0900, Inki Dae wrote: > Hi Rob, > > > -----Original Message----- > > From: robdclark at gmail.com [mailto:robdclark at gmail.com] On Behalf Of > > Rob > > Clark > > Sent: Wednesday, June 27, 2012 9:20 PM > > To: Inki Dae > > Cc: airlied at linux.ie; dri-devel at lists.freedesktop.org; > > kyungmin.park at samsung.com; sw0312.kim at samsung.com > > Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to > > exported gem buffer into dmabuf > > > > On Wed, Jun 27, 2012 at 3:03 AM, Inki Dae <inki.dae at samsung.com> wrote: > > > exported gem buffer into dmabuf should be released when a gem relese is > > > requested by user. with dma_buf_put() call, if file->f_count is 0 then > > > a release callback of exynos gem module(exporter) will be called to > > release > > > its own gem buffer. > > > > > > Signed-off-by: Inki Dae <inki.dae at samsung.com> > > > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com> > > > --- > > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 1 + > > > drivers/gpu/drm/exynos/exynos_drm_gem.c | 16 ++++++++++++++++ > > > drivers/gpu/drm/exynos/exynos_drm_gem.h | 4 ++++ > > > 3 files changed, 21 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > index d6de2e0..1501dd2 100644 > > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > @@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = { > > > .gem_init_object = exynos_drm_gem_init_object, > > > .gem_free_object = exynos_drm_gem_free_object, > > > .gem_vm_ops = &exynos_drm_gem_vm_ops, > > > + .gem_close_object = exynos_drm_gem_close_object, > > > .dumb_create = exynos_drm_gem_dumb_create, > > > .dumb_map_offset = exynos_drm_gem_dumb_map_offset, > > > .dumb_destroy = exynos_drm_gem_dumb_destroy, > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c > > b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > > index 411d82b..5ca8641 100644 > > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > > @@ -27,6 +27,7 @@ > > > #include "drm.h" > > > > > > #include <linux/shmem_fs.h> > > > +#include <linux/dma-buf.h> > > > #include <drm/exynos_drm.h> > > > > > > #include "exynos_drm_drv.h" > > > @@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file > > *file_priv, > > > return 0; > > > } > > > > > > +void exynos_drm_gem_close_object(struct drm_gem_object *obj, > > > + struct drm_file *file) > > > +{ > > > + DRM_DEBUG_KMS("%s\n", __FILE__); > > > + > > > + /* > > > + * exported dmabuf should be released when a gem is released by > > user. > > > + * with dma_buf_put() call, if file->f_count is 0 then a release > > > + * callback of gem module(exporter) will be called to release > > > + * its own gem buffer. > > > + */ > > > + if (obj->export_dma_buf) > > > + dma_buf_put(obj->export_dma_buf); > > > > this doesn't seem quite right to me.. although I think the dmabuf > > release fxn needs to NULL out the obj->export_dma_buf. > > > > If your GEM obj is getting released before your dmabuf then there is > > something going wrong with the ref cnt'ing. > > > > BR, > > -R > > > > Could you look into below steps? I understood gem and dmabuf life cycle like > below so thought we needs this patch. with this patch, the gem object isn't > getting released before dmabuf and the gem will be released by dma_buf_put(). > if there is my missing point then please give me any comment. > > Reference count(step number) > ==================================== > gem object1 gem object2 dmabuf > ------------------------------------ > 1(1) > 2(2) 1(2) > 1(3) 2(3) > 0(4) 1(4) > 1(5) 0(5) > 0(5) > ==================================== > > 1. create gem1 > 2. export the gem1 into dmabuf > 3. import the dmabuf into gem2 > 4. close gem2 > 5. close gem1
Step 5 looks wrong, simply closing the gem object 1 (in the exporting driver) can't change the reference count of the dmabuf object. Furthermore the dmabuf object _must_ be able to outlive the object it's been created from. E.g. when you use dma-buf fd handles to facilitate cross-process buffer sharing (maybe even on the same drm device, i.e. not necessarily across devices), process A exports the dmabuf and passes the fd and, process B imports it. Currently with dri2/gem_flink process A needs to keep the gem object around until process B has confirmed that it has imported the object, which is really ugly. But because fds themselves hold a reference, this is not required for dma-buf cross-process sharing. But if you break that (whith something like this patch) that won't work. Long story short, your description above is missing step 6: 6. close dma-buf fd > Reference count(step number) > ==================================== > gem object1 gem object2 dmabuf > ------------------------------------ > 1(1) > 2(2) 1(2) > 1(3) 2(3) > 0(4) 1(4) > 1(5) 1(5) > 0(6) > ==================================== Only then may the object's backing storage be freed. Cheers, Daniel PS: There's the funny thing what happens when you import a dma-buf into the same gem/drm device: Then the driver _must_ _not_ create a new gem object, but instead must increment the reference count of the underlying gem object and create a new fd name for that underlying gem object. The driver can check for this case by comparing the dma-buf ops and priv fields. -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
