On Wed, Jun 27, 2012 at 09:44:15PM +0900, Inki Dae wrote: > Hi Rob, > > > -----Original Message----- > > From: robdclark@xxxxxxxxx [mailto:robdclark@xxxxxxxxx] On Behalf Of Rob > > Clark > > Sent: Wednesday, June 27, 2012 9:20 PM > > To: Inki Dae > > Cc: airlied@xxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > > kyungmin.park@xxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx > > 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > > > --- > > > 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@xxxxxxxx Mobile: +41 (0)79 365 57 48 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
