[Intel-gfx] [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Tvrtko Ursulin]

On 28/02/2017 14:52, Michal Wajdeczko wrote:
> On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote:

[snip]

>>> +	GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES);
>>
>> The caller of this function iterates 0..ARRAY_SIZE(intel_engines) and also
>> filters with HAS_ENGINE before calling it so not sure this is absolutely
>> needed. Maybe instead:
>>
>> GEM_BUG_ON(id >= ARRAY_SIZE(dev_priv->engine));
>>
>
> With your approach we could drop GEM_BUG_ON completely as with correct
> iteration we should never hit condition id > ARRAY_SIZE.
 >
 > If we could assume that everyone is doing right, then we should never
 > need any asserts at all.

That is not correct. I suggested the function should just check the size 
of the array it is concerned with, rather than assuming how 
I915_NUM_ENGINES relates to the same array.

> Problem is that this function does not know anything about the caller.
> And also it does not know if enums were defined correctly.
> But then it uses these enums as index into two external arrays.
> In my opition we should do our best to catch any inproper usage/definitions.
> If not everywhere, then at least once during build or boot.

Agreed, but intel_engine_setup does not care about the enum so much. It 
cares that it doesn't do out of bounds access to the two arrays. In the 
light of that, GEM_BUG_ON(id >= ARRAY_SIZE(intel_engines) || id >= 
ARRAY_SIZE(dev_priv->engines)) sounds like the most robust solution to me.

Since the function handles failure as well, perhaps we could even 
upgrade that to a WARN_ON and return -EINVAL. Not sure.

Regards,

Tvrtko