From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: RE: [PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf Date: Wed, 27 Jun 2012 22:31:22 +0900 Message-ID: <00a601cd5469$24706f60$6d514e20$%dae@samsung.com> References: <1340784192-21352-1-git-send-email-inki.dae@samsung.com> <1340784192-21352-11-git-send-email-inki.dae@samsung.com> <009501cd5462$8f9fdfc0$aedf9f40$%dae@samsung.com> <20120627125456.GD5326@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout4.samsung.com (mailout4.samsung.com [203.254.224.34]) by gabe.freedesktop.org (Postfix) with ESMTP id 9E8EAA0921 for ; Wed, 27 Jun 2012 06:31:09 -0700 (PDT) Received: from epcpsbgm1.samsung.com (mailout4.samsung.com [203.254.224.34]) by mailout4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M6A00BMJ2VKSH10@mailout4.samsung.com> for dri-devel@lists.freedesktop.org; Wed, 27 Jun 2012 22:31:08 +0900 (KST) Received: from NOINKIDAE02 ([10.90.51.52]) by mmp1.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0M6A008U82VVSF20@mmp1.samsung.com> for dri-devel@lists.freedesktop.org; Wed, 27 Jun 2012 22:31:07 +0900 (KST) In-reply-to: <20120627125456.GD5326@phenom.ffwll.local> Content-language: ko List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: 'Daniel Vetter' Cc: kyungmin.park@samsung.com, sw0312.kim@samsung.com, dri-devel@lists.freedesktop.org, 'Rob Clark' List-Id: dri-devel@lists.freedesktop.org > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Wednesday, June 27, 2012 9:55 PM > To: Inki Dae > Cc: 'Rob Clark'; kyungmin.park@samsung.com; sw0312.kim@samsung.com; dri- > devel@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@gmail.com [mailto:robdclark@gmail.com] On Behalf Of > Rob > > > Clark > > > Sent: Wednesday, June 27, 2012 9:20 PM > > > To: Inki Dae > > > Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org; > > > kyungmin.park@samsung.com; sw0312.kim@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 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 > > > > Signed-off-by: Kyungmin Park > > > > --- > > > > 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 > > > > +#include > > > > #include > > > > > > > > #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@ffwll.ch > Mobile: +41 (0)79 365 57 48