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

[Anuj Phogat]

On Tue, Sep 20, 2016 at 12:35 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.
>
> 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 | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/tools/decoder.c b/src/intel/tools/decoder.c
> index b5f557c..bea9f22 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 dw;
uint64_t qw?
>        float f;
>     } v;
>
> @@ -500,7 +518,13 @@ 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.dw = ((uint64_t) iter->p[index+1] << 32 ) | iter->p[index];
Omit the white space after 32.
> +   else
> +      v.dw = iter->p[index];
> +
>     switch (f->type.kind) {
>     case GEN_TYPE_UNKNOWN:
>     case GEN_TYPE_INT:
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

With above comments fixed:
Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>