[Mesa-dev] [PATCH 1/2] aubinator: Add a new tool called Aubinator to the src/intel/tools folder.
Gandikota, Sirisha
sirisha.gandikota at intel.com
Mon Aug 15 17:47:18 UTC 2016
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.velikov at gmail.com]
Sent: Monday, August 15, 2016 5:41 AM
To: Gandikota, Sirisha <sirisha.gandikota at intel.com>
Cc: ML mesa-dev <mesa-dev at 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.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
More information about the mesa-dev
mailing list