Re: Error in inner loop in validate_cmds_sorted / out of bounds issue

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 28, 2015 at 08:36:18PM +0100, Chris Wilson wrote:
> 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@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@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@hboeck.de>
> Reviewed-by: Chris Wilson <chris@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@hboeck.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok I hand-assembled the patches since it's your first, but please followo
Documentation/Submitting patches next time around, i.e. 1 mail per patch,
no attachments, diff included at the bottom. Otherwise the tools can't
just pick up patches on my side. git send-email will do all that
formatting for you correctly, so that's what I recommend to use.

Anyway pachtes applied to drm-intel-next-queued, thanks a lot for doing
them.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx