[Mesa-dev] [PATCH 1/2] intel: gen-decoder: rework how we handle groups
Rafael Antognolli
rafael.antognolli at intel.com
Tue May 30 21:39:42 UTC 2017
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.
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.
> 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?
Rafael
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list