[2/2] intel/tools: Fix program disassembly in aubinator_error_decode.

Submitted by Kenneth Graunke on Nov. 11, 2017, 12:55 a.m.

Details

Message ID 20171111005542.13701-2-kenneth@whitecape.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Nov. 11, 2017, 12:55 a.m.
This indexing is bogus.  idx_program is the offset of the next program.
At this point, we've walked through the entire batch, and accumulated
all the programs.  So adding it again simply skips over 100% of the
programs.
---
 src/intel/tools/aubinator_error_decode.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
index 2322bac8391..00e1a872bd0 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -601,15 +601,14 @@  read_data_file(FILE *file)
             printf("Disassembly of programs in instruction buffer at "
                    "0x%08"PRIx64":\n", gtt_offset);
             for (int i = 0; i < num_programs; i++) {
-               int idx = (idx_program + i) % MAX_NUM_PROGRAMS;
-               if (programs[idx].instruction_base_address == gtt_offset) {
+               if (programs[i].instruction_base_address == gtt_offset) {
                     printf("\n%s (specified by %s at batch offset "
                            "0x%08"PRIx64") at offset 0x%08"PRIx64"\n",
-                           programs[idx].type,
-                           programs[idx].command,
-                           programs[idx].command_offset,
-                           programs[idx].ksp);
-                    gen_disasm_disassemble(disasm, data, programs[idx].ksp,
+                           programs[i].type,
+                           programs[i].command,
+                           programs[i].command_offset,
+                           programs[i].ksp);
+                    gen_disasm_disassemble(disasm, data, programs[i].ksp,
                                            stdout);
                }
             }

Comments

:(

The intention was to deal with cases where we've encountered more than 
MAX_NUM_PROGRAMS (actually happened to me).
So we start by the index + 1 assuming this is the oldest program because 
we're in a rolling window of programs.

This is obviously broken with the < 4096 programs case. Maybe we should 
just replace idx with :

idx = ((num_programs == MAX_NUM_PROGRAMS ? idx_program : 0) + i) % 
MAX_NUM_PROGRAMS;

On 11/11/17 00:55, Kenneth Graunke wrote:
> This indexing is bogus.  idx_program is the offset of the next program.
> At this point, we've walked through the entire batch, and accumulated
> all the programs.  So adding it again simply skips over 100% of the
> programs.
> ---
>   src/intel/tools/aubinator_error_decode.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
> index 2322bac8391..00e1a872bd0 100644
> --- a/src/intel/tools/aubinator_error_decode.c
> +++ b/src/intel/tools/aubinator_error_decode.c
> @@ -601,15 +601,14 @@ read_data_file(FILE *file)
>               printf("Disassembly of programs in instruction buffer at "
>                      "0x%08"PRIx64":\n", gtt_offset);
>               for (int i = 0; i < num_programs; i++) {
> -               int idx = (idx_program + i) % MAX_NUM_PROGRAMS;
> -               if (programs[idx].instruction_base_address == gtt_offset) {
> +               if (programs[i].instruction_base_address == gtt_offset) {
>                       printf("\n%s (specified by %s at batch offset "
>                              "0x%08"PRIx64") at offset 0x%08"PRIx64"\n",
> -                           programs[idx].type,
> -                           programs[idx].command,
> -                           programs[idx].command_offset,
> -                           programs[idx].ksp);
> -                    gen_disasm_disassemble(disasm, data, programs[idx].ksp,
> +                           programs[i].type,
> +                           programs[i].command,
> +                           programs[i].command_offset,
> +                           programs[i].ksp);
> +                    gen_disasm_disassemble(disasm, data, programs[i].ksp,
>                                              stdout);
>                  }
>               }
On Saturday, November 11, 2017 3:35:21 PM PST Lionel Landwerlin wrote:
> :(
> 
> The intention was to deal with cases where we've encountered more than 
> MAX_NUM_PROGRAMS (actually happened to me).
> So we start by the index + 1 assuming this is the oldest program because 
> we're in a rolling window of programs.
> 
> This is obviously broken with the < 4096 programs case. Maybe we should 
> just replace idx with :
> 
> idx = ((num_programs == MAX_NUM_PROGRAMS ? idx_program : 0) + i) % 
> MAX_NUM_PROGRAMS;

Okay, let's drop this patch.  I just sent an 11 patch series which
includes patches to drop the arbitrary 4096 limit altogether, and
decodes shaders near the packets, like regular aubinator does.

That should work out better in both cases :)

--Ken