[Mesa-dev] [PATCH 5/8] i965: Add script to gen code for OA counter queries

[Emil Velikov]

Hi Robert,

There's a few minor comments below. Feel free to address here or as
follow-up, if applicable.

On 24 February 2017 at 13:58, Robert Bragg <robert at sixbynine.org> wrote:
> Avoiding lots of error prone boilerplate and easing our ability to add +
> maintain support for multiple OA performance counter queries for each
> generation:
>
> This adds a python script to generate code for building up
> performance_queries from the metric sets and counters described in
> brw_oa_hsw.xml as well as functions to normalize each counter based on
> the RPN expressions given.
>
> Although the XML file currently only includes a single metric set, the
> code generated assumes there could be many sets.
>
> The metrics as described in XML get translated into C structures
> which are registered in a brw->perfquery.oa_metrics_table hash table
> keyed by the GUID of the metric set in XML.
>
Mauro, Tapani, we have another piece which will break Androids. Feel
free to send a fixup for Robert to squash.

> Signed-off-by: Robert Bragg <robert at sixbynine.org>
> ---
>  src/mesa/drivers/dri/i965/Makefile.am      |  15 +-
>  src/mesa/drivers/dri/i965/Makefile.sources |   2 +
>  src/mesa/drivers/dri/i965/brw_oa.py        | 543 +++++++++++++++++++++++++++++
>  3 files changed, 559 insertions(+), 1 deletion(-)
>  create mode 100644 src/mesa/drivers/dri/i965/brw_oa.py
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.am b/src/mesa/drivers/dri/i965/Makefile.am
> index f87fa67ef8..0130afff5f 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.am
> +++ b/src/mesa/drivers/dri/i965/Makefile.am
> @@ -93,7 +93,9 @@ BUILT_SOURCES = $(i965_compiler_GENERATED_FILES)
>  CLEANFILES = $(BUILT_SOURCES)
>
>  EXTRA_DIST = \
> -       brw_nir_trig_workarounds.py
> +       brw_nir_trig_workarounds.py \
> +       brw_oa_hsw.xml \
> +       brw_oa.py
>
>  TEST_LIBS = \
>         libi965_compiler.la \
> @@ -169,3 +171,14 @@ test_eu_validate_SOURCES = \
>  test_eu_validate_LDADD = \
>         $(top_builddir)/src/gtest/libgtest.la \
>         $(TEST_LIBS)
> +
> +BUILT_SOURCES = \
> +       brw_oa_hsw.h \
> +       brw_oa_hsw.c
> +
> +brw_oa_hsw.h brw_oa_hsw.c: brw_oa_hsw.xml brw_oa.py Makefile
Eric reminded me that this is a bit buggy/racy. Something like the
following should be better:

brw_oa_hsw.h: brw_oa_hsw.c
brw_oa_hsw.c: brw_oa_hsw.xml brw_oa.py
 ...

> +       $(PYTHON2) $(PYTHON_FLAGS) $(srcdir)/brw_oa.py \
> +          --header=$(builddir)/brw_oa_hsw.h \
> +          --code=$(builddir)/brw_oa_hsw.c \
> +          --chipset="hsw" \
> +          $(srcdir)/brw_oa_hsw.xml
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
> index 5278e86339..60acd15d41 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.sources
> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> @@ -135,6 +135,8 @@ i965_FILES = \
>         brw_nir_uniforms.cpp \
>         brw_object_purgeable.c \
>         brw_pipe_control.c \
> +       brw_oa_hsw.h \
> +       brw_oa_hsw.c \
H is after C ;-)

>         brw_performance_query.h \
>         brw_performance_query.c \
>         brw_program.c \
> diff --git a/src/mesa/drivers/dri/i965/brw_oa.py b/src/mesa/drivers/dri/i965/brw_oa.py

> new file mode 100644
> index 0000000000..2c622531af
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_oa.py
> @@ -0,0 +1,543 @@
> +#!/usr/bin/env python2
Thanks for omitting the execute bit !
Maybe drop this, since one should/can not execute the file directly ?

> +
> +if args.header:
> +    header_file = open(args.header, 'w')
> +

> +
> +h(copyright)
> +h("""#pragma once
Use proper ifndef guards please.

Thanks
Emil