[Mesa-dev] [PATCH 09/11] intel/tools/error: Decode shaders while decoding batch commands.

[Kenneth Graunke]

On Sunday, November 12, 2017 3:57:56 AM PST Lionel Landwerlin wrote:
> On 12/11/17 08:35, Kenneth Graunke wrote:
> > This makes aubinator_error_decode's shader dumping work like aubinator.
> > Instead of printing them after the fact, it prints them right inside the
> > 3DSTATE_VS/HS/DS/GS/PS packet that references them.  This saves you the
> > effort of cross-referencing things and jumping back and forth.
> >
> > It also reduces a bunch of book-keeping, and eliminates the limitation
> > that we could only handle 4096 programs.  That code was also broken and
> > failed to print any shaders if there were under 4096 programs.
> > ---
> >   src/intel/tools/aubinator_error_decode.c | 134 +++++++++++--------------------
> >   1 file changed, 49 insertions(+), 85 deletions(-)
> >
> > diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
> > index cea68523ae2..96b4989936e 100644
> > --- a/src/intel/tools/aubinator_error_decode.c
> > +++ b/src/intel/tools/aubinator_error_decode.c
> > @@ -221,35 +221,28 @@ struct section {
> >   #define MAX_SECTIONS 30
> >   static struct section sections[MAX_SECTIONS];
> >   
> > -struct program {
> > -   const char *type;
> > -   const char *command;
> > -   uint64_t command_offset;
> > -   uint64_t instruction_base_address;
> > -   uint64_t ksp;
> > -};
> > -
> > -#define MAX_NUM_PROGRAMS 4096
> > -static struct program programs[MAX_NUM_PROGRAMS];
> > -static int idx_program = 0, num_programs = 0;
> > -
> > -static int next_program(void)
> > +static void
> > +disassemble_program(struct gen_disasm *disasm, const char *type,
> > +                    const struct section *instruction_section,
> > +                    uint64_t ksp)
> >   {
> > -   int ret = idx_program;
> > -   idx_program = (idx_program + 1) % MAX_NUM_PROGRAMS;
> > -   num_programs = MIN(num_programs + 1, MAX_NUM_PROGRAMS);
> > -   return ret;
> > +   if (!instruction_section)
> > +      return;
> > +
> > +   printf("\nReferenced %s:\n", type);
> > +   gen_disasm_disassemble(disasm, instruction_section->data, ksp, stdout);
> >   }
> >   
> >   static void
> > -decode(struct gen_spec *spec, const struct section *section)
> > +decode(struct gen_spec *spec, struct gen_disasm *disasm,
> > +       const struct section *section)
> >   {
> >      uint64_t gtt_offset = section->gtt_offset;
> >      uint32_t *data = section->data;
> >      uint32_t *p, *end = (data + section->count);
> >      int length;
> >      struct gen_group *inst;
> > -   uint64_t current_instruction_base_address = 0;
> > +   const struct section *current_instruction_buffer = NULL;
> >   
> >      for (p = data; p < end; p += length) {
> >         const char *color = option_full_decode ? BLUE_HEADER : NORMAL,
> > @@ -284,7 +277,13 @@ decode(struct gen_spec *spec, const struct section *section)
> >   
> >            while (gen_field_iterator_next(&iter)) {
> >               if (strcmp(iter.name, "Instruction Base Address") == 0) {
> > -               current_instruction_base_address = strtol(iter.value, NULL, 16);
> > +               uint64_t instr_base_address = strtol(iter.value, NULL, 16);
> > +               current_instruction_buffer = NULL;
> > +               for (int s = 0; s < MAX_SECTIONS; s++) {
> > +                  if (sections[s].gtt_offset == instr_base_address) {
> 
> Is it a guarantee that the instruction buffer is going to be given in 
> its own section?

The section corresponds to a buffer marked with EXEC_OBJECT_CAPTURE, so
assuming the driver has programmed STATE_BASE_ADDRESS to the start of a
buffer, this will be true.  Otherwise, we'll get no assembly or state
dumping.

That seems like the obvious thing for a driver to do, but I suppose they
could also point it the middle of a buffer somewhere.  Mesa and SNA both
point to the start of a buffer.

I can write a follow-up patch to allow that, if you want...

> Or do we need to check bounds ?
> 
> if (instr_base_address >= sections[s].gtt_offset && inst_base_address <= 
> sections[s].gtt_offset + sections[s].count * 4)
> 
> Otherwise it looks fine to me :
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171113/915194d7/attachment-0001.sig>