[Mesa-dev] [PATCH 11/11] intel/aubinator_error_decode: Disassemble shader programs
Matt Turner
mattst88 at gmail.com
Mon May 15 19:05:47 UTC 2017
On Fri, May 5, 2017 at 12:44 AM, Pohjolainen, Topi
<topi.pohjolainen at 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.pohjolainen at 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!
More information about the mesa-dev
mailing list