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.jus...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
