[PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Daniel Vetter]

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