* [PATCH 1/2] android: fix reference leak in sync_fence_create @ 2014-08-14 9:53 Maarten Lankhorst 2014-08-14 9:54 ` [PATCH 2/2] android: add sync_fence_create_dma Maarten Lankhorst 2014-08-18 12:57 ` [PATCH 1/2] android: fix reference leak in sync_fence_create Dan Carpenter 0 siblings, 2 replies; 11+ messages in thread From: Maarten Lankhorst @ 2014-08-14 9:53 UTC (permalink / raw) To: Sumit Semwal, Greg Kroah-Hartman, linaro-mm-sig, LKML Cc: Daniel Vetter, Colin Cross, John Stultz, devel According to the documentation sync_fence_create takes ownership of the point, not a reference on the point. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Colin Cross <ccross@google.com> --- drivers/staging/android/sync.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index e7b2e0234196..69139ce7420d 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -199,7 +199,6 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) fence->num_fences = 1; atomic_set(&fence->status, 1); - fence_get(&pt->base); fence->cbs[0].sync_pt = &pt->base; fence->cbs[0].fence = fence; if (fence_add_callback(&pt->base, &fence->cbs[0].cb, -- 2.0.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] android: add sync_fence_create_dma 2014-08-14 9:53 [PATCH 1/2] android: fix reference leak in sync_fence_create Maarten Lankhorst @ 2014-08-14 9:54 ` Maarten Lankhorst 2014-08-14 20:09 ` Jesse Barnes 2014-08-15 6:46 ` Greg Kroah-Hartman 2014-08-18 12:57 ` [PATCH 1/2] android: fix reference leak in sync_fence_create Dan Carpenter 1 sibling, 2 replies; 11+ messages in thread From: Maarten Lankhorst @ 2014-08-14 9:54 UTC (permalink / raw) To: Sumit Semwal, Greg Kroah-Hartman, linaro-mm-sig, LKML Cc: Daniel Vetter, Colin Cross, John Stultz, devel, Jesse Barnes This allows users of dma fences to create a android fence. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/staging/android/sync.c | 24 ++++++++++++++++++++---- drivers/staging/android/sync.h | 11 +++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index 69139ce7420d..c9331250ac26 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb) } /* TODO: implement a create which takes more that one sync_pt */ -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) +static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt) { struct sync_fence *fence; @@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) fence->num_fences = 1; atomic_set(&fence->status, 1); - fence->cbs[0].sync_pt = &pt->base; + fence->cbs[0].sync_pt = pt; fence->cbs[0].fence = fence; - if (fence_add_callback(&pt->base, &fence->cbs[0].cb, - fence_check_cb_func)) + if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func)) atomic_dec(&fence->status); sync_fence_debug_add(fence); return fence; } + +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt) +{ + struct sync_fence *fence; + + fence = sync_fence_create_noref(name, fence_get(pt)); + if (!fence) + fence_put(pt); + + return fence; +} +EXPORT_SYMBOL(sync_fence_create_dma); + +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) +{ + return sync_fence_create_noref(name, &pt->base); +} EXPORT_SYMBOL(sync_fence_create); struct sync_fence *sync_fence_fdget(int fd) diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h index 66b0f431f63e..7b3bf560790c 100644 --- a/drivers/staging/android/sync.h +++ b/drivers/staging/android/sync.h @@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt); */ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); +/** + * sync_fence_create_dma() - creates a sync fence from a dma fence + * @name: name of fence to create + * @pt: dma fence to add to the sync fence + * + * Creates a fence containg @pt. Once this is called, the fence takes + * a reference on @pt, unlike sync_fence_create which doesn't add one. + */ +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt); + + /* * API for sync_fence consumers */ -- 2.0.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-14 9:54 ` [PATCH 2/2] android: add sync_fence_create_dma Maarten Lankhorst @ 2014-08-14 20:09 ` Jesse Barnes 2014-08-15 6:46 ` Greg Kroah-Hartman 1 sibling, 0 replies; 11+ messages in thread From: Jesse Barnes @ 2014-08-14 20:09 UTC (permalink / raw) To: Maarten Lankhorst Cc: Sumit Semwal, Greg Kroah-Hartman, linaro-mm-sig, LKML, Daniel Vetter, Colin Cross, John Stultz, devel On Thu, 14 Aug 2014 11:54:52 +0200 Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > This allows users of dma fences to create a android fence. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/staging/android/sync.c | 24 ++++++++++++++++++++---- > drivers/staging/android/sync.h | 11 +++++++++++ > 2 files changed, 31 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > index 69139ce7420d..c9331250ac26 100644 > --- a/drivers/staging/android/sync.c > +++ b/drivers/staging/android/sync.c > @@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb) > } > > /* TODO: implement a create which takes more that one sync_pt */ > -struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) > +static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt) > { > struct sync_fence *fence; > > @@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) > fence->num_fences = 1; > atomic_set(&fence->status, 1); > > - fence->cbs[0].sync_pt = &pt->base; > + fence->cbs[0].sync_pt = pt; > fence->cbs[0].fence = fence; > - if (fence_add_callback(&pt->base, &fence->cbs[0].cb, > - fence_check_cb_func)) > + if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func)) > atomic_dec(&fence->status); > > sync_fence_debug_add(fence); > > return fence; > } > + > +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt) > +{ > + struct sync_fence *fence; I ran into the same naming trouble in my implementation; I think I ended up with sfence for sync fence declarations. > + > + fence = sync_fence_create_noref(name, fence_get(pt)); > + if (!fence) > + fence_put(pt); > + > + return fence; > +} > +EXPORT_SYMBOL(sync_fence_create_dma); > + > +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) > +{ > + return sync_fence_create_noref(name, &pt->base); > +} > EXPORT_SYMBOL(sync_fence_create); > > struct sync_fence *sync_fence_fdget(int fd) > diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h > index 66b0f431f63e..7b3bf560790c 100644 > --- a/drivers/staging/android/sync.h > +++ b/drivers/staging/android/sync.h > @@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt); > */ > struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); > > +/** > + * sync_fence_create_dma() - creates a sync fence from a dma fence > + * @name: name of fence to create > + * @pt: dma fence to add to the sync fence > + * > + * Creates a fence containg @pt. Once this is called, the fence takes > + * a reference on @pt, unlike sync_fence_create which doesn't add one. > + */ > +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt); > + > + > /* > * API for sync_fence consumers > */ Yeah, I've been using this, looks good. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-14 9:54 ` [PATCH 2/2] android: add sync_fence_create_dma Maarten Lankhorst 2014-08-14 20:09 ` Jesse Barnes @ 2014-08-15 6:46 ` Greg Kroah-Hartman 2014-08-15 16:23 ` Jesse Barnes 2014-08-28 6:54 ` Maarten Lankhorst 1 sibling, 2 replies; 11+ messages in thread From: Greg Kroah-Hartman @ 2014-08-15 6:46 UTC (permalink / raw) To: Maarten Lankhorst Cc: Sumit Semwal, linaro-mm-sig, LKML, Daniel Vetter, Colin Cross, John Stultz, devel, Jesse Barnes On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: > This allows users of dma fences to create a android fence. Who is going to use these functions? I need an in-kernel user before I can add new api calls. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-15 6:46 ` Greg Kroah-Hartman @ 2014-08-15 16:23 ` Jesse Barnes 2014-08-28 6:54 ` Maarten Lankhorst 1 sibling, 0 replies; 11+ messages in thread From: Jesse Barnes @ 2014-08-15 16:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Maarten Lankhorst, Sumit Semwal, linaro-mm-sig, LKML, Daniel Vetter, Colin Cross, John Stultz, devel On Fri, 15 Aug 2014 14:46:56 +0800 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: > > This allows users of dma fences to create a android fence. > > Who is going to use these functions? I need an in-kernel user before I > can add new api calls. I have some code that uses them, but I need a userspace user to push my stuff. :) I'm working on the latter bit too in Mesa, and we have demand from Android, so we should get some users shortly. -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-15 6:46 ` Greg Kroah-Hartman 2014-08-15 16:23 ` Jesse Barnes @ 2014-08-28 6:54 ` Maarten Lankhorst 2014-08-28 11:57 ` Dan Carpenter 1 sibling, 1 reply; 11+ messages in thread From: Maarten Lankhorst @ 2014-08-28 6:54 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Sumit Semwal, linaro-mm-sig, LKML, Daniel Vetter, Colin Cross, John Stultz, devel, Jesse Barnes Hey, On 15-08-14 08:46, Greg Kroah-Hartman wrote: > On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: >> This allows users of dma fences to create a android fence. > > Who is going to use these functions? I need an in-kernel user before I > can add new api calls. So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. Will you apply these patches? ~Maarten ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-28 6:54 ` Maarten Lankhorst @ 2014-08-28 11:57 ` Dan Carpenter 2014-09-01 12:33 ` Maarten Lankhorst 0 siblings, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2014-08-28 11:57 UTC (permalink / raw) To: Maarten Lankhorst Cc: Greg Kroah-Hartman, devel, Daniel Vetter, LKML, Colin Cross, linaro-mm-sig, John Stultz, Jesse Barnes, Sumit Semwal On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote: > Hey, > > On 15-08-14 08:46, Greg Kroah-Hartman wrote: > > On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: > >> This allows users of dma fences to create a android fence. > > > > Who is going to use these functions? I need an in-kernel user before I > > can add new api calls. > > So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. > Will you apply these patches? Can you resend the patches? Fix the changelog of the first one to mention that it is a bugfix. Send a [patch 3/3] which uses the new functions in [patch 2/3]. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-08-28 11:57 ` Dan Carpenter @ 2014-09-01 12:33 ` Maarten Lankhorst 2014-09-01 12:45 ` Dan Carpenter 0 siblings, 1 reply; 11+ messages in thread From: Maarten Lankhorst @ 2014-09-01 12:33 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, devel, Daniel Vetter, LKML, Colin Cross, linaro-mm-sig, John Stultz, Jesse Barnes, Sumit Semwal Hey, Op 28-08-14 om 13:57 schreef Dan Carpenter: > On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote: >> Hey, >> >> On 15-08-14 08:46, Greg Kroah-Hartman wrote: >>> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: >>>> This allows users of dma fences to create a android fence. >>> Who is going to use these functions? I need an in-kernel user before I >>> can add new api calls. >> So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. >> Will you apply these patches? > Can you resend the patches? Fix the changelog of the first one to > mention that it is a bugfix. Send a [patch 3/3] which uses the new > functions in [patch 2/3]. The second patch will have to be applied without an in-kernel user because it will be used in the drm subsystem, by someone other than me. Their code is not ready yet, but will likely will be for the 3.18 merge window. ~Maarten ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] android: add sync_fence_create_dma 2014-09-01 12:33 ` Maarten Lankhorst @ 2014-09-01 12:45 ` Dan Carpenter 0 siblings, 0 replies; 11+ messages in thread From: Dan Carpenter @ 2014-09-01 12:45 UTC (permalink / raw) To: Maarten Lankhorst Cc: devel, Greg Kroah-Hartman, LKML, Colin Cross, linaro-mm-sig, John Stultz, Jesse Barnes, Daniel Vetter, Sumit Semwal On Mon, Sep 01, 2014 at 02:33:59PM +0200, Maarten Lankhorst wrote: > Hey, > > Op 28-08-14 om 13:57 schreef Dan Carpenter: > > On Thu, Aug 28, 2014 at 08:54:05AM +0200, Maarten Lankhorst wrote: > >> Hey, > >> > >> On 15-08-14 08:46, Greg Kroah-Hartman wrote: > >>> On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote: > >>>> This allows users of dma fences to create a android fence. > >>> Who is going to use these functions? I need an in-kernel user before I > >>> can add new api calls. > >> So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. > >> Will you apply these patches? > > Can you resend the patches? Fix the changelog of the first one to > > mention that it is a bugfix. Send a [patch 3/3] which uses the new > > functions in [patch 2/3]. > The second patch will have to be applied without an in-kernel user because it > will be used in the drm subsystem, by someone other than me. Their code is not > ready yet, but will likely will be for the 3.18 merge window. Let's just wait until the user is ready. It might be easiest if they push your patch? Anyway, please resend the first patch so we can apply that right away. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] android: fix reference leak in sync_fence_create 2014-08-14 9:53 [PATCH 1/2] android: fix reference leak in sync_fence_create Maarten Lankhorst 2014-08-14 9:54 ` [PATCH 2/2] android: add sync_fence_create_dma Maarten Lankhorst @ 2014-08-18 12:57 ` Dan Carpenter 2014-08-18 13:06 ` Maarten Lankhorst 1 sibling, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2014-08-18 12:57 UTC (permalink / raw) To: Maarten Lankhorst Cc: Sumit Semwal, Greg Kroah-Hartman, linaro-mm-sig, LKML, devel, Daniel Vetter, John Stultz, Colin Cross On Thu, Aug 14, 2014 at 11:53:38AM +0200, Maarten Lankhorst wrote: > According to the documentation sync_fence_create takes ownership of the point, > not a reference on the point. > What are the user visible effects of this bug? I assume this is a real bug but judging solely based on your patch description, it sounds like you could just update the documentation instead of changing the code. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] android: fix reference leak in sync_fence_create 2014-08-18 12:57 ` [PATCH 1/2] android: fix reference leak in sync_fence_create Dan Carpenter @ 2014-08-18 13:06 ` Maarten Lankhorst 0 siblings, 0 replies; 11+ messages in thread From: Maarten Lankhorst @ 2014-08-18 13:06 UTC (permalink / raw) To: Dan Carpenter Cc: Sumit Semwal, Greg Kroah-Hartman, linaro-mm-sig, LKML, devel, Daniel Vetter, John Stultz, Colin Cross Hey, Op 18-08-14 om 14:57 schreef Dan Carpenter: > On Thu, Aug 14, 2014 at 11:53:38AM +0200, Maarten Lankhorst wrote: >> According to the documentation sync_fence_create takes ownership of the point, >> not a reference on the point. >> > What are the user visible effects of this bug? I assume this is a real > bug but judging solely based on your patch description, it sounds like > you could just update the documentation instead of changing the code. > Small memory leak on every created android fence when you run out of tree android drivers. But because it happens every frame (or possibly even more often) it's worth fixing. ~Maarten ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-09-01 12:45 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-14 9:53 [PATCH 1/2] android: fix reference leak in sync_fence_create Maarten Lankhorst 2014-08-14 9:54 ` [PATCH 2/2] android: add sync_fence_create_dma Maarten Lankhorst 2014-08-14 20:09 ` Jesse Barnes 2014-08-15 6:46 ` Greg Kroah-Hartman 2014-08-15 16:23 ` Jesse Barnes 2014-08-28 6:54 ` Maarten Lankhorst 2014-08-28 11:57 ` Dan Carpenter 2014-09-01 12:33 ` Maarten Lankhorst 2014-09-01 12:45 ` Dan Carpenter 2014-08-18 12:57 ` [PATCH 1/2] android: fix reference leak in sync_fence_create Dan Carpenter 2014-08-18 13:06 ` Maarten Lankhorst
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).