Re: [PATCH 3/3] drm/i915: rename & update eb_select_ring()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 20/07/16 18:31, Chris Wilson wrote:
> On Wed, Jul 20, 2016 at 06:16:07PM +0100, Dave Gordon wrote:
>> 'ring' is an old deprecated term for a GPU engine, so we're trying to
>> phase out all such terminology. eb_select_ring() not only has 'ring'
>> (meaning engine) in its name, but it has an ugly calling convention
>> whereby it returns an errno and stores a pointer-to-engine indirectly
>> through an output parameter. As there is only one error it ever returns
>> (-EINVAL), we can make it return the pointer directly, and have the
>> caller pass back the error code -EINVAL if the pointer result is NULL.
>>
>> Thus we can replace
>> -	ret = eb_select_ring(dev_priv, file, args, &engine);
>> -	if (ret)
>> -		return ret;
>> with
>> +	engine = eb_select_engine(dev_priv, file, args);
>> +	if (!engine)
>> +		return -EINVAL;
>> for increased clarity and maybe save a few cycles too.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>
> Thanks. I feel foolish for missing that easy transform before.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> There's a bsd_ring buried beneath here as well...

The ugly calling convention was my doing in:

   commit de1add360522c876c25ef2bbbbab1c94bdb509ab
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Fri Jan 15 15:12:50 2016 +0000

       drm/i915: Decouple execbuf uAPI from internal implementation

And the reason for it was to avoid growing the text size of 
i915_gem_do_execbuffer while at the same time extracting all the engine 
selection logic into a separate function. For some reason GCC most liked 
it like that. Or maybe I was only trying the ERR_PTR route, not the 
NULL/ptr return. Don't remember now.

Anyway, I don't mind, just providing reasoning for the "ugly" calling 
convention.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx