[Mesa-dev] [PATCH 3/3] i965: adjust gen6+ timestamp pipe_control writes

[Kenneth Graunke]

On 06/26/2012 07:28 AM, Daniel Vetter wrote:
> Similar treatment to the depth count pipe_control writes
> - Add the CS_STALL workaround, timestamp writes are non-zero post-sync
>   ops, too.
> - Also ensure that we write the full 64bits by using the 5 dword long
>   variant of pipe_control.
> ---
>  src/mesa/drivers/dri/i965/brw_queryobj.c |   32 ++++++++++++++++++++++++++---
>  1 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c b/src/mesa/drivers/dri/i965/brw_queryobj.c
> index dae7af0..36de43e 100644
> --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> @@ -131,14 +131,26 @@ brw_begin_query(struct gl_context *ctx, struct gl_query_object *q)
>  				     4096, 4096);
>  
>        if (intel->gen >= 6) {
> -	  BEGIN_BATCH(4);
> -	  OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +	  BEGIN_BATCH(9);
> +
> +          /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
> +           * needs a pipe control with CS_STALL set beforehand.
> +           * Workaround: CS_STALL can't be set alone, we pick STALL_AT_SCOREBOARD
> +           * like the kernel. */

It's a timestamp write here, not depth count. :)  I would actually
prefer a comment along the lines of:

/* Workaround: A non-zero post-sync op (i.e. the timestamp write below)
 * must be preceded by a CS stall.  See rationale in the comments for
 * intel_emit_post_sync_nonzero_flush().
 */

The workaround chain is pretty well explained in the comments on that
function, and so may be more helpful than "like the kernel."

As for the substance of the patch (actually adding the CS stall), that
seems reasonable to me.  Odd though.  Normally failing to do this
results in GPU hangs.  Haven't seen any of those.  Still.

> +          OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +          OUT_BATCH(PIPE_CONTROL_CS_STALL |
> +                    PIPE_CONTROL_STALL_AT_SCOREBOARD);
> +          OUT_BATCH(0);
> +          OUT_BATCH(0);
> +
> +	  OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);

OUT_BATCH(_3DSTATE_PIPE_CONTROL | (5 - 2))

>  	  OUT_BATCH(PIPE_CONTROL_WRITE_TIMESTAMP);
>  	  OUT_RELOC(query->bo,
>  		  I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  		  PIPE_CONTROL_GLOBAL_GTT_WRITE |
>  		  0);
>  	  OUT_BATCH(0);
> +	  OUT_BATCH(0);
>  	  ADVANCE_BATCH();
>        
>        } else {
> @@ -199,14 +211,26 @@ brw_end_query(struct gl_context *ctx, struct gl_query_object *q)
>     switch (query->Base.Target) {
>     case GL_TIME_ELAPSED_EXT:
>        if (intel->gen >= 6) {
> -	  BEGIN_BATCH(4);
> -	  OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +	  BEGIN_BATCH(9);
> +
> +          /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
> +           * needs a pipe control with CS_STALL set beforehand.
> +           * Workaround: CS_STALL can't be set alone, we pick STALL_AT_SCOREBOARD
> +           * like the kernel. */

ditto (comment)

> +          OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +          OUT_BATCH(PIPE_CONTROL_CS_STALL |
> +                    PIPE_CONTROL_STALL_AT_SCOREBOARD);
> +          OUT_BATCH(0);
> +          OUT_BATCH(0);
> +
> +	  OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);

ditto (5 - 2)

>  	  OUT_BATCH(PIPE_CONTROL_WRITE_TIMESTAMP);
>  	  OUT_RELOC(query->bo,
>  		  I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  		  PIPE_CONTROL_GLOBAL_GTT_WRITE |
>  		  8);
>  	  OUT_BATCH(0);
> +	  OUT_BATCH(0);
>  	  ADVANCE_BATCH();
>        
>        } else {

With those changes,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>