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

[Inki Dae]

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


Thanks,
Inki Dae

> > +}
> > +
> >  int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault
> *vmf)
> >  {
> >        struct drm_gem_object *obj = vma->vm_private_data;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > index 14d038b..4f1ba1a 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > @@ -162,4 +162,8 @@ int exynos_drm_gem_fault(struct vm_area_struct *vma,
> struct vm_fault *vmf);
> >  /* set vm_flags and we can change the vm attribute to other one at here.
> */
> >  int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> >
> > +/* do extra release exynos gem module needs when gem close is called.
> */
> > +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> > +                               struct drm_file *file);
> > +
> >  #endif
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel