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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux