[Mesa-dev] [PATCH 2/3] intel/gen_decoder: return -1 for unknown command formats

[Lionel Landwerlin]

Sounds like a good strategy :)

Acked-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

On 04/04/17 00:22, Jordan Justen wrote:
> Decoding with aubinator encountered a command of 0xffffffff. With the
> previous code, it caused aubinator to jump 255 + 2 dwords to start
> decoding again.
>
> Instead we can attempt to detect the known instruction formats. If the
> format is not recognized, then we can advance just 1 dword.
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>   src/intel/common/gen_decoder.c                | 21 +++++++++++++++------
>   src/intel/tools/aubinator.c                   |  4 ++--
>   src/mesa/drivers/dri/i965/intel_batchbuffer.c |  2 ++
>   3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
> index 1244f4c4480..d5ba3e6265e 100644
> --- a/src/intel/common/gen_decoder.c
> +++ b/src/intel/common/gen_decoder.c
> @@ -692,28 +692,37 @@ gen_group_get_length(struct gen_group *group, const uint32_t *p)
>   
>      case 3: /* Render */ {
>         uint32_t subtype = field(h, 27, 28);
> +      uint32_t opcode = field(h, 24, 26);
>         switch (subtype) {
>         case 0:
> -         return field(h, 0, 7) + 2;
> +         if (opcode < 2)
> +            return field(h, 0, 7) + 2;
> +         else
> +            return -1;
>         case 1:
> -         return 1;
> +         if (opcode < 2)
> +            return 1;
> +         else
> +            return -1;
>         case 2: {
> -         uint32_t opcode = field(h, 24, 26);
> -         assert(opcode < 3 /* 3 and above currently reserved */);
>            if (opcode == 0)
>               return field(h, 0, 7) + 2;
>            else if (opcode < 3)
>               return field(h, 0, 15) + 2;
>            else
> -            return 1; /* FIXME: if more opcodes are added */
> +            return -1;
>         }
>         case 3:
> -         return field(h, 0, 7) + 2;
> +         if (opcode < 4)
> +            return field(h, 0, 7) + 2;
> +         else
> +            return -1;
>         }
>      }
>      }
>   
>      unreachable("bad opcode");
> +   return -1;
>   }
>   
>   void
> diff --git a/src/intel/tools/aubinator.c b/src/intel/tools/aubinator.c
> index cae578babac..a64bce7a536 100644
> --- a/src/intel/tools/aubinator.c
> +++ b/src/intel/tools/aubinator.c
> @@ -687,12 +687,12 @@ parse_commands(struct gen_spec *spec, uint32_t *cmds, int size, int engine)
>   
>      for (p = cmds; p < end; p += length) {
>         inst = gen_spec_find_instruction(spec, p);
> +      length = gen_group_get_length(inst, p);
>         if (inst == NULL) {
>            fprintf(outfile, "unknown instruction %08x\n", p[0]);
> -         length = (p[0] & 0xff) + 2;
> +         length = MIN2(1, length);
>            continue;
>         }
> -      length = gen_group_get_length(inst, p);
>   
>         const char *color, *reset_color = NORMAL;
>         uint64_t offset;
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 54bab9efb02..d16b4622456 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -318,6 +318,8 @@ do_batch_dump(struct brw_context *brw)
>         }
>   
>         length = gen_group_get_length(inst, p);
> +      assert(length > 0);
> +      length = MAX2(length, 1);
>      }
>   
>      if (ret == 0) {