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,
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 */
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
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
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.
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
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
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
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
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
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
linaro-mm-sig@lists.linaro.org
-
Dan Carpenter -
Greg Kroah-Hartman -
Jesse Barnes -
Maarten Lankhorst