[Mesa-dev] [PATCH 1/2] intel: gen-decoder: rework how we handle groups
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Tue May 30 22:28:06 UTC 2017
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.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