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

[Michal Wajdeczko]

On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote:
> 
> On 28/02/2017 14:00, Michal Wajdeczko wrote:
> > Additionally use runtime check to catch invalid engine indices.
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a238304..8df53ae 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >  	const struct engine_info *info = &intel_engines[id];
> >  	struct intel_engine_cs *engine;
> > 
> > +	BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES);
> 
> For some reason I feel this is too strict. ;)

It has to be strict to be useful. 


> 
> > +	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.

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.

-Michal