Thanks Emil/Jason/Ken/Matt/Kristian... for your time and review comments. Noted all the suggestions and I'll send out the updated patches soon.
-Sirisha -----Original Message----- From: Emil Velikov [mailto:emil.l.veli...@gmail.com] Sent: Monday, August 15, 2016 5:41 AM To: Gandikota, Sirisha <sirisha.gandik...@intel.com> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org> Subject: Re: [Mesa-dev] [PATCH 1/2] aubinator: Add a new tool called Aubinator to the src/intel/tools folder. 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.gandik...@intel.com> wrote: > From: Kristian Høgsberg Kristensen <k...@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.lesp...@intel.com> and Kristian Høgsberg Kristensen > <k...@bitplanet.net>. > > Signed-off-by: Sirisha Gandikota <sirisha.gandik...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
