[Mesa-dev] [PATCH 1/2] intel/genxml: Fix decoding of groups with fields smaller than a DWord.

[Jordan Justen]

On 2017-10-25 21:17:13, Kenneth Graunke wrote:
> Groups containing fields smaller than a DWord were not being decoded
> correctly.  For example:
> 
>     <group count="32" start="32" size="4">
>       <field name="Vertex Element Enables" start="0" end="3" type="uint"/>
>     </group>
> 
> gen_field_iterator_next would properly walk over each element of the
> array, incrementing group_iter, and calling iter_group_offset_bits()
> to advance to the proper DWord.  However, the code to print the actual
> values only considered iter->field->start/end, which are 0 and 3 in the
> above example.  So it would always fetch bits 3:0 of the current DWord
> when printing values, instead of advancing to each element of the array,
> printing bits 0-3, 4-7, 8-11, and so on.
> 
> To fix this, we add new iter->start/end tracking, which properly
> advances for each instance of a group's field.
> 
> Caught by Matt Turner while working on 3DSTATE_VF_COMPONENT_PACKING,
> with a patch to convert it to use an array of bitfields (the example
> above).
> 
> This also fixes the decoding of 3DSTATE_SBE's "Attribute Active
> Component Format" fields.
> ---
>  src/intel/common/gen_decoder.c | 24 ++++++++++++++----------
>  src/intel/common/gen_decoder.h |  2 ++
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
> index 85880143f00..1bf69ac4f94 100644
> --- a/src/intel/common/gen_decoder.c
> +++ b/src/intel/common/gen_decoder.c
> @@ -843,8 +843,12 @@ iter_advance_field(struct gen_field_iterator *iter)
>         strncpy(iter->name, iter->field->name, sizeof(iter->name));
>     else
>        memset(iter->name, 0, sizeof(iter->name));
> -   iter->dword = iter_group_offset_bits(iter, iter->group_iter) / 32 +
> -      iter->field->start / 32;
> +
> +   int group_member_offset = iter_group_offset_bits(iter, iter->group_iter);
> +
> +   iter->start = group_member_offset + iter->field->start;
> +   iter->end = group_member_offset + iter->field->end;
> +   iter->dword = iter->start / 32;
>     iter->struct_desc = NULL;
>  
>     return true;
> @@ -861,7 +865,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>     if (!iter_advance_field(iter))
>        return false;
>  
> -   if ((iter->field->end - iter->field->start) > 32)
> +   if ((iter->end - iter->start) > 32)
>        v.qw = ((uint64_t) iter->p[iter->dword+1] << 32) | iter->p[iter->dword];
>     else
>        v.qw = iter->p[iter->dword];
> @@ -871,13 +875,13 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>     switch (iter->field->type.kind) {
>     case GEN_TYPE_UNKNOWN:
>     case GEN_TYPE_INT: {
> -      uint64_t value = field(v.qw, iter->field->start, iter->field->end);
> +      uint64_t value = field(v.qw, iter->start, iter->end);
>        snprintf(iter->value, sizeof(iter->value), "%"PRId64, value);
>        enum_name = gen_get_enum_name(&iter->field->inline_enum, value);
>        break;
>     }
>     case GEN_TYPE_UINT: {
> -      uint64_t value = field(v.qw, iter->field->start, iter->field->end);
> +      uint64_t value = field(v.qw, iter->start, iter->end);
>        snprintf(iter->value, sizeof(iter->value), "%"PRIu64, value);
>        enum_name = gen_get_enum_name(&iter->field->inline_enum, value);
>        break;
> @@ -886,7 +890,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>        const char *true_string =
>           iter->print_colors ? "\e[0;35mtrue\e[0m" : "true";
>        snprintf(iter->value, sizeof(iter->value), "%s",
> -               field(v.qw, iter->field->start, iter->field->end) ?
> +               field(v.qw, iter->start, iter->end) ?
>                 true_string : "false");
>        break;
>     }
> @@ -896,7 +900,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>     case GEN_TYPE_ADDRESS:
>     case GEN_TYPE_OFFSET:
>        snprintf(iter->value, sizeof(iter->value), "0x%08"PRIx64,
> -               field_address(v.qw, iter->field->start, iter->field->end));
> +               field_address(v.qw, iter->start, iter->end));
>        break;
>     case GEN_TYPE_STRUCT:
>        snprintf(iter->value, sizeof(iter->value), "<struct %s>",
> @@ -907,8 +911,8 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>        break;
>     case GEN_TYPE_UFIXED:
>        snprintf(iter->value, sizeof(iter->value), "%f",
> -               (float) field(v.qw, iter->field->start,
> -                             iter->field->end) / (1 << iter->field->type.f));
> +               (float) field(v.qw, iter->start,
> +                             iter->end) / (1 << iter->field->type.f));

This line break seems odd. Could 'iter->end) /' move to the previous line?

Either way, Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

>        break;
>     case GEN_TYPE_SFIXED:
>        /* FIXME: Sign extend extracted field. */
> @@ -917,7 +921,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>     case GEN_TYPE_MBO:
>         break;
>     case GEN_TYPE_ENUM: {
> -      uint64_t value = field(v.qw, iter->field->start, iter->field->end);
> +      uint64_t value = field(v.qw, iter->start, iter->end);
>        snprintf(iter->value, sizeof(iter->value),
>                 "%"PRId64, value);
>        enum_name = gen_get_enum_name(iter->field->type.gen_enum, value);
> diff --git a/src/intel/common/gen_decoder.h b/src/intel/common/gen_decoder.h
> index cfc9f2e3f15..12d4551a127 100644
> --- a/src/intel/common/gen_decoder.h
> +++ b/src/intel/common/gen_decoder.h
> @@ -58,6 +58,8 @@ struct gen_field_iterator {
>     struct gen_group *struct_desc;
>     const uint32_t *p;
>     int dword; /**< current field starts at &p[dword] */
> +   int start; /**< current field starts at this bit number */
> +   int end;   /**< current field ends at this bit number */
>  
>     int field_iter;
>     int group_iter;
> -- 
> 2.14.2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev