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

[Lionel Landwerlin]

On 04/04/17 19:49, Jordan Justen wrote:
> 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

Oops, sorry, I got confused between sub-type and 3D opcode.

Unfortunately, I found another one that doesn't match any of the current 
cases... :

3DSTATE_BINDING_TABLE_EDIT_DS has 3d opcode = 0 but Dword Length on 0-8 
bits (not 0-7...)
It doesn't really matter because we don't appear to use this 
instruction. And this patch improves the current state :

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

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