On Fri, Feb 24, 2017 at 2:50 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> 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 <rob...@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.
Ah, can you maybe give me a bit more of a hint about what I'm breaking
for Android? Just extra work running python scripts as part of an
Android build I guess and requiring some change to
./src/mesa/drivers/dri/i965/Android.gen.mk?
>
>> Signed-off-by: Robert Bragg <rob...@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
as one issue, I can see that brw_oa_hsw.h isn't declared as a
prerequisite for compiling brw_oa_hsw.c, am I missing more issues?
Based on that I'd imagine adding:
brw_oa_hsw.c: brw_oa_hsw.h
as a way to ensure we can't attempt to compile brw_oa_hsw.c before
brw_oa_hsw.h is generated.
Is there a reason for removing the Makefile prerequisite? If I were to
mess with the arguments passed to the script in the codegen rules I'd
like a Makefile change to result in re-running those rules.
I just noticed that I should also be using += for BUILT_SOURCES here, oops.
> ...
>
>> + $(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 ?
yeah, can do. I suppose it could be considered a hint that it requires
python2, but just checking whether the script really depends on
python2 if I avoid xrange then the script actually runs fine with
either version.
>
>> +
>> +if args.header:
>> + header_file = open(args.header, 'w')
>> +
>
>> +
>> +h(copyright)
>> +h("""#pragma once
> Use proper ifndef guards please.
Oh, I really like #pragma once though :-)
It looks like we already assume #pragma once is supported by all the
compilers we care about. git grep '#pragma once'|wc -l shows 107
existing uses with 9 of those under /dri/i965/
Is it ok to keep considering that it seems to already be in quite wide
use within mesa?
Thanks for the comments,
- Robert
>
> Thanks
> Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
