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

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

[Michal Wajdeczko]

Additionally use runtime check to catch invalid engine indices.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Jani Nikula <jani.nikula@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);
+	GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES);
 	GEM_BUG_ON(dev_priv->engine[id]);
 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
 	if (!engine)
-- 
2.7.4

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

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Joonas Lahtinen]

On ti, 2017-02-28 at 14:00 +0000, Michal Wajdeczko wrote:
> Additionally use runtime check to catch invalid engine indices.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Chris Wilson]

On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote:
> Additionally use runtime check to catch invalid engine indices.

Oh no you don't!
 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@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);
> +	GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES);

Are you sure sparse/smatch won't complain?
/me too lazy to check the C standard for signedness of an enum without a
negative value.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Chris Wilson]

On Tue, Feb 28, 2017 at 02:12:59PM +0000, Chris Wilson wrote:
> On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote:
> > Additionally use runtime check to catch invalid engine indices.
> 
> Oh no you don't!

Parse error on my part, GEM_BUG_ON didn't register as runtime.
Conditionally runtime!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Michal Wajdeczko]

On Tue, Feb 28, 2017 at 02:12:59PM +0000, Chris Wilson wrote:
> On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote:
> > Additionally use runtime check to catch invalid engine indices.
> 
> Oh no you don't!

We can skip it, but today there is no way to verify that our enums fits
into [0..I915_NUM_ENGINES) range.

Additionally, you've moved definition of the I915_NUM_ENGINES to
separate file far away from enum intel_engine_id definition.


>  
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Jani Nikula <jani.nikula@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);
> > +	GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES);
> 
> Are you sure sparse/smatch won't complain?
> /me too lazy to check the C standard for signedness of an enum without a
> negative value.

enums are int's and as such can be negative ;)
see http://en.cppreference.com/w/c/language/enum

-Michal

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

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Tvrtko Ursulin]

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@intel.com>
> Cc: Jani Nikula <jani.nikula@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. ;)

> +	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));

?

>  	GEM_BUG_ON(dev_priv->engine[id]);
>  	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
>  	if (!engine)
>

Regards,

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

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Joonas Lahtinen]

On ti, 2017-02-28 at 14:21 +0000, Tvrtko Ursulin wrote:
> 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));

I think that's even better.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@intel.com>
> > Cc: Jani Nikula <jani.nikula@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Chris Wilson]

On Tue, Feb 28, 2017 at 03:52:31PM +0100, Michal Wajdeczko wrote:
> 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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@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. 

But is pointless if it doesn't apply to gen+1, or tomorrow's packing of
sparse engines, which is where Tvrtko is coming from.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Michal Wajdeczko]

On Tue, Feb 28, 2017 at 03:08:19PM +0000, Chris Wilson wrote:
> On Tue, Feb 28, 2017 at 03:52:31PM +0100, Michal Wajdeczko wrote:
> > 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@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@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. 
> 
> But is pointless if it doesn't apply to gen+1, or tomorrow's packing of
> sparse engines, which is where Tvrtko is coming from.

But it applies.

It shall still work as each ring bit used by HAS_ENGINE() will likely
still represent single entry in dev_priv->engine[].

As design assumes strong relation between intel_engines[] and dev_priv->engine[]
arrays, we should have some way to validate correctness of that assumption.

Note that this check should help us catch any mistakes that Tvrtko introduces ;)

Unless, there will be full redesign.

-Michal


ps. I'm coming from almost the same place as Tvrtko ;)

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

✗ Fi.CI.BAT: failure for drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Patchwork]

== Series Details ==

Series: drm/i915: Use BUILD_BUG_ON to detected missing engine definitions
URL   : https://patchwork.freedesktop.org/series/20394/
State : failure

== Summary ==

Series 20394v1 drm/i915: Use BUILD_BUG_ON to detected missing engine definitions
https://patchwork.freedesktop.org/api/1.0/series/20394/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup gtt-vebox:
                skip       -> INCOMPLETE (fi-byt-j1900)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:36   pass:30   dwarn:0   dfail:0   fail:0   skip:5  
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

5d37006b578e38562382215e8782cfced9c992ce drm-tip: 2017y-02m-28d-16h-27m-13s UTC integration manifest
ad75ffb drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4004/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx