| Message ID | 1448278471-31181-3-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 23-11-15 om 12:34 schreef John.C.Harrison@Intel.com: > From: Maarten Lankhorst <maarten.lankhorst@canonical.com> > > This allows users of dma fences to create a android fence. > > v2: Added kerneldoc. (Tvrtko Ursulin). > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: devel@driverdev.osuosl.org > Cc: Riley Andrews <riandrews@android.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Arve Hjønnevåg <arve@android.com> > --- > drivers/staging/android/sync.c | 13 +++++++++---- > drivers/staging/android/sync.h | 12 +++++++++++- > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > index f83e00c..7f0e919 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) > +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt) > { > struct sync_fence *fence; > > @@ -199,16 +199,21 @@ 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; > } > +EXPORT_SYMBOL(sync_fence_create_dma); > + > +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) > +{ > + return sync_fence_create_dma(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 61f8a3a..798cd56 100644 > --- a/drivers/staging/android/sync.h > +++ b/drivers/staging/android/sync.h > @@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt); > * @pt: sync_pt to add to the fence > * > * Creates a fence containg @pt. Once this is called, the fence takes > - * ownership of @pt. > + * a reference on @pt. > */ > struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); No it doesn't. > +/** > + * sync_fence_create_dma() - creates a sync fence from dma-fence > + * @name: name of fence to create > + * @pt: dma-fence to add to the fence > + * > + * Creates a fence containg @pt. Once this is called, the fence takes > + * a reference on @pt. > + */ No it doesn't. > +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt); > + > /* > * API for sync_fence consumers > */
On 23/11/2015 13:27, Maarten Lankhorst wrote: > Op 23-11-15 om 12:34 schreef John.C.Harrison@Intel.com: >> From: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> >> This allows users of dma fences to create a android fence. >> >> v2: Added kerneldoc. (Tvrtko Ursulin). >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >> Cc: devel@driverdev.osuosl.org >> Cc: Riley Andrews <riandrews@android.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Arve Hjønnevåg <arve@android.com> >> --- >> drivers/staging/android/sync.c | 13 +++++++++---- >> drivers/staging/android/sync.h | 12 +++++++++++- >> 2 files changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c >> index f83e00c..7f0e919 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) >> +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt) >> { >> struct sync_fence *fence; >> >> @@ -199,16 +199,21 @@ 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; >> } >> +EXPORT_SYMBOL(sync_fence_create_dma); >> + >> +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) >> +{ >> + return sync_fence_create_dma(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 61f8a3a..798cd56 100644 >> --- a/drivers/staging/android/sync.h >> +++ b/drivers/staging/android/sync.h >> @@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt); >> * @pt: sync_pt to add to the fence >> * >> * Creates a fence containg @pt. Once this is called, the fence takes >> - * ownership of @pt. >> + * a reference on @pt. >> */ >> struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); > No it doesn't. >> +/** >> + * sync_fence_create_dma() - creates a sync fence from dma-fence >> + * @name: name of fence to create >> + * @pt: dma-fence to add to the fence >> + * >> + * Creates a fence containg @pt. Once this is called, the fence takes >> + * a reference on @pt. >> + */ > No it doesn't. This is your patch isn't it? Or is this something Tvrtko added on the way past? Either way, what should the correct description be? It takes a copy of the pointer to 'pt'. Is the comment meaning a reference in the sense of 'pass by reference', i.e. a pointer? But you are meaning a reference in the sense of incrementing a usage count? >> +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt); >> + >> /* >> * API for sync_fence consumers >> */
On 23/11/15 13:38, John Harrison wrote: > On 23/11/2015 13:27, Maarten Lankhorst wrote: >> Op 23-11-15 om 12:34 schreef John.C.Harrison@Intel.com: >>> From: Maarten Lankhorst <maarten.lankhorst@canonical.com> >>> >>> This allows users of dma fences to create a android fence. >>> >>> v2: Added kerneldoc. (Tvrtko Ursulin). >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >>> Cc: devel@driverdev.osuosl.org >>> Cc: Riley Andrews <riandrews@android.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: Arve Hjønnevåg <arve@android.com> >>> --- >>> drivers/staging/android/sync.c | 13 +++++++++---- >>> drivers/staging/android/sync.h | 12 +++++++++++- >>> 2 files changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/staging/android/sync.c >>> b/drivers/staging/android/sync.c >>> index f83e00c..7f0e919 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) >>> +struct sync_fence *sync_fence_create_dma(const char *name, struct >>> fence *pt) >>> { >>> struct sync_fence *fence; >>> @@ -199,16 +199,21 @@ 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; >>> } >>> +EXPORT_SYMBOL(sync_fence_create_dma); >>> + >>> +struct sync_fence *sync_fence_create(const char *name, struct >>> sync_pt *pt) >>> +{ >>> + return sync_fence_create_dma(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 61f8a3a..798cd56 100644 >>> --- a/drivers/staging/android/sync.h >>> +++ b/drivers/staging/android/sync.h >>> @@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt); >>> * @pt: sync_pt to add to the fence >>> * >>> * Creates a fence containg @pt. Once this is called, the fence takes >>> - * ownership of @pt. >>> + * a reference on @pt. >>> */ >>> struct sync_fence *sync_fence_create(const char *name, struct >>> sync_pt *pt); >> No it doesn't. >>> +/** >>> + * sync_fence_create_dma() - creates a sync fence from dma-fence >>> + * @name: name of fence to create >>> + * @pt: dma-fence to add to the fence >>> + * >>> + * Creates a fence containg @pt. Once this is called, the fence takes >>> + * a reference on @pt. >>> + */ >> No it doesn't. > This is your patch isn't it? Or is this something Tvrtko added on the > way past? Either way, what should the correct description be? It takes a It is Maarten's patch, I just copied the comment over from one function to another. The one I picked up was https://patchwork.freedesktop.org/patch/38074/, December 2014. > copy of the pointer to 'pt'. Is the comment meaning a reference in the > sense of 'pass by reference', i.e. a pointer? But you are meaning a > reference in the sense of incrementing a usage count? Maybe he means it is still taking ownership, not reference? Regards, Tvrtko
Op 23-11-15 om 14:44 schreef Tvrtko Ursulin: > > On 23/11/15 13:38, John Harrison wrote: >> On 23/11/2015 13:27, Maarten Lankhorst wrote: >>> Op 23-11-15 om 12:34 schreef John.C.Harrison@Intel.com: >>>> From: Maarten Lankhorst <maarten.lankhorst@canonical.com> >>>> >>>> This allows users of dma fences to create a android fence. >>>> >>>> v2: Added kerneldoc. (Tvrtko Ursulin). >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >>>> Cc: devel@driverdev.osuosl.org >>>> Cc: Riley Andrews <riandrews@android.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: Arve Hjønnevåg <arve@android.com> >>>> --- >>>> drivers/staging/android/sync.c | 13 +++++++++---- >>>> drivers/staging/android/sync.h | 12 +++++++++++- >>>> 2 files changed, 20 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/staging/android/sync.c >>>> b/drivers/staging/android/sync.c >>>> index f83e00c..7f0e919 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) >>>> +struct sync_fence *sync_fence_create_dma(const char *name, struct >>>> fence *pt) >>>> { >>>> struct sync_fence *fence; >>>> @@ -199,16 +199,21 @@ 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; >>>> } >>>> +EXPORT_SYMBOL(sync_fence_create_dma); >>>> + >>>> +struct sync_fence *sync_fence_create(const char *name, struct >>>> sync_pt *pt) >>>> +{ >>>> + return sync_fence_create_dma(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 61f8a3a..798cd56 100644 >>>> --- a/drivers/staging/android/sync.h >>>> +++ b/drivers/staging/android/sync.h >>>> @@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt); >>>> * @pt: sync_pt to add to the fence >>>> * >>>> * Creates a fence containg @pt. Once this is called, the fence takes >>>> - * ownership of @pt. >>>> + * a reference on @pt. >>>> */ >>>> struct sync_fence *sync_fence_create(const char *name, struct >>>> sync_pt *pt); >>> No it doesn't. >>>> +/** >>>> + * sync_fence_create_dma() - creates a sync fence from dma-fence >>>> + * @name: name of fence to create >>>> + * @pt: dma-fence to add to the fence >>>> + * >>>> + * Creates a fence containg @pt. Once this is called, the fence takes >>>> + * a reference on @pt. >>>> + */ >>> No it doesn't. >> This is your patch isn't it? Or is this something Tvrtko added on the >> way past? Either way, what should the correct description be? It takes a > > It is Maarten's patch, I just copied the comment over from one function to another. The one I picked up was https://patchwork.freedesktop.org/patch/38074/, December 2014. > >> copy of the pointer to 'pt'. Is the comment meaning a reference in the >> sense of 'pass by reference', i.e. a pointer? But you are meaning a >> reference in the sense of incrementing a usage count? > > Maybe he means it is still taking ownership, not reference? Indeed. See commit 3ea411c56ef "android: fix reference leak in sync_fence_create". This broke compatibility with android by creating a ref leak. :)
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index f83e00c..7f0e919 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) +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt) { struct sync_fence *fence; @@ -199,16 +199,21 @@ 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; } +EXPORT_SYMBOL(sync_fence_create_dma); + +struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) +{ + return sync_fence_create_dma(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 61f8a3a..798cd56 100644 --- a/drivers/staging/android/sync.h +++ b/drivers/staging/android/sync.h @@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt); * @pt: sync_pt to add to the fence * * Creates a fence containg @pt. Once this is called, the fence takes - * ownership of @pt. + * a reference on @pt. */ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt); +/** + * sync_fence_create_dma() - creates a sync fence from dma-fence + * @name: name of fence to create + * @pt: dma-fence to add to the fence + * + * Creates a fence containg @pt. Once this is called, the fence takes + * a reference on @pt. + */ +struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt); + /* * API for sync_fence consumers */