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

[Emil Velikov]

Hi Rob,

Pardon there, I've missed a couple of bits/wasn't clear enough:

On 1 March 2017 at 17:49, Robert Bragg <robert at sixbynine.org> wrote:


> +BUILT_SOURCES += \
> +       brw_oa_hsw.h \
> +       brw_oa_hsw.c
> +
By bad here - forgot to mention:
Please add these two to a i965_OA_GENERATED_FILES (or similar)
variable in Makefile.sources. And then:

-libi965_dri_la_SOURCES = $(i965_FILES)
+libi965_dri_la_SOURCES = \
+ $(i965_FILES) \
+ $(i965_OA_GENERATED_FILES)

+BUILT_SOURCES += $(i965_OA_GENERATED_FILES)

> +brw_oa_hsw.h brw_oa_hsw.c: brw_oa_hsw.xml brw_oa.py Makefile.am
Should be:
brw_oa_hsw.h: 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
General note: please avoid using spaces for identation/alignment in
Makefiles - things can go funny. Rewrapping these to fill the full
width would be appreciated.


> +brw_oa_hsw.c: brw_oa_hsw.h
> 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 \
With the above suggestion - you can drop this.


> +
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("xml", help="XML description of metrics")
> +    parser.add_argument("--header", help="Header file to write")
> +    parser.add_argument("--code", help="C file to write")
> +    parser.add_argument("--chipset", help="Chipset to generate code for")
> +
I realise that we're not consistent, but we really want these as
required. Otherwise we'll get nasty python backtrace as opposed to a
nice error. Something like the following should do it.

parser.add_argument("--foo", help="bar", required=True)


> +
> +    h(copyright)
> +    h(textwrap.dedent("""\
> +        #pragma once
Please change to ifndef guards - be that here or as a follow-up commit.

#ifndef FOO_H
#define FOO_H
...

#endif FOO_H


Thanks
Emil