[Mesa-dev] [PATCH 1/2] aubinator: Add a new tool called Aubinator to the src/intel/tools folder.

[Emil Velikov]

Hi Sirisha,

A few misc suggestions all over on top of Matt's input. Please don't
read too much into them ;-)

On 10 August 2016 at 00:52, Sirisha Gandikota
<sirisha.gandikota at intel.com> wrote:
> From: Kristian Høgsberg Kristensen <krh at bitplanet.net>
>
> The Aubinator tool is designed to help the driver developers in debugging
> the driver functionality by decoding the data in the .aub files.
> Primary Authors of this tool are Damien Lespiau <damien.lespiau at intel.com>
> and Kristian Høgsberg Kristensen <krh at bitplanet.net>.
>
> Signed-off-by: Sirisha Gandikota <Sirisha.Gandikota at intel.com>
> ---
>  configure.ac                 |    1 +
>  src/Makefile.am              |    4 +
>  src/intel/tools/Makefile.am  |   65 +++
>  src/intel/tools/aubinator.c  | 1039 ++++++++++++++++++++++++++++++++++++++++++
>  src/intel/tools/decoder.c    |  520 +++++++++++++++++++++
>  src/intel/tools/decoder.h    |   57 +++
>  src/intel/tools/disasm.c     |  109 +++++
>  src/intel/tools/gen_disasm.h |   35 ++
>  src/intel/tools/intel_aub.h  |  154 +++++++
>  9 files changed, 1984 insertions(+)
>  create mode 100644 src/intel/tools/Makefile.am
>  create mode 100644 src/intel/tools/aubinator.c
>  create mode 100644 src/intel/tools/decoder.c
>  create mode 100644 src/intel/tools/decoder.h
>  create mode 100644 src/intel/tools/disasm.c
>  create mode 100644 src/intel/tools/gen_disasm.h
>  create mode 100644 src/intel/tools/intel_aub.h
>

> --- /dev/null
> +++ b/src/intel/tools/Makefile.am

> +CLEANFILES = \
> +       aubinator-aubinator.o \
> +       aubinator-decoder.o \
> +       disasm.lo \
> +       libdisasm.la
> +
make clean should be able to sort the above file even without this
hunk, correct ?

> +noinst_LTLIBRARIES = libdisasm.la
> +
> +AM_CPPFLAGS = \
> +       $(INTEL_CFLAGS) \
> +       $(VALGRIND_CFLAGS) \
> +       $(DEFINES) \
> +       -I$(top_srcdir)/include \
> +       -I$(top_builddir)/src \
> +       -I$(top_srcdir)/src \
> +       -I$(top_builddir)/src/compiler \
> +       -I$(top_srcdir)/src/compiler \
> +       -I$(top_builddir)/src/compiler/nir \
> +       -I$(top_srcdir)/src/mapi \
> +       -I$(top_srcdir)/src/mesa \
> +       -I$(top_srcdir)/src/mesa/drivers/dri/common \
> +       -I$(top_srcdir)/src/mesa/drivers/dri/i965 \
> +       -I$(top_srcdir)/src/gallium/auxiliary \
> +       -I$(top_srcdir)/src/gallium/include \
> +       -I$(top_builddir)/src/intel \
> +       -I$(top_srcdir)/src/intel
> +
> +libdisasm_la_SOURCES = \
> +       gen_disasm.h \
> +       disasm.c
> +libdisasm_la_LIBADD = $(top_builddir)/src/mesa/drivers/dri/i965/libi965_compiler.la \
> +               $(top_builddir)/src/mesa/libmesa.la \
I believe that the i965 compiler no longer depends on libmesa (props
to Jason) thus this can go. Same goes for many of the includes in the
above list.

> +               $(PER_GEN_LIBS) \
> +               $(PTHREAD_LIBS) \
> +               $(DLOPEN_LIBS) \
> +               -lm
> +
Indentation is inconsistent - please use only tabs in makefiles.

> +
> +bin_PROGRAMS = aubinator
I second Matt here - don't think that installing this is good idea.
Especially since the headers that we depend on are only available in
the tarball. Please make this noinst_PROGRAMS.

> +
> +aubinator_SOURCES = intel_aub.h decoder.c decoder.h aubinator.c
> +aubinator_CFLAGS =  $(EXPAT_CFLAGS) $(AM_CFLAGS) -I$(top_srcdir)/src -I$(top_srcdir)/include
> +
> +aubinator_LDADD = $(EXPAT_LIBS) libdisasm.la
Please follow the format set by the begining of the file and split
these three into separate lines. Note: please keep things alphabetical
and place system libraries _after_ the local static ones.

Aside: if we have no planned (second) user of libdisasm.la one can
even build the whole thing in one go.

> diff --git a/src/intel/tools/aubinator.c b/src/intel/tools/aubinator.c
> new file mode 100644
> index 0000000..99d67a1
> --- /dev/null
> +++ b/src/intel/tools/aubinator.c

> +#define _GNU_SOURCE
> +
Please don't define any _*_SOURCE macros. The build already does it as needed.


> +static void
> +handle_3dstate_ps(struct gen_spec *spec, uint32_t *p)
> +{
> +   uint32_t mask = ~((1 << 6) - 1);
> +   uint64_t start;
> +   struct brw_instruction *insns;
> +   static const char unused[] = "unused";
Add the 8/16/32 pixel static const char[] and reuse them ?

> +   const char *k0, *k1, *k2;
> +   uint32_t k_mask, k1_offset, k2_offset;
> +
> +   if (gen_spec_get_gen(spec) >= gen_make_gen(8, 0)) {
> +      k_mask = p[6] & 7;
> +      k1_offset = 8;
> +      k2_offset = 10;
> +   } else {
> +      k_mask = p[4] & 7;
> +      k1_offset = 6;
> +      k2_offset = 7;
> +   }
> +
> +#define DISPATCH_8 1
> +#define DISPATCH_16 2
> +#define DISPATCH_32 4
> +
> +   switch (k_mask) {

> +   default:
The compiler should catch that this is unreachable thus we can just
drop the default case ?


> +struct custom_handler {
> +   uint32_t opcode;
> +   void (*handle)(struct gen_spec *spec, uint32_t *p);
> +} custom_handlers[] = {
Big(ish) hunks of const data should be static const ?

> +struct { const char *name; uint32_t gen; } device_map[] = {
As above - also please use the same coding style.


> +int main(int argc, char *argv[])
> +{
> +   struct gen_spec *spec;
> +   struct aub_file *file;
> +   int i, pci_id = 0;
> +   bool found_arg_gen = false, pager = true;
> +   int gen_major, gen_minor;
> +   const char *value;
> +   char gen_file[256];
> +
> +   for (i = 1; i < argc; ++i) {
> +      if (strcmp(argv[i], "--no-pager") == 0) {
[...]

> +      } else {
> +         break;
> +      }
> +   }
> +
> +   if (argv[i] == NULL) {
> +      print_help(stderr);
> +      exit(EXIT_FAILURE);
> +   }
> +
> +   if (argv[i][0] == '-') {
> +      fprintf(stderr, "unknown option %s\n", argv[i]);
> +      exit(EXIT_FAILURE);
Move this two in the final else/break. Also please don't access
argv[1] when argc == 1.


> +   while (aub_file_more_stuff(file))
> +      aub_file_decode_batch(file, spec);
> +
> +   fflush(stdout);
> +   close(1);
Why the "close(1)" ?

> --- /dev/null
> +++ b/src/intel/tools/decoder.c

> +#define _DEFAULT_SOURCE /* for strdup() */
Please remove this - the build sets it up for us.


> diff --git a/src/intel/tools/decoder.h b/src/intel/tools/decoder.h
> new file mode 100644
> index 0000000..af9e075
> --- /dev/null
> +++ b/src/intel/tools/decoder.h

> +#pragma once
For this and the other headers - please don't use pragma once, but a
proper ifndef guard(s).


> --- /dev/null
> +++ b/src/intel/tools/intel_aub.h

This file is available/provided by libdrm_intel. While the API has
been deprecated (since it was moved to IGT and now here) yet the
format will stay the same forever, correct ?
Can/should we reuse the libdrm_intel version ?

Thanks
Emil