Re: [Mesa-dev] [PATCH 1/2] intel: gen-decoder: rework how we handle groups

[Lionel Landwerlin]

On 30/05/17 22:39, Rafael Antognolli wrote:
On Tue, May 30, 2017 at 08:59:06PM +0100, Lionel Landwerlin wrote:
The current way of handling groups doesn't seem to be able to handle
MI_LOAD_REGISTER_* with more than one register.
Hi Lionel, I don't think this is entirely true. I have added support to read
variable length structs on commit
1ea41163eb0657e7c6cd46c45bdbc3fd4e8e72f8. Maybe it doesn't work for every
case, but at least partial support is there.

Yeah, I looked at this commit and I couldn't work out how things worked.
That's mostly why I rewrote it this way.


I also tested this patch on arb_gpu_shader5-xfb-streams on hsw, because I
wanted to see the behavior when reading the MI_LOAD_REGISTER_IMM from
hsw_begin_transform_feedback, and it seems that aubinator gets into some kind
of infinite loop if I don't apply patch #2. But if I do apply it, then
aubinator crashes.

I just took an aubdump of arb_gpu_shader5-xfb-streams and it seems to 
work fine.
Do you have dump I could use to reproduce?


the way we handle groups by building a traversal list on load the
GENXML files.

Let's say you have

Instruction {
   Field0
   Field1
   Field2
   Group0 (count=2) {
     Field0-0
     Field0-1
   }
   Group1 (count=4) {
     Field1-0
     Field1-1
   }
}

We build of linked on load that goes :

Instruction -> Group0 -> Group1

All of those are gen_group structures, making the traversal trivial.
We just need to iterate groups for the right number of times (count
field in genxml).

The more fancy case is when you have only a single group of unknown
size (count=0). In that case we keep on reading that group for as long
as we're within the DWordLength of that instruction.
If I understood correctly, the main change is that the groups are organized in
a linked list instead of an array, and that makes the traversal of the whole
thing easier, right? Is there any other functional change?

That's pretty much it. There was a couple of variable completely unused 
in gen_group & gen_field_iterator.
Sorry it's a bit messy :/

-
Lionel


Rafael

Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
---
  src/intel/common/gen_decoder.c | 208 +++++++++++++++++++++++++++--------------
  src/intel/common/gen_decoder.h |  19 ++--
  src/intel/tools/aubinator.c    |   4 +-
  3 files changed, 151 insertions(+), 80 deletions(-)

diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
index 3ea2eaf8dbf..6db02c854ff 100644
--- a/src/intel/common/gen_decoder.c
+++ b/src/intel/common/gen_decoder.c
@@ -38,6 +38,8 @@

  #define XML_BUFFER_SIZE 4096

+#define MAX(a, b) ((a) < (b) ? (b) : (a))
+
  #define MAKE_GEN(major, minor) ( ((major) << 8) | (minor) )

  struct gen_spec {
@@ -67,9 +69,6 @@ struct parser_context {
     struct gen_group *group;
     struct gen_enum *enoom;

-   int nfields;
-   struct gen_field *fields[128];
-
     int nvalues;
     struct gen_value *values[256];

@@ -178,8 +177,32 @@ xzalloc(size_t s)
     return fail_on_null(zalloc(s));
  }

+static void
+get_group_offset_count(const char **atts, uint32_t *offset, uint32_t *count,
+                       uint32_t *size, bool *variable)
+{
+   char *p;
+   int i;
+
+   for (i = 0; atts[i]; i += 2) {
+      if (strcmp(atts[i], "count") == 0) {
+         *count = strtoul(atts[i + 1], &p, 0);
+         if (*count == 0)
+            *variable = true;
+      } else if (strcmp(atts[i], "start") == 0) {
+         *offset = strtoul(atts[i + 1], &p, 0);
+      } else if (strcmp(atts[i], "size") == 0) {
+         *size = strtoul(atts[i + 1], &p, 0);
+      }
+   }
+   return;
+}
+
  static struct gen_group *
-create_group(struct parser_context *ctx, const char *name, const char **atts)
+create_group(struct parser_context *ctx,
+             const char *name,
+             const char **atts,
+             struct gen_group *parent)
  {
     struct gen_group *group;

@@ -190,9 +213,17 @@ create_group(struct parser_context *ctx, const char *name, 
const char **atts)
     group->spec = ctx->spec;
     group->group_offset = 0;
     group->group_count = 0;
-   group->variable_offset = 0;
     group->variable = false;

+   if (parent) {
+      group->parent = parent;
+      get_group_offset_count(atts,
+                             &group->group_offset,
+                             &group->group_count,
+                             &group->group_size,
+                             &group->variable);
+   }
+
     return group;
  }

@@ -211,28 +242,6 @@ create_enum(struct parser_context *ctx, const char *name, 
const char **atts)
  }

  static void
