[Mesa-dev] [PATCH 2/3] i965: we want 64bit writes for depth count

[Kenneth Graunke]

On 06/26/2012 07:28 AM, Daniel Vetter wrote:
> ... and the hardware seems to take the lenght of the pipe control
> command to indicate whether the write is 64bit or 32bit. Which makes
> sense for immediate writes.
> 
> I've discovered this by writing a pattern into the query object bo and
> noticing that the high 32bits are left intact, even on those pipe
> control writes that seemingly worked.

I can't find any documentation or other justification for this, but if
you think it's useful, adding the extra 0 shouldn't hurt anything.

Minor bikeshed below.

> ---
>  src/mesa/drivers/dri/i965/brw_queryobj.c |   10 ++++++----
>  src/mesa/drivers/dri/intel/intel_reg.h   |    1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c b/src/mesa/drivers/dri/i965/brw_queryobj.c
> index d7870d1..dae7af0 100644
> --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> @@ -333,7 +333,7 @@ brw_emit_query_begin(struct brw_context *brw)
>        return;
>  
>     if (intel->gen >= 6) {
> -       BEGIN_BATCH(12);
> +       BEGIN_BATCH(13);
>  
>         /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
>  	* needs a pipe control with CS_STALL set beforehand.
> @@ -346,13 +346,14 @@ brw_emit_query_begin(struct brw_context *brw)
>         OUT_BATCH(0);
>  
>         /* The actual DEPTH_COUNT write. */
> -       OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +       OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);

I would really prefer this to be

       OUT_BATCH(_3DSTATE_PIPE_CONTROL | (5 - 2));

since this is what we do with every other state packet command.  I'm not
a fan of PIPE_CONTROL being the only command with a length baked into
the define.  (That said, I'm not suggesting removing the baked length
right now...but doing this will result in | 2 | 3 which is fine, since 3
includes all the bits for 2 anyway.)

Otherwise I have to wonder what a PIPE_CONTROL_5 is.

>         OUT_BATCH(PIPE_CONTROL_WRITE_DEPTH_COUNT);
>         OUT_RELOC(brw->query.bo,
>  	         I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  		 PIPE_CONTROL_GLOBAL_GTT_WRITE |
>  		 ((brw->query.index * 2) * sizeof(uint64_t)));
>         OUT_BATCH(0);
> +       OUT_BATCH(0);
>  
>         /* We need to emit depth stall to get the right value for the depth
>  	* count. As a workaround this needs a preceeding pipe control with a
> @@ -403,7 +404,7 @@ brw_emit_query_end(struct brw_context *brw)
>        return;
>  
>     if (intel->gen >= 6) {
> -       BEGIN_BATCH(12);
> +       BEGIN_BATCH(13);
>  
>         /* Workaround: A non-zero post-sync op (i.e. the DEPTH_COUNT write below
>  	* needs a pipe control with CS_STALL set beforehand.
> @@ -416,13 +417,14 @@ brw_emit_query_end(struct brw_context *brw)
>         OUT_BATCH(0);
>  
>         /* The actual DEPTH_COUNT write. */
> -       OUT_BATCH(_3DSTATE_PIPE_CONTROL);
> +       OUT_BATCH(_3DSTATE_PIPE_CONTROL_5);

ditto

>         OUT_BATCH(PIPE_CONTROL_WRITE_DEPTH_COUNT);
>         OUT_RELOC(brw->query.bo,
>  	         I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  		 PIPE_CONTROL_GLOBAL_GTT_WRITE |
>  		 ((brw->query.index * 2 + 1) * sizeof(uint64_t)));
>         OUT_BATCH(0);
> +       OUT_BATCH(0);
>  
>         /* We need to emit depth stall to get the right value for the depth
>  	* count. As a workaround this needs a preceeding pipe control with a
> diff --git a/src/mesa/drivers/dri/intel/intel_reg.h b/src/mesa/drivers/dri/intel/intel_reg.h
> index e2a6ee2..04f7a8d 100644
> --- a/src/mesa/drivers/dri/intel/intel_reg.h
> +++ b/src/mesa/drivers/dri/intel/intel_reg.h
> @@ -59,6 +59,7 @@
>   * additional flushing control.
>   */
>  #define _3DSTATE_PIPE_CONTROL		(CMD_3D | (3 << 27) | (2 << 24) | 2)
> +#define _3DSTATE_PIPE_CONTROL_5		(CMD_3D | (3 << 27) | (2 << 24) | 3)
>  #define PIPE_CONTROL_CS_STALL		(1 << 20)
>  #define PIPE_CONTROL_GLOBAL_SNAPSHOT_COUNT_RESET	(1 << 19)
>  #define PIPE_CONTROL_TLB_INVALIDATE	(1 << 18)
> 

and then no need to add this.

With that change, this is:
Acked-by: Kenneth Graunke <kenneth at whitecape.org>

Though, again, I'm not sure why it'd be necessary.