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

[Ilia Mirkin]

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.

> - 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

> - 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

>  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