-get_group_offset_count(struct parser_context *ctx, const char *name,
-                       const char **atts, uint32_t *offset, uint32_t *count,
-                       uint32_t *elem_size, bool *variable)
-{
-   char *p;
-   int i;
-
-   for (i = 0; atts[i]; i += 2) {
-      if (strcmp(atts[i], "count") == 0) {
-         *count = strtoul(atts[i + 1], &p, 0);
-         if (*count == 0)
-            *variable = true;
-      } else if (strcmp(atts[i], "start") == 0) {
-         *offset = strtoul(atts[i + 1], &p, 0);
-      } else if (strcmp(atts[i], "size") == 0) {
-         *elem_size = strtoul(atts[i + 1], &p, 0);
-      }
-   }
-   return;
-}
-
-static void
  get_register_offset(const char **atts, uint32_t *offset)
  {
     char *p;
@@ -336,14 +345,9 @@ create_field(struct parser_context *ctx, const char **atts)
        if (strcmp(atts[i], "name") == 0)
           field->name = xstrdup(atts[i + 1]);
        else if (strcmp(atts[i], "start") == 0)
-         field->start = ctx->group->group_offset+strtoul(atts[i + 1], &p, 0);
+         field->start = strtoul(atts[i + 1], &p, 0);
        else if (strcmp(atts[i], "end") == 0) {
-         field->end = ctx->group->group_offset+strtoul(atts[i + 1], &p, 0);
-         if (ctx->group->group_offset) {
-            ctx->group->group_offset = field->end+1;
-            if (ctx->group->variable)
-               ctx->group->variable_offset = ctx->group->group_offset;
-         }
+         field->end = strtoul(atts[i + 1], &p, 0);
        } else if (strcmp(atts[i], "type") == 0)
           field->type = string_to_type(ctx, atts[i + 1]);
        else if (strcmp(atts[i], "default") == 0 &&
@@ -372,9 +376,31 @@ create_value(struct parser_context *ctx, const char **atts)
  }

  static void
+create_and_append_field(struct parser_context *ctx,
+                        const char **atts)
+{
+   if (ctx->group->nfields == ctx->group->fields_size) {
+      ctx->group->fields_size = MAX(ctx->group->fields_size * 2, 2);
+
+      if (!ctx->group->fields) {
+         ctx->group->fields =
+            (struct gen_field **) calloc(ctx->group->fields_size,
+                                         sizeof(ctx->group->fields[0]));
+      } else {
+         ctx->group->fields =
+            (struct gen_field **) realloc(ctx->group->fields,
+                                          sizeof(ctx->group->fields[0]) * 
ctx->group->fields_size);
+      }
+   }
+
+   ctx->group->fields[ctx->group->nfields++] = create_field(ctx, atts);
+}
+
+static void
  start_element(void *data, const char *element_name, const char **atts)
  {
     struct parser_context *ctx = data;
+   struct gen_spec *prev_spec = ctx->spec;
     int i;
     const char *name = NULL;
     const char *gen = NULL;
@@ -405,25 +431,30 @@ start_element(void *data, const char *element_name, const 
char **atts)
        ctx->spec->gen = MAKE_GEN(major, minor);
     } else if (strcmp(element_name, "instruction") == 0 ||
                strcmp(element_name, "struct") == 0) {
-      ctx->group = create_group(ctx, name, atts);
+      ctx->group = create_group(ctx, name, atts, NULL);
     } else if (strcmp(element_name, "register") == 0) {
-      ctx->group = create_group(ctx, name, atts);
+      ctx->group = create_group(ctx, name, atts, NULL);
        get_register_offset(atts, &ctx->group->register_offset);
     } else if (strcmp(element_name, "group") == 0) {
-      get_group_offset_count(ctx, name, atts, &ctx->group->group_offset,
-                             &ctx->group->group_count, &ctx->group->elem_size,
-                             &ctx->group->variable);
+      struct gen_group *previous_group = ctx->group;
+      while (previous_group->next)
+         previous_group = previous_group->next;
+
+      struct gen_group *group = create_group(ctx, "", atts, ctx->group);
+      previous_group->next = group;
+      ctx->group = group;
     } else if (strcmp(element_name, "field") == 0) {
-      do {
-         ctx->fields[ctx->nfields++] = create_field(ctx, atts);
-         if (ctx->group->group_count)
-            ctx->group->group_count--;
-      } while (ctx->group->group_count > 0);
+      create_and_append_field(ctx, atts);
     } else if (strcmp(element_name, "enum") == 0) {
        ctx->enoom = create_enum(ctx, name, atts);
     } else if (strcmp(element_name, "value") == 0) {
        ctx->values[ctx->nvalues++] = create_value(ctx, atts);
+      assert(ctx->nvalues < ARRAY_SIZE(ctx->values));
     }
+
+   assert(prev_spec == ctx->spec);
+
+
  }

  static void
