Re: [RFC 02/12] staging/android/sync: add sync_fence_create_dma

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 23/11/2015 13:27, Maarten Lankhorst wrote:
> Op 23-11-15 om 12:34 schreef John.C.Harrison@xxxxxxxxx:
> > From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx>
> >
> > This allows users of dma fences to create a android fence.
> >
> > v2: Added kerneldoc. (Tvrtko Ursulin).
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx>
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> > Cc: devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Riley Andrews <riandrews@xxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> > ---
> >   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
> >    */

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel