> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Wednesday, June 27, 2012 9:55 PM > To: Inki Dae > Cc: 'Rob Clark'; kyungmin.park@xxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to > exported gem buffer into dmabuf > > 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 Ok, I understood. with this patch, process B can't import the gem if process A already released the gem before that. as you mentioned, I missed step 6. thanks for your comment. > 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. Above case was just simple example. actually our driver checks for that case. Thanks, Inki Dae > -- > 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