@@ -435,14 +466,9 @@ end_element(void *data, const char *name)
     if (strcmp(name, "instruction") == 0 ||
         strcmp(name, "struct") == 0 ||
         strcmp(name, "register") == 0) {
-      size_t size = ctx->nfields * sizeof(ctx->fields[0]);
        struct gen_group *group = ctx->group;

-      group->fields = xzalloc(size);
-      group->nfields = ctx->nfields;
-      memcpy(group->fields, ctx->fields, size);
-      ctx->nfields = 0;
-      ctx->group = NULL;
+      ctx->group = ctx->group->parent;

        for (int i = 0; i < group->nfields; i++) {
           if (group->fields[i]->start >= 16 &&
@@ -461,12 +487,15 @@ end_element(void *data, const char *name)
           spec->structs[spec->nstructs++] = group;
        else if (strcmp(name, "register") == 0)
           spec->registers[spec->nregisters++] = group;
+
+      assert(spec->ncommands < ARRAY_SIZE(spec->commands));
+      assert(spec->nstructs < ARRAY_SIZE(spec->structs));
+      assert(spec->nregisters < ARRAY_SIZE(spec->registers));
     } else if (strcmp(name, "group") == 0) {
-      ctx->group->group_offset = 0;
-      ctx->group->group_count = 0;
+      ctx->group = ctx->group->parent;
     } else if (strcmp(name, "field") == 0) {
-      assert(ctx->nfields > 0);
-      struct gen_field *field = ctx->fields[ctx->nfields - 1];
+      assert(ctx->group->nfields > 0);
+      struct gen_field *field = ctx->group->fields[ctx->group->nfields - 1];
        size_t size = ctx->nvalues * sizeof(ctx->values[0]);
        field->inline_enum.values = xzalloc(size);
        field->inline_enum.nvalues = ctx->nvalues;
@@ -752,12 +781,11 @@ gen_field_iterator_init(struct gen_field_iterator *iter,
                          const uint32_t *p,
                          bool print_colors)
  {
+   memset(iter, 0, sizeof(*iter));
+
     iter->group = group;
     iter->p = p;
-   iter->i = 0;
     iter->print_colors = print_colors;
-   iter->repeat = false;
-   iter->addr_inc = 0;
  }

  static const char *
@@ -771,6 +799,56 @@ gen_get_enum_name(struct gen_enum *e, uint64_t value)
     return NULL;
  }

+static bool iter_more_fields(const struct gen_field_iterator *iter)
+{
+   return iter->field_iter < iter->group->nfields;
+}
+
+static uint32_t iter_group_offset_bits(const struct gen_field_iterator *iter,
+                                       uint32_t group_iter)
+{
+   return iter->group->group_offset + (group_iter * iter->group->group_size);
+}
+
+static bool iter_more_groups(const struct gen_field_iterator *iter)
+{
+   if (iter->group->variable) {
+      return iter_group_offset_bits(iter, iter->group_iter + 1) <
+              (gen_group_get_length(iter->group, iter->p) * 32);
+   } else {
+      return (iter->group_iter + 1) < iter->group->group_count ||
+         iter->group->next != NULL;
+   }
+}
+
+static void iter_advance_group(struct gen_field_iterator *iter)
+{
+   if (iter->group->variable)
+      iter->group_iter++;
+   else {
+      if ((iter->group_iter + 1) < iter->group->group_count) {
+         iter->group_iter++;
+      } else {
+         iter->group = iter->group->next;
+         iter->group_iter = 0;
+      }
+   }
+
+   iter->field_iter = 0;
+}
+
+static void iter_advance_field(struct gen_field_iterator *iter)
+{
+   if (iter->field_iter == iter->group->nfields)
+      iter_advance_group(iter);
+
+   iter->field = iter->group->fields[iter->field_iter++];
+   iter->name = iter->field->name;
+   iter->dword = iter_group_offset_bits(iter, iter->group_iter) / 32 +
+      iter->field->start / 32;
+   iter->struct_desc = NULL;
+}
+
  bool
  gen_field_iterator_next(struct gen_field_iterator *iter)
  {
@@ -779,22 +857,10 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
        float f;
     } v;

-   if (iter->i == iter->group->nfields) {
-      if (iter->group->group_size > 0) {
-         int iter_length = iter->group->elem_size;
-
-         iter->group->group_size -= iter_length / 32;
-         iter->addr_inc += iter_length;
-         iter->dword = (iter->field->start + iter->addr_inc) / 32;
-         return true;
-      }
+   if (!iter_more_fields(iter) && !iter_more_groups(iter))
        return false;
-   }

-   iter->field = iter->group->fields[iter->i++];
-   iter->name = iter->field->name;
-   iter->dword = iter->field->start / 32;
-   iter->struct_desc = NULL;
+   iter_advance_field(iter);

     if ((iter->field->end - iter->field->start) > 32)
        v.qw = ((uint64_t) iter->p[iter->dword+1] << 32) | iter->p[iter->dword];
diff --git a/src/intel/common/gen_decoder.h b/src/intel/common/gen_decoder.h
index 4f4295ff95a..0a4de6bc8f9 100644
--- a/src/intel/common/gen_decoder.h
+++ b/src/intel/common/gen_decoder.h
@@ -58,23 +58,28 @@ struct gen_field_iterator {
     struct gen_group *struct_desc;
     const uint32_t *p;
     int dword; /**< current field starts at &p[dword] */
-   int i;
+
+   int field_iter;
+   int group_iter;
+
     struct gen_field *field;
     bool print_colors;
-   bool repeat;
-   uint32_t addr_inc;
  };

  struct gen_group {
     struct gen_spec *spec;
     char *name;
-   int nfields;
+
     struct gen_field **fields;
+   uint32_t nfields;
+   uint32_t fields_size;
+
     uint32_t group_offset, group_count;
-   uint32_t elem_size;
-   uint32_t variable_offset;
-   bool variable;
     uint32_t group_size;
+   bool variable;
+
+   struct gen_group *parent;
+   struct gen_group *next;

     uint32_t opcode_mask;
     uint32_t opcode;
diff --git a/src/intel/tools/aubinator.c b/src/intel/tools/aubinator.c
index 53b2a27fa90..5ad884c9e61 100644
--- a/src/intel/tools/aubinator.c
+++ b/src/intel/tools/aubinator.c
@@ -108,8 +108,8 @@ valid_offset(uint32_t offset)
  static void
  group_set_size(struct gen_group *strct, uint32_t size)
  {
-   if (strct->variable && strct->elem_size)
-      strct->group_size = size - (strct->variable_offset / 32);
+   if (strct->variable && strct->group_size)
+      strct->group_size = size - (strct->group_offset / 32);
  }

  static void
--
2.11.0
_______________________________________________
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