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

Tvrtko Ursulin
2016-07-21T09:36:10Z

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