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

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

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


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