[RFC PATCH v2 06/17] gpu: host1x: Cleanup and refcounting for syncpoints

[Mikko Perttunen]

On 9/5/20 5:30 PM, Dmitry Osipenko wrote:
> 05.09.2020 13:34, Mikko Perttunen пишет:
> ...
>> +
>> +/**
>> + * host1x_syncpt_put() - free a requested syncpoint
>> + * @sp: host1x syncpoint
>> + *
>> + * Release a syncpoint previously allocated using host1x_syncpt_request(). A
>> + * host1x client driver should call this when the syncpoint is no longer in
>> + * use.
>> + */
>> +void host1x_syncpt_put(struct host1x_syncpt *sp)
>> +{
>> +	if (!sp)
>> +		return;
>> +
>> +	kref_put(&sp->ref, syncpt_release);
>> +}
>> +EXPORT_SYMBOL(host1x_syncpt_put);
>>   
>>   void host1x_syncpt_deinit(struct host1x *host)
>>   {
>> @@ -471,16 +478,48 @@ unsigned int host1x_syncpt_nb_mlocks(struct host1x *host)
>>   }
>>   
>>   /**
>> - * host1x_syncpt_get() - obtain a syncpoint by ID
>> + * host1x_syncpt_get_by_id() - obtain a syncpoint by ID
>> + * @host: host1x controller
>> + * @id: syncpoint ID
>> + */
>> +struct host1x_syncpt *host1x_syncpt_get_by_id(struct host1x *host,
>> +					      unsigned int id)
>> +{
>> +	if (id >= host->info->nb_pts)
>> +		return NULL;
>> +
>> +	if (kref_get_unless_zero(&host->syncpt[id].ref))
>> +		return &host->syncpt[id];
>> +	else
>> +		return NULL;
>> +}
>> +EXPORT_SYMBOL(host1x_syncpt_get_by_id);
>> +
>> +/**
>> + * host1x_syncpt_get_by_id_noref() - obtain a syncpoint by ID but don't
>> + * 	increase the refcount.
>>    * @host: host1x controller
>>    * @id: syncpoint ID
>>    */
>> -struct host1x_syncpt *host1x_syncpt_get(struct host1x *host, unsigned int id)
>> +struct host1x_syncpt *host1x_syncpt_get_by_id_noref(struct host1x *host,
>> +						    unsigned int id)
>>   {
>>   	if (id >= host->info->nb_pts)
>>   		return NULL;
>>   
>> -	return host->syncpt + id;
>> +	return &host->syncpt[id];
>> +}
>> +EXPORT_SYMBOL(host1x_syncpt_get_by_id_noref);
>> +
>> +/**
>> + * host1x_syncpt_get() - increment syncpoint refcount
>> + * @sp: syncpoint
>> + */
>> +struct host1x_syncpt *host1x_syncpt_get(struct host1x_syncpt *sp)
>> +{
>> +	kref_get(&sp->ref);
>> +
>> +	return sp;
>>   }
>>   EXPORT_SYMBOL(host1x_syncpt_get);
> 
> Hello, Mikko!
> 
> What do you think about to open-code all the host1x structs by moving
> them all out into the public linux/host1x.h? Then we could inline all
> these trivial single-line functions by having them defined in the public
> header. This will avoid all the unnecessary overhead by allowing
> compiler to optimize the code nicely.
> 
> Of course this could be a separate change and it could be done sometime
> later, I just wanted to share this quick thought for the start of the
> review.
> 

Hi :)

I think for such micro-optimizations we should have a benchmark to 
evaluate against. I'm not sure we have all that many function calls into 
here overall that it would make a noticeable difference. In any case, as 
you said, I'd prefer to keep further refactoring to a separate series to 
avoid growing this series too much.

Mikko