On Fri, May 5, 2017 at 12:44 AM, Pohjolainen, Topi <topi.pohjolai...@gmail.com> wrote: > On Mon, May 01, 2017 at 01:54:55PM -0700, Matt Turner wrote: >> --- >> src/intel/Makefile.tools.am | 6 +- >> src/intel/tools/aubinator_error_decode.c | 178 >> ++++++++++++++++++++++++++++++- >> 2 files changed, 180 insertions(+), 4 deletions(-) > > I had a few nits but the logic looks right to me. Thanks for working on this, > I hope we can catch some of the hangs in CI now! > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > >> >> diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am >> index 576beea..1175118 100644 >> --- a/src/intel/Makefile.tools.am >> +++ b/src/intel/Makefile.tools.am >> @@ -47,11 +47,15 @@ tools_aubinator_LDADD = \ >> >> >> tools_aubinator_error_decode_SOURCES = \ >> - tools/aubinator_error_decode.c >> + tools/aubinator_error_decode.c \ >> + tools/disasm.c \ >> + tools/gen_disasm.h >> >> tools_aubinator_error_decode_LDADD = \ >> common/libintel_common.la \ >> + compiler/libintel_compiler.la \ >> $(top_builddir)/src/util/libmesautil.la \ >> + $(PTHREAD_LIBS) \ >> $(EXPAT_LIBS) \ >> $(ZLIB_LIBS) >> >> diff --git a/src/intel/tools/aubinator_error_decode.c >> b/src/intel/tools/aubinator_error_decode.c >> index 244bef8..ef77c01 100644 >> --- a/src/intel/tools/aubinator_error_decode.c >> +++ b/src/intel/tools/aubinator_error_decode.c >> @@ -40,6 +40,7 @@ >> >> #include "common/gen_decoder.h" >> #include "util/macros.h" >> +#include "gen_disasm.h" >> >> #define CSI "\e[" >> #define BLUE_HEADER CSI "0;44m" >> @@ -209,6 +210,17 @@ print_fault_data(struct gen_device_info *devinfo, >> uint32_t data1, uint32_t data0 >> #define CSI "\e[" >> #define NORMAL CSI "0m" >> >> +struct program { >> + const char *type; >> + const char *command; >> + uint64_t command_offset; >> + uint64_t instruction_base_address; >> + uint64_t ksp; >> +}; >> + >> +static struct program programs[4096]; > > How about? > > #define MAX_NUM_PROGRAMS 4096 > static struct program programs[MAX_NUM_PROGRAMS]; > > And guard against overflowing in the iterator further down? Just in case > input is somehow corrupted.
Sounds good. Changed locally. > >> +static int num_programs = 0; >> + >> static void decode(struct gen_spec *spec, >> const char *buffer_name, >> const char *ring_name, >> @@ -219,6 +231,7 @@ static void decode(struct gen_spec *spec, >> uint32_t *p, *end = (data + *count); >> int length; >> struct gen_group *inst; >> + uint64_t current_instruction_base_address = 0; >> >> for (p = data; p < end; p += length) { >> const char *color = option_full_decode ? BLUE_HEADER : NORMAL, >> @@ -246,6 +259,127 @@ static void decode(struct gen_spec *spec, >> >> if (strcmp(inst->name, "MI_BATCH_BUFFER_END") == 0) >> break; >> + >> + if (strcmp(inst->name, "STATE_BASE_ADDRESS") == 0) { >> + struct gen_field_iterator iter; >> + gen_field_iterator_init(&iter, inst, p, false); >> + >> + while (gen_field_iterator_next(&iter)) { >> + if (strcmp(iter.name, "Instruction Base Address") == 0) { >> + current_instruction_base_address = strtol(iter.value, NULL, >> 16); >> + } >> + } >> + } else if (strcmp(inst->name, "WM_STATE") == 0 || >> + strcmp(inst->name, "3DSTATE_PS") == 0 || >> + strcmp(inst->name, "3DSTATE_WM") == 0) { >> + struct gen_field_iterator iter; >> + gen_field_iterator_init(&iter, inst, p, false); >> + uint64_t ksp[3] = {0, 0, 0}; >> + bool enabled[3] = {false, false, false}; >> + >> + while (gen_field_iterator_next(&iter)) { >> + if (strncmp(iter.name, "Kernel Start Pointer ", >> + strlen("Kernel Start Pointer ")) == 0) { >> + int idx = iter.name[strlen("Kernel Start Pointer ")] - '0'; >> + ksp[idx] = strtol(iter.value, NULL, 16); >> + } else if (strcmp(iter.name, "8 Pixel Dispatch Enable") == 0) { >> + enabled[0] = strcmp(iter.value, "true") == 0; >> + } else if (strcmp(iter.name, "16 Pixel Dispatch Enable") == 0) { >> + enabled[1] = strcmp(iter.value, "true") == 0; >> + } else if (strcmp(iter.name, "32 Pixel Dispatch Enable") == 0) { >> + enabled[2] = strcmp(iter.value, "true") == 0; >> + } >> + } >> + >> + /* FINISHME: Broken for multi-program WM_STATE, >> + * which Mesa does not use >> + */ >> + if (enabled[0] + enabled[1] + enabled[2] == 1) { >> + const char *type = enabled[0] ? "SIMD8 fragment shader" : >> + enabled[1] ? "SIMD16 fragment shader" : >> + enabled[2] ? "SIMD32 fragment shader" : NULL; >> + >> + programs[num_programs++] = (struct program) { >> + .type = type, >> + .command = inst->name, >> + .command_offset = offset, >> + .instruction_base_address = current_instruction_base_address, >> + .ksp = ksp[0], >> + }; >> + } else { >> + if (enabled[0]) /* SIMD8 */ { >> + programs[num_programs++] = (struct program) { >> + .type = "SIMD8 fragment shader", >> + .command = inst->name, >> + .command_offset = offset, >> + .instruction_base_address = >> current_instruction_base_address, >> + .ksp = ksp[0], >> + .ksp = ksp[0], /* SIMD8 shader is specified by ksp[0] */ >> + }; >> + } >> + if (enabled[1]) /* SIMD16 */ { >> + programs[num_programs++] = (struct program) { >> + .type = "SIMD16 fragment shader", >> + .command = inst->name, >> + .command_offset = offset, >> + .instruction_base_address = >> current_instruction_base_address, >> + .ksp = ksp[2], /* SIMD16 shader is specified by ksp[2] */ >> + }; >> + } >> + if (enabled[2]) /* SIMD32 */ { >> + programs[num_programs++] = (struct program) { >> + .type = "SIMD32 fragment shader", >> + .command = inst->name, >> + .command_offset = offset, >> + .instruction_base_address = >> current_instruction_base_address, >> + .ksp = ksp[1], /* SIMD32 shader is specified by ksp[1] */ >> + }; >> + } >> + } >> + } else if (strcmp(inst->name, "VS_STATE") == 0 || >> + strcmp(inst->name, "GS_STATE") == 0 || >> + strcmp(inst->name, "SF_STATE") == 0 || >> + strcmp(inst->name, "CLIP_STATE") == 0 || >> + strcmp(inst->name, "3DSTATE_DS") == 0 || >> + strcmp(inst->name, "3DSTATE_HS") == 0 || >> + strcmp(inst->name, "3DSTATE_GS") == 0 || >> + strcmp(inst->name, "3DSTATE_VS") == 0) { >> + struct gen_field_iterator iter; >> + gen_field_iterator_init(&iter, inst, p, false); >> + uint64_t ksp; > > In case of fragment programs you initialise to zero, should we do it here > also? Otherwise there is possibility that we fill random value to program > list below. (I suppose input would need to be invalid again...). I think it's safe either way, but I've added an initialization. >> + bool is_simd8 = false; /* vertex shaders on Gen8+ only */ >> + bool is_enabled = true; > > And shouldn't this be 'false' by default? I think it's correct as is. It's only Gen8+ vertex shaders that have the field (Gen7 and earlier are vec4, geometry shaders have sort of an enum that I missed, tess shaders don't have a bit). So if 3DSTATE_VS doesn't have a bit, it's vec4. If it does have a bit, we check it. I've fixed the geometry shader bug locally. Thanks for reviewing! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev