[Mesa-dev] [PATCH 1/3] intel/gen_decoder: Fix length for Media State/Object commands

[Jordan Justen]

On 2017-04-04 06:21:04, Lionel Landwerlin wrote:
> On 04/04/17 00:22, Jordan Justen wrote:
> >  From BDW PRM, Volume 6: Command Stream Programming, 'Render Command
> > Header Format'.
> >
> > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > ---
> >   src/intel/common/gen_decoder.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
> > index 1ae78c88e3f..1244f4c4480 100644
> > --- a/src/intel/common/gen_decoder.c
> > +++ b/src/intel/common/gen_decoder.c
> > @@ -697,8 +697,16 @@ gen_group_get_length(struct gen_group *group, const uint32_t *p)
> >            return field(h, 0, 7) + 2;
> >         case 1:
> >            return 1;
> > -      case 2:
> > -         return 2;
> > +      case 2: {
> > +         uint32_t opcode = field(h, 24, 26);
> > +         assert(opcode < 3 /* 3 and above currently reserved */);
> > +         if (opcode == 0)
> > +            return field(h, 0, 7) + 2;
> 
> It's a bit hard to read (so I might be wrong), but this doesn't seem 
> correct.
> For example 3DSTATE_AA_LINE_PARAMETERS has an opcode=1, yet it's 
> DWordLength field is 0-7bits.

Yeah, it is pretty tough to follow the code. :\ The 'case 2' switch is
for the sub-type. 3DSTATE_AA_LINE_PARAMETERS sub-type is 3, so it
should fall into 'case 3' below, which only looks at bits 0-7 for all
opcodes.

-Jordan

> 
> At this point I'm wondering whether it's not better off using looking 
> for the actual instruction and getting the "DWord Length" to know how to 
> read the length right.
> Potentially printing a warning to tell genxml might need to be updated 
> if we're missing the instruction in genxml.
> What do you think?
> 
> > +         else if (opcode < 3)
> > +            return field(h, 0, 15) + 2;
> > +         else
> > +            return 1; /* FIXME: if more opcodes are added */
> > +      }
> >         case 3:
> >            return field(h, 0, 7) + 2;
> >         }
> 
>