[Mesa-dev] [PATCH] [v2] i965: implement ARB_pipeline_statistics_query

[Ben Widawsky]

On Tue, Dec 02, 2014 at 10:47:37PM -0500, Ilia Mirkin wrote:
> On Tue, Dec 2, 2014 at 9:33 PM, Ben Widawsky
> <benjamin.widawsky at intel.com> wrote:
> > This patch implements ARB_pipeline_statistics_query. This addition to GL does
> > not add a new API. Instead, it adds new tokens to the existing query APIs. The
> > work to hook up the new tokens is trivial due to it's similarity to the previous
> > work done for the query APIs. I've implemented all the new tokens to some
> > degree, but have stubbed out the untested ones at the entry point for Begin().
> > Doing this should allow the remainder of the code to be left in.
> >
> > The new tokens give GL clients a way to obtain stats about the GL pipeline.
> > Generally, you get the number of things going in, invocations, and number of
> > things coming out, primitives, of the various stages. There are two immediate
> > uses for this, performance information, and debugging various types of
> > misrendering. I doubt one can use these for debugging very complex applications,
> > but for piglit tests, it should be quite useful.
> >
> > Tessellation shaders, and compute shaders are not addressed in this patch
> > because there is no upstream implementation. I've implemented how I believe
> > tessellation shader stats will work for Intel hardware (though there is a bit of
> > ambiguity). Compute shaders are a bit more interesting though, and I don't yet
> > know what we'll do there.
> >
> > For the lazy, here is a link to the relevant part of the spec:
> > https://www.opengl.org/registry/specs/ARB/pipeline_statistics_query.txt
> >
> > Running the piglit tests
> > http://lists.freedesktop.org/archives/piglit/2014-November/013321.html
> > (http://cgit.freedesktop.org/~bwidawsk/piglit/log/?h=pipe_stats)
> > yield the following results:
> >
> >> python2 ./piglit-run.py -t stats tests/all.py output/pipeline_stats
> >> [5/5] pass: 5 Running Test(s): 5
> >
> > Previously I was seeing the adjacent vertex test failing on certain Intel
> > hardware. I am currently not able to reproduce this, and therefore for now, I'll
> > assume it was some transient issue which has been fixed.
> >
> > v2:
> > - Don't allow pipeline_stats to be per stream (Ilia). This would be needed for
> >   AMD_transform_feedback4, which we do not support.
> >    > If AMD_transform_feedback4 is supported then GEOMETRY_SHADER_PRIMITIVES_-
> >    > EMITTED_ARB counts primitives emitted to any of the vertex streams for which
> >    > STREAM_RASTERIZATION_AMD is enabled.
> 
> Actually it wouldn't be needed even if mesa supported AMD_tf4. The way
> I'm reading it, the counter should count primitives emitted to *ANY*
> vertex stream with rasterization, not keep separate counters per
> vertex stream.
> 

I'll defer to you on this since I really don't understand the implications of
TF4. I don't quite understand how "any" makes sense when there are multiple
vertex streams coming from the GS. So, I read it as, you need to capture all and
let the user of the API figure out what to do with it. Again, I'm more than
happy to defer to you on that.

> > - Remove comment from GL3.txt because it is only used for extensions that are
> >   part of required versions (Ilia)
> 
> You should still add it to relnotes though... the file's been created already

Sorry, you are correct. I was too hasty in sending this out.

> 
> > - Move the new tokens to a new XML doc instead of using the main GL4x.xml (Ilia)
> > - Add a fallthrough comment (Ilia)
> > - Only divide PS invocations by 4 on HSW+ (Ben)
> >
> > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  .../glapi/gen/ARB_pipeline_statistics_query.xml    |  24 ++++
> >  src/mesa/drivers/dri/i965/gen6_queryobj.c          | 121 +++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/intel_extensions.c       |   1 +
> 
> Didn't we agree that you'd split this into 2-3 patches, with the intel
> stuff in the last patch? Normally you'd have
> 
> patch 1: xml, queryobj stub, maybe include extension var + table
> entry, but can also be in patch 2.
> patch 2: queryobj impl, driver hook
> patch 3: driver impl
> 

Sorry, you are correct. Same excuse as above.

> >  src/mesa/main/config.h                             |   3 +
> >  src/mesa/main/extensions.c                         |   1 +
> >  src/mesa/main/mtypes.h                             |  15 +++
> >  src/mesa/main/queryobj.c                           |  77 +++++++++++++
> >  7 files changed, 242 insertions(+)
> >  create mode 100644 src/mapi/glapi/gen/ARB_pipeline_statistics_query.xml
> >
> > diff --git a/src/mapi/glapi/gen/ARB_pipeline_statistics_query.xml b/src/mapi/glapi/gen/ARB_pipeline_statistics_query.xml
> > new file mode 100644
> > index 0000000..db37267
> > --- /dev/null
> > +++ b/src/mapi/glapi/gen/ARB_pipeline_statistics_query.xml
> 
> This needs to be added to src/mapi/glapi/Makefile.am or similar --
> look for a giant list of xml files, add to that. You also need to
> include it in the master xml file. I wonder why you weren't having
> issues... maybe you're not adding functions so it works out, dunno.
> 
> [skipping all intel-specific stuff]
> 
> The rest seems reasonable.
> 
>   -ilia

Thanks for looking again. Hopefully I can get v3 out by the end of this week.
Juggling too much stuff atm.