[PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf
Inki Dae
inki.dae at samsung.com
Wed Jun 27 06:31:22 PDT 2012
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 27, 2012 9:55 PM
> To: Inki Dae
> Cc: 'Rob Clark'; kyungmin.park at samsung.com; sw0312.kim at samsung.com; dri-
> devel at lists.freedesktop.org
> 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 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
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 at ffwll.ch
> Mobile: +41 (0)79 365 57 48
More information about the dri-devel
mailing list