[Mesa-dev] [PATCH v2 1/2] intel: gen-decoder: rework how we handle groups
Rafael Antognolli
rafael.antognolli at intel.com
Mon Jun 5 22:54:53 UTC 2017
Hi Lionel,
I went through this patch more than once, but I think I understood what you
did, and I think it highly simplifies the code, not necessarily in lines of
code but at least the logic is way easier to follow.
Just a minor nitpick below.
On Wed, May 31, 2017 at 10:32:52AM +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. This change reworks
> the way we handle groups by building a traversal list on loading 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 timers (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.
>
> v2: Fix handling on empty groups & nameless fields (see gen75.xml) (Lionel)
> Now that we have proper iteration over count=0 groups, don't let
> aubinator overwrite the group_size fields (Lionel
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
> src/intel/common/gen_decoder.c | 226 +++++++++++++++++++++++++++--------------
> src/intel/common/gen_decoder.h | 21 ++--
> src/intel/tools/aubinator.c | 18 ----
> 3 files changed, 162 insertions(+), 103 deletions(-)
>
> diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
> index 8d0bf3cee67..b51268ececb 100644
> --- a/src/intel/common/gen_decoder.c
> +++ b/src/intel/common/gen_decoder.c
> @@ -39,6 +39,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 {
> @@ -68,9 +70,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];
>
> @@ -179,8 +178,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;
>
> @@ -191,9 +214,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;
> }
>
> @@ -212,28 +243,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;
> @@ -325,7 +334,7 @@ string_to_type(struct parser_context *ctx, const char *s)
> }
>
> static struct gen_field *
> -create_field(struct parser_context *ctx, const char **atts, int group_idx)
> +create_field(struct parser_context *ctx, const char **atts)
> {
> struct gen_field *field;
> char *p;
> @@ -335,21 +344,11 @@ create_field(struct parser_context *ctx, const char **atts, int group_idx)
>
> for (i = 0; atts[i]; i += 2) {
> if (strcmp(atts[i], "name") == 0)
> - if (ctx->group->elem_size == 0) {
> - field->name = xstrdup(atts[i + 1]);
> - } else {
> - field->name =
> - ralloc_asprintf(NULL, "%s[%d]", atts[i + 1], group_idx);
> - }
> + 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->elem_size > 0) {
> - 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 &&
> @@ -378,6 +377,27 @@ 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);
> + }
realloc() does not initialize the newly added memory, unlike calloc(). So you
can't depend on those new pointers to be zero. It doesn't look like you depend
on that because you have the nfields variable anyways. But if you really don't
care about initializing it, you could simply do a realloc (there's no need for
the calloc part), since if ctx->group->fields is NULL it will act as a simple
malloc anyway.
With this fixed, this patch is:
Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>
> + }
> +
> + 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;
> @@ -411,23 +431,27 @@ 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) {
> - for (int g = 0; g < MAX2(ctx->group->group_count, 1); g++) {
> - ctx->fields[ctx->nfields++] = create_field(ctx, atts, g);
> - }
> + 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));
> }
> +
> }
>
> static void
> @@ -439,14 +463,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 &&
> @@ -465,12 +484,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;
> @@ -756,12 +778,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 *
> @@ -775,6 +796,65 @@ 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 bool iter_advance_field(struct gen_field_iterator *iter)
> +{
> + while (!iter_more_fields(iter)) {
> + if (iter_more_groups(iter))
> + iter_advance_group(iter);
> + else
> + return false;
> + }
> +
> + iter->field = iter->group->fields[iter->field_iter++];
> + if (iter->field->name)
> + strncpy(iter->name, iter->field->name, sizeof(iter->name));
> + else
> + memset(iter->name, 0, sizeof(iter->name));
> + iter->dword = iter_group_offset_bits(iter, iter->group_iter) / 32 +
> + iter->field->start / 32;
> + iter->struct_desc = NULL;
> +
> + return true;
> +}
> +
> bool
> gen_field_iterator_next(struct gen_field_iterator *iter)
> {
> @@ -783,22 +863,8 @@ 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_advance_field(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;
>
> if ((iter->field->end - iter->field->start) > 32)
> v.qw = ((uint64_t) iter->p[iter->dword+1] << 32) | iter->p[iter->dword];
> @@ -864,6 +930,12 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
> }
> }
>
> + if (strlen(iter->group->name) == 0) {
> + int length = strlen(iter->name);
> + snprintf(iter->name + length, sizeof(iter->name) - length,
> + "[%i]", iter->group_iter);
> + }
> +
> if (enum_name) {
> int length = strlen(iter->value);
> snprintf(iter->value + length, sizeof(iter->value) - length,
> diff --git a/src/intel/common/gen_decoder.h b/src/intel/common/gen_decoder.h
> index 4f4295ff95a..cfc9f2e3f15 100644
> --- a/src/intel/common/gen_decoder.h
> +++ b/src/intel/common/gen_decoder.h
> @@ -53,28 +53,33 @@ struct gen_enum *gen_spec_find_enum(struct gen_spec *spec, const char *name);
>
> struct gen_field_iterator {
> struct gen_group *group;
> - const char *name;
> + char name[128];
> char value[128];
> 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..f481fbe0075 100644
> --- a/src/intel/tools/aubinator.c
> +++ b/src/intel/tools/aubinator.c
> @@ -95,23 +95,6 @@ valid_offset(uint32_t offset)
> return offset < gtt_end;
> }
>
> -/**
> - * Set group variable size for groups with count="0".
> - *
> - * By default the group size is fixed and not needed because the struct
> - * describing a group knows the number of elements. However, for groups with
> - * count="0" we have a variable number of elements, and the struct describing
> - * the group only includes one of them. So we calculate the remaining size of
> - * the group based on the size we get here, and the offset after the last
> - * element added to the group.
> - */
> -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);
> -}
> -
> static void
> decode_group(struct gen_group *strct, const uint32_t *p, int starting_dword)
> {
> @@ -740,7 +723,6 @@ parse_commands(struct gen_spec *spec, uint32_t *cmds, int size, int engine)
> gen_group_get_name(inst), reset_color);
>
> if (option_full_decode) {
> - group_set_size(inst, length);
> decode_group(inst, p, 0);
>
> for (i = 0; i < ARRAY_LENGTH(custom_handlers); i++) {
> --
> 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