[Intel-gfx] Error in inner loop in validate_cmds_sorted / out of bounds issue

[Chris Wilson]

On Tue, Jul 28, 2015 at 11:14:19AM -0700, Hanno Böck wrote:
> Hi,
> 
> On Tue, 28 Jul 2015 10:14:51 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > > Indeed, nice catch. Could you please read
> > > Documentation/SubmittingPatches and apply your Signed-off-by and
> > > then we can accept this patch under your authorship.
> > > 
> > > Preferrably this is two patches, (a) fix the tables, (b) fix the
> > > validator. That way we can delay enabling the validator if we need
> > > to fix the tables for others.
> 
> I think I have checked all tables, not just the ones used on my gpu,
> they should be fine. But I've splittet the patch.
> 
> > Also can you please add signed-off-by lines to your patch when
> > resubmitting? See Documentation/SubmittingPatches for all the details.
> 
> The patch already had a "Signed-off-by" line.
> 
> The checkpatch script complains that it doesn't like the formatting of
> the CMD command. However I won't change that in this patch, as this is
> how the whole file is formatted. If this is wanted I can submit a patch
> changing the formatting afterwards, but I think this is an unrelated
> change.
> 
> Please apply.
> 
> -- 
> Hanno Böck
> http://hboeck.de/
> 
> mail/jabber: hanno at hboeck.de
> GPG: BBB51E42

> Properly sort cmd tables.

drm/i915: Properly sort MI coomand table

In the future, we may want to speed up command/register searching using
a bisection and so we require them to be in ascending order respectively
by command value or register address. However, this was not true for one
pair in the MI table; make it so.
 
> Signed-off-by: Hanno Boeck <hanno at hboeck.de>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..5fdd8c8 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -151,8 +151,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
>  	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> -	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,

> Fix loop checking cmd tables.

drm/i915: Fix command parser table validator

As we may like to use a bisection search on the tables in future, we
need them to be ordered. For convenience we expect the compiled tables
to be order and check on initialisation. However, the validator used the
wrong iterators failed to spot the misordered MI tables and instead
walked off into the unknown (as spotted by kasan).

> Signed-off-by: Hanno Boeck <hanno at hboeck.de>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 306d9e4..3a53bf3 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -564,7 +564,7 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring,
>  
>  		for (j = 0; j < table->count; j++) {
>  			const struct drm_i915_cmd_descriptor *desc =
> -				&table->table[i];
> +				&table->table[j];
>  			u32 curr = desc->cmd.value & desc->cmd.mask;
>  
>  			if (curr < previous) {

-- 
Chris Wilson, Intel Open Source Technology Centre