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

From: John Harrison <John.C.Harrison@Intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Intel-GFX@Lists.FreeDesktop.Org
Cc: devel@driverdev.osuosl.org,
	"Tvrtko Ursulin" <tvrtko.ursulin@intel.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Jesse Barnes" <jbarnes@virtuousgeek.org>,
	"Riley Andrews" <riandrews@android.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Maarten Lankhorst" <maarten.lankhorst@canonical.com>
Subject: Re: [RFC 02/12] staging/android/sync: add sync_fence_create_dma
Date: Mon, 23 Nov 2015 13:38:32 +0000	[thread overview]
Message-ID: <565316D8.8010101@Intel.com> (raw)
In-Reply-To: <5653142A.8090606@linux.intel.com>

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
>>    */

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