[Mesa-dev] [PATCH] intel/decoder: Use 'DWord Length' and 'bias' fields for packet length.

[Lionel Landwerlin]

On 26/10/2018 15:06, Toni Lönnberg wrote:
> ----- Forwarded message from Toni Lönnberg <toni.lonnberg at intel.com> -----
>
> Date: Fri, 26 Oct 2018 17:03:54 +0300
> From: Toni Lönnberg <toni.lonnberg at intel.com>
> To: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Message-ID: <20181026140354.GD3100 at intel.com>
> User-Agent: Mutt/1.9.4 (2018-02-28)
> Subject: Re: [Mesa-dev] [PATCH] intel/decoder: Use 'DWord Length' and 'bias' fields for packet length.
>
> On Fri, Oct 26, 2018 at 02:24:19PM +0100, Lionel Landwerlin wrote:
>> Overall I agree this probably what we should do rather than this long
>> special case switch in gen_group_get_length().
> There are some other changes that I'm trying to do because some of the opcodes are being shared between engines but have completely different formats depending on the engine they're sent to, MEDIA_CURBE_LOAD vs. MFX_SURFACE_STATE for example, making it impossible to distinguish between the two of them at the moment AFAIK.


I guess the way to do this would be to add a field 
engine="(3d|media|all)" for each instruction in the genxml file.

Then change gen_spec_find_instruction() to add an engine enum and do the 
matching there.

We could set the engine on the gen_batch_decoder 
(src/intel/common/gen_batch_decoder.c) when we initialize it.


I think some discussion happened at some point to try to split things a 
bit, so we can reduce the xml file and maybe not embed everything in the 
3d driver.

For instance we won't care much about media instructions. Similarly 
we've pretty much stopped using the blitter (I have patches for those 
instructions), if we want them for debug tools, better have that separate...


-

Lionel


>
>> I have one suggestion below.
>>
>> On 26/10/2018 14:07, Toni Lönnberg wrote:
>>> Use the 'DWord Length' and 'bias' fields from the instruction definition to
>>> parse the packet length from the command stream when possible. The hardcoded
>>> mechanism is used whenever an instruction doesn't have this field.
>>> ---
>>>    src/intel/common/gen_decoder.c | 16 ++++++++++++++--
>>>    src/intel/common/gen_decoder.h |  1 +
>>>    2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
>>> index 5f6e7a0b808..2d58708f5dd 100644
>>> --- a/src/intel/common/gen_decoder.c
>>> +++ b/src/intel/common/gen_decoder.c
>>> @@ -163,11 +163,15 @@ create_group(struct parser_context *ctx,
>>>       group->spec = ctx->spec;
>>>       group->variable = false;
>>>       group->fixed_length = fixed_length;
>>> +   group->dw_length = 0;
>>> +   group->bias = 1;
>>>       for (int i = 0; atts[i]; i += 2) {
>>>          char *p;
>>>          if (strcmp(atts[i], "length") == 0) {
>>>             group->dw_length = strtoul(atts[i + 1], &p, 0);
>>> +      } else if (strcmp(atts[i], "bias") == 0) {
>>> +         group->bias = strtoul(atts[i + 1], &p, 0);
>>>          }
>>>       }
>>> @@ -743,8 +747,16 @@ gen_group_find_field(struct gen_group *group, const char *name)
>>>    int
>>>    gen_group_get_length(struct gen_group *group, const uint32_t *p)
>>>    {
>>> -   if (group && group->fixed_length)
>>> -      return group->dw_length;
>>> +   if (group) {
>>> +      if (group->fixed_length)
>>> +         return group->dw_length;
>>> +      else {
>>> +         struct gen_field *field = gen_group_find_field(group, "DWord Length");
>>
>> Since "DWord Length" is a field we're going to use quite often, I would put
>> a pointer to the field in gen_group.
>>
>> You can do that in the create_field() function and save your self the
>> find_field() for every instruction.
> Good point.
>
>>
>>> +         if (field) {
>>> +            return field_value(p[0], field->start, field->end) + group->bias;
>>>            }
>>> +      }
>>> +   }
>>
>> If we had tests (I'm meant to write some), I would go as far as removing the
>> whole thing below and just return 1.
>>
>> It means we don't have a group and in that case we're likely going to just
>> iterate dword by dword.
> Since we don't have tests for this, IMHO it's better to leave it there for the time being. I don't have any information whether there are any non-single dword packets without the 'DWord Length' field, but I'm doubtful.


If you ever have a pressing need to write tests, I could thing a few 
things that could be automated so we have decent coverage across gens.


>
>>
>>>       uint32_t h = p[0];
>>>       uint32_t type = field_value(h, 29, 31);
>>> diff --git a/src/intel/common/gen_decoder.h b/src/intel/common/gen_decoder.h
>>> index a80c50b6647..0b33eb32cfc 100644
>>> --- a/src/intel/common/gen_decoder.h
>>> +++ b/src/intel/common/gen_decoder.h
>>> @@ -101,6 +101,7 @@ struct gen_group {
>>>       struct gen_field *fields; /* linked list of fields */
>>>       uint32_t dw_length;
>>> +   uint32_t bias; /* <instruction> specific */
>>>       uint32_t group_offset, group_count;
>>>       uint32_t group_size;
>>>       bool variable; /* <group> specific */
>>
> ----- End forwarded message -----
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev