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

[Lionel Landwerlin]

Overall I agree this probably what we should do rather than this long 
special case switch in gen_group_get_length().
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.


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


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