[Mesa-dev] [PATCH] i965: implement ARB_pipeline_statistics_query
Kenneth Graunke
kenneth at whitecape.org
Tue Feb 17 22:00:05 PST 2015
On Tuesday, February 17, 2015 04:26:19 PM Ben Widawsky wrote:
> NOTE: The implementation was initially one patch, this. All the history is kept
> here, even though all the core mesa changes were moved to the parent of this
> patch.
>
> 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:
>
> > piglit-run.py -t stats tests/all.py output/pipeline_stats
> > [5/5] pass: 5 Running Test(s): 5
>
> v2:
> - Don't allow pipeline_stats to be per stream (Ilia). This may (not sure) 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.
> - Remove comment from GL3.txt because it is only used for extensions that are
> part of required versions (Ilia)
> - 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)
>
> v3:
> - Add ARB_pipeline_statistics_query to relnotes.html
> - Add ARB_pipeline_statistics_query.xml to the Makefile.am, and master XML (Ilia)
> - Correct extension number (Ilia)
> - Add link to xml in the main GL API xml (Ilia)
> - remove special GS case from gen6_end_query (Ian)
> - Make lookup table static so gcc doesn't initialized it on every call (Ian)
> - Use if (_mesa_has_geometry_shaders(ctx)) instead of explicit checks (Ian)
> - Core mesa parts moved into a prep patch (Ilia)
>
> v4:
> - Change to 10.6 relnotes since we missed 10.5 window
> - Moved compute shader stuff into the switch statement (Jordan)
> - Jordan: Add compute shader support
>
> v5:
> - Fixed relnote styles (Ilia)
>
> Cc: Ian Romanick <idr at freedesktop.org>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> docs/relnotes/10.6.0.html | 4 +
> src/mesa/drivers/dri/i965/gen6_queryobj.c | 117 +++++++++++++++++++++++++++
> src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
> src/mesa/drivers/dri/i965/intel_reg.h | 1 +
> 4 files changed, 123 insertions(+)
>
> diff --git a/docs/relnotes/10.6.0.html b/docs/relnotes/10.6.0.html
> index 10e926a..ee4c9c5 100644
> --- a/docs/relnotes/10.6.0.html
> +++ b/docs/relnotes/10.6.0.html
> @@ -43,6 +43,10 @@ TBD.
> Note: some of the new features are only available with certain drivers.
> </p>
>
> +<ul>
> +<li>GL_ARB_pipeline_statistics_query on i965</li>
> +<ul>
Still invalid HTML - you want:
<ul>
<li>GL_ARB_pipeline_statistics_query on i965</li>
</ul>
> +
> TBD.
>
> <h2>Bug fixes</h2>
> diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> index de71bb5..d28cb5d 100644
> --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> @@ -109,6 +109,74 @@ write_xfb_primitives_written(struct brw_context *brw,
> }
> }
>
> +static inline const int
> +pipeline_target_to_index(int target)
> +{
> + if (target == GL_GEOMETRY_SHADER_INVOCATIONS)
> + return MAX_PIPELINE_STATISTICS - 1;
> + else
> + return target - GL_VERTICES_SUBMITTED_ARB;
> +}
> +
> +static void
> +emit_pipeline_stat(struct brw_context *brw, drm_intel_bo *bo,
> + int stream, int target, int idx)
> +{
vvvvvv bonus blank line
> + /*
> + * There are 2 confusing parts to implementing the various target. The first is
> + * the distinction between vertices submitted and primitives submitted. The
> + * spec tries to clear this up, and luckily our hardware seems to understand
> + * it:
> + *
> + * (8) What stage the VERTICES_SUBMITTED_ARB and PRIMITIVES_SUBMITTED_ARB
> + * belong to? What do they count?
> + *
> + * DISCUSSION: There is no separate pipeline stage introduced in the
> + * specification that matches D3D's "input assembler" stage. While the
> + * latest version of the GL specification mentions a "vertex puller" stage
> + * in the pipeline diagram, this stage does not have a corresponding chapter
> + * in the specification that introduces it.
> + *
> + * RESOLVED: Introduce VERTICES_SUBMITTED_ARB and PRIMITIVES_SUBMITTED_ARB
> + * in chapter 10, Vertex Specification and Drawing Command. They count the
> + * total number of vertices and primitives processed by the GL. Including
> + * multiple instances.
Now that you've figured it out, do you still think this is confusing,
and worth explaining in a comment? To me, it seems pretty clear: the
hardware's "Vertex Fetcher" (VF) stage corresponds to the DX "Input
Assembler" (IA) stage. I believe it's even called that in the hardware
documentation at one point. And...the distinction between vertices
submitted and primitives submitted seems pretty clear, too.
I'd personally remove it. If you still think it's valuable, then we can
certainly keep it.
> + *
> + * The second confusion is the tessellation shader statistics. Our hardware has
> + * no statistics specific to the TE unit. Ideally we could have the HS
> + * primitives for TESS_CONTROL_SHADER_PATCHES_ARB, and the DS invocations as
> + * the register for TESS_CONTROL_SHADER_PATCHES_ARB. Unfortunately we don't
> + * have HS primitives, we only have HS invocations.
> + */
This part seems worth keeping.
> +
> + /* Everything except GEOMETRY_SHADER_INVOCATIONS can be kept in a simple
> + * lookup table
> + */
> + static const uint32_t target_to_register[] = {
> + IA_VERTICES_COUNT, /* VERTICES_SUBMITTED */
> + IA_PRIMITIVES_COUNT, /* PRIMITIVES_SUBMITTED */
> + VS_INVOCATION_COUNT, /* VERTEX_SHADER_INVOCATIONS */
> + 0, /* HS_INVOCATION_COUNT,*/ /* TESS_CONTROL_SHADER_PATCHES */
> + 0, /* DS_INVOCATION_COUNT,*/ /* TESS_EVALUATION_SHADER_INVOCATIONS */
> + GS_PRIMITIVES_COUNT, /* GEOMETRY_SHADER_PRIMITIVES_EMITTED */
> + PS_INVOCATION_COUNT, /* FRAGMENT_SHADER_INVOCATIONS */
> + CS_INVOCATION_COUNT, /* COMPUTE_SHADER_INVOCATIONS */
> + CL_INVOCATION_COUNT, /* CLIPPING_INPUT_PRIMITIVES */
> + CL_PRIMITIVES_COUNT, /* CLIPPING_OUTPUT_PRIMITIVES */
> + GS_INVOCATION_COUNT /* This one is special... */
> + };
> + STATIC_ASSERT(ARRAY_SIZE(target_to_register) == MAX_PIPELINE_STATISTICS);
> + uint32_t reg = target_to_register[pipeline_target_to_index(target)];
> + assert(reg != 0);
> +
> + /* Emit a flush to make sure various parts of the pipeline are complete and
> + * we get an accurate value */
*/ on own line
> + intel_batchbuffer_emit_mi_flush(brw);
> +
> + brw_store_register_mem64(brw, bo, reg, idx);
> +}
> +
> +
> /**
> * Wait on the query object's BO and calculate the final result.
> */
> @@ -169,9 +237,29 @@ gen6_queryobj_get_results(struct gl_context *ctx,
>
> case GL_PRIMITIVES_GENERATED:
> case GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN:
> + case GL_VERTICES_SUBMITTED_ARB:
> + case GL_PRIMITIVES_SUBMITTED_ARB:
> + case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> + case GL_GEOMETRY_SHADER_INVOCATIONS:
> + case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> + case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> + case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> + case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> query->Base.Result = results[1] - results[0];
> break;
>
> + case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> + query->Base.Result = (results[1] - results[0]);
> + /* HW reports this count 4X the actual value and therefore SW must divide
> + * the count by 4 for correct reporting.
> + * XXX: Not in public docs?
> + */
This sounds like text from the documentation - if so, I'd put it in
quotes. Or perhaps use the workaround name:
/* Implement the "WaDividePSInvocationCountBy4:HSW,BDW" workaround:
* "Invocation counter is 4 times actual. WA: SW to divide HW reported
* PS Invocations value by 4."
*/
> + if (brw->gen >= 8 || brw->is_haswell)
> + query->Base.Result /= 4;
> + break;
> +
> + case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> + case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> default:
> unreachable("Unrecognized query target in brw_queryobj_get_results()");
> }
> @@ -240,6 +328,20 @@ gen6_begin_query(struct gl_context *ctx, struct gl_query_object *q)
> write_xfb_primitives_written(brw, query->bo, query->Base.Stream, 0);
> break;
>
> + case GL_VERTICES_SUBMITTED_ARB:
> + case GL_PRIMITIVES_SUBMITTED_ARB:
> + case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> + case GL_GEOMETRY_SHADER_INVOCATIONS:
> + case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> + case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> + case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> + case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> + case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> + emit_pipeline_stat(brw, query->bo, query->Base.Stream, query->Base.Target, 0);
> + break;
> +
> + case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> + case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
> default:
> unreachable("Unrecognized query target in brw_begin_query()");
> }
It's a little weird that you have the tessellation ones in the
unreachable case here..........
> @@ -278,6 +380,21 @@ gen6_end_query(struct gl_context *ctx, struct gl_query_object *q)
> write_xfb_primitives_written(brw, query->bo, query->Base.Stream, 1);
> break;
>
> + case GL_VERTICES_SUBMITTED_ARB:
> + case GL_PRIMITIVES_SUBMITTED_ARB:
> + case GL_VERTEX_SHADER_INVOCATIONS_ARB:
> + case GL_TESS_CONTROL_SHADER_PATCHES_ARB:
> + case GL_TESS_EVALUATION_SHADER_INVOCATIONS_ARB:
...but in the emit_pipeline_stat case here. Consistency would be nice.
Either way is probably fine.
> + case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED_ARB:
> + case GL_FRAGMENT_SHADER_INVOCATIONS_ARB:
> + case GL_COMPUTE_SHADER_INVOCATIONS_ARB:
> + case GL_CLIPPING_INPUT_PRIMITIVES_ARB:
> + case GL_CLIPPING_OUTPUT_PRIMITIVES_ARB:
> + case GL_GEOMETRY_SHADER_INVOCATIONS:
> + emit_pipeline_stat(brw, query->bo,
> + query->Base.Stream, query->Base.Target, 1);
> + break;
> +
> default:
> unreachable("Unrecognized query target in brw_end_query()");
> }
> diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c b/src/mesa/drivers/dri/i965/intel_extensions.c
> index 4dacfd0..608bfac9 100644
> --- a/src/mesa/drivers/dri/i965/intel_extensions.c
> +++ b/src/mesa/drivers/dri/i965/intel_extensions.c
> @@ -198,6 +198,7 @@ intelInitExtensions(struct gl_context *ctx)
> ctx->Extensions.ARB_map_buffer_range = true;
> ctx->Extensions.ARB_occlusion_query = true;
> ctx->Extensions.ARB_occlusion_query2 = true;
> + ctx->Extensions.ARB_pipeline_statistics_query = true;
> ctx->Extensions.ARB_point_sprite = true;
> ctx->Extensions.ARB_seamless_cube_map = true;
> ctx->Extensions.ARB_shader_bit_encoding = true;
> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h b/src/mesa/drivers/dri/i965/intel_reg.h
> index 8b630c5..af1c1df 100644
> --- a/src/mesa/drivers/dri/i965/intel_reg.h
> +++ b/src/mesa/drivers/dri/i965/intel_reg.h
> @@ -112,6 +112,7 @@
> #define CL_INVOCATION_COUNT 0x2338
> #define CL_PRIMITIVES_COUNT 0x2340
> #define PS_INVOCATION_COUNT 0x2348
> +#define CS_INVOCATION_COUNT 0x2290
> #define PS_DEPTH_COUNT 0x2350
>
> #define GEN6_SO_PRIM_STORAGE_NEEDED 0x2280
>
With the HTML fixed, this is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150217/1277db0f/attachment-0001.sig>
More information about the mesa-dev
mailing list