Re: [PATCH v3] drm/i915: Small compaction of the engine init code

Tvrtko Ursulin
2016-06-23T13:17:00Z

On 23/06/16 13:11, Chris Wilson wrote:
> On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
>>
>> On 23/06/16 12:25, Chris Wilson wrote:
>>> On Thu, Jun 23, 2016 at 12:12:29PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Effectively removes one layer of indirection between the mask of
>>>> possible engines and the engine constructors. Instead of spelling
>>>> out in code the mapping of HAS_<engine> to constructors, makes
>>>> more use of the recently added data driven approach by putting
>>>> engine constructor vfuncs into the table as well.
>>>>
>>>> Effect is fewer lines of source and smaller binary.
>>>>
>>>> At the same time simplify the error handling since engine
>>>> destructors can run on unitialized engines anyway.
>>>>
>>>> Similar approach could be done for legacy submission is wanted.
>>>>
>>>> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
>>>> ENGINE_MASK and HAS_ENGINE macros.
>>>> Also removed the forward declarations by shuffling functions
>>>> around.
>>>>
>>>> v3: Warn when logical_rings table does not contain enough data
>>>> and disable the engines which could not be initialized.
>>>> (Chris Wilson)
>>>
>>> I was happy with the BUILD_BUG suggestion :)
>>
>> I've changed my mind later. :)
>>
>>>> + for (i = 0;
>>>> + i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
>>>
>>> HAS_ENGINE() == false if i >= I915_NUM_ENGINES
>>
>> Don't follow. :) Why is v3 not good enough?
>
> Both (all three) is overkill.
>
> I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
> succinctly. For belt and braces,
>
> WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));

I don't think this works - you meant testing that bits higher than
BIT(I915_NUM_ENGINES) were not set in ring_mask?

And it probably belongs somewhere else, in common code which initializes
intel_device_info I think.

> for (i = 0; i < ARRAY_SIZE(); i++) {
> if (!HAS_ENGINE(i))
> continue;
>
> if (!logical_rings[i].init)
> continue;
>
> ret = logical_rings[i].init(logical_rings_engine(i));
> if (ret)
> goto err;
>
> mask |= ENGINE_MASK(i);
> }
>
> WARN_ON(mask != dev_priv->intel_info.rings_mask) ...
>
> ?

I like it, will resend when we clarify the above. Also solves one more
issue than the previous versions which is a potentially sparse
logical_rings array if engine ids get renumbered.

Tvrtko