[Mesa-dev] [PATCH v3] aubinator: Fix the decoding of values that span two Dwords

[Anuj Phogat]

On Tue, Sep 20, 2016 at 3:59 PM, Sirisha Gandikota
<sirisha.gandikota at intel.com> wrote:
> From: Sirisha Gandikota <Sirisha.Gandikota at intel.com>
>
> Fixed the way the values that span two Dwords are decoded.
> Based on the start and end indices of the field, the Dwords
> are fetched and decoded accordingly.
>
> v2: rename dw to qw in gen_field_iterator_next
> and remove extra white space (Anuj)
>
> v3: change all instances of dw to qw (Anuj)
>
> Earlier, 64-bit fields (such as most pointers on Gen8+)
> weren't decoded correctly.  gen_field_iterator_next seemed
> to walk one DWord at a time, sets v.dw, and then passes it
> to field(). So, even though field() takes a uint64_t, we're
> passing it a uint32_t (which gets promoted, so the top 32
> bits will always be zero). This seems pretty bogus... (Ken)
>
> Signed-off-by: Sirisha Gandikota <Sirisha.Gandikota at intel.com>
> ---
>  src/intel/tools/decoder.c | 50 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/tools/decoder.c b/src/intel/tools/decoder.c
> index b5f557c..be3558b 100644
> --- a/src/intel/tools/decoder.c
> +++ b/src/intel/tools/decoder.c
> @@ -191,6 +191,26 @@ get_register_offset(const char **atts, uint32_t *offset)
>     return;
>  }
>
> +static void
> +get_start_end_pos(int *start, int *end)
> +{
> +   /* start value has to be mod with 32 as we need the relative
> +    * start position in the first DWord. For the end position, add
> +    * the length of the field to the start position to get the
> +    * relative postion in the 64 bit address.
> +    */
> +   if (*end - *start > 32) {
> +      int len = *end - *start;
> +      *start = *start % 32;
> +      *end = *start + len;
> +   } else {
> +      *start = *start % 32;
> +      *end = *end % 32;
> +   }
> +
> +   return;
> +}
> +
>  static inline uint64_t
>  mask(int start, int end)
>  {
> @@ -204,18 +224,16 @@ mask(int start, int end)
>  static inline uint64_t
>  field(uint64_t value, int start, int end)
>  {
> -   /* The field values are obtained from the DWord,
> -    * Hence, get the relative start and end positions
> -    * by doing a %32 on the start and end positions
> -    */
> -   return (value & mask(start % 32, end % 32)) >> (start % 32);
> +   get_start_end_pos(&start, &end);
> +   return (value & mask(start, end)) >> (start);
>  }
>
>  static inline uint64_t
>  field_address(uint64_t value, int start, int end)
>  {
>     /* no need to right shift for address/offset */
> -   return (value & mask(start % 32, end % 32));
> +   get_start_end_pos(&start, &end);
> +   return (value & mask(start, end));
>  }
>
>  static struct gen_type
> @@ -491,7 +509,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>  {
>     struct gen_field *f;
>     union {
> -      uint32_t dw;
> +      uint64_t qw;
>        float f;
>     } v;
>
> @@ -500,20 +518,26 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>
>     f = iter->group->fields[iter->i++];
>     iter->name = f->name;
> -   v.dw = iter->p[f->start / 32];
> +   int index = f->start / 32;
> +
> +   if ((f->end - f->start) > 32)
> +      v.qw = ((uint64_t) iter->p[index+1] << 32) | iter->p[index];
> +   else
> +      v.qw = iter->p[index];
> +
>     switch (f->type.kind) {
>     case GEN_TYPE_UNKNOWN:
>     case GEN_TYPE_INT:
>        snprintf(iter->value, sizeof(iter->value),
> -               "%ld", field(v.dw, f->start, f->end));
> +               "%ld", field(v.qw, f->start, f->end));
>        break;
>     case GEN_TYPE_UINT:
>        snprintf(iter->value, sizeof(iter->value),
> -               "%lu", field(v.dw, f->start, f->end));
> +               "%lu", field(v.qw, f->start, f->end));
>        break;
>     case GEN_TYPE_BOOL:
>        snprintf(iter->value, sizeof(iter->value),
> -               "%s", field(v.dw, f->start, f->end) ? "true" : "false");
> +               "%s", field(v.qw, f->start, f->end) ? "true" : "false");
>        break;
>     case GEN_TYPE_FLOAT:
>        snprintf(iter->value, sizeof(iter->value), "%f", v.f);
> @@ -521,7 +545,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%08lx", field_address(v.dw, f->start, f->end));
> +               "0x%08lx", field_address(v.qw, f->start, f->end));
>        break;
>     case GEN_TYPE_STRUCT:
>        snprintf(iter->value, sizeof(iter->value),
> @@ -529,7 +553,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>        break;
>     case GEN_TYPE_UFIXED:
>        snprintf(iter->value, sizeof(iter->value),
> -               "%f", (float) field(v.dw, f->start, f->end) / (1 << f->type.f));
> +               "%f", (float) field(v.qw, f->start, f->end) / (1 << f->type.f));
>        break;
>     case GEN_TYPE_SFIXED:
>        /* FIXME: Sign extend extracted field. */
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>