[Mesa-dev] [PATCH] i965/gen6: In HiZ op, emit valid pointers in 3DSTATE_CC_STATE_POINTERS

[Kenneth Graunke]

On 02/10/2012 10:54 AM, Chad Versace wrote:
> Before this patch, the HiZ op was setting the pointers to COLOR_CALC_STATE
> and to BLEND_STATE to 0. This was probably safe, since the HiZ op doesn't
> use the cc or the blending. And it caused no problems with Piglit and
> Citybench.
>
> But, we don't know exactly what the GPU does with those pointers. So, to
> be really safe, this patches replaces the 0's with valid pointers.
>
> There is no need to do the analogous change for gen7 because a separate
> packet is emitted for each of DEPTH_STENCIL_STATE, COLOR_CALC_STATE, and
> BLEND_STATE. In gen7_hiz_exec, we emit only
> 3DSTATE_DEPTH_STENCIL_STATE_POINTERS.

Actually, this is inconsistent.  If you don't send 
3DSTATE_CC_STATE_POINTERS and 3DSTATE_BLEND_STATE_POINTERS, you're just 
leaving them programmed in their existing state.  You can do the same 
thing on Gen6 too: just don't set the "BLEND_STATE Change" and 
"DEPTH_STENCIL_STATE Change" bits.

If the GPU does use those pointers, it doesn't seem safe to leave them 
at their own value: you flush the batch when starting a HiZ op, so the 
old pointers will still be offsets from the start of the old batch...but 
misinterpreted, added to the start of the new base address.

If the GPU doesn't use those pointers, all this is moot.  The fact that 
this hasn't died in a fire on either platform suggests that it doesn't. 
  But it'd be nice to know for sure.

> Signed-off-by: Chad Versace<chad.versace at linux.intel.com>
> ---
>   src/mesa/drivers/dri/i965/gen6_hiz.c |   24 +++++++++++++++++++++---
>   1 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen6_hiz.c b/src/mesa/drivers/dri/i965/gen6_hiz.c
> index a86c147..5c1844e 100644
> --- a/src/mesa/drivers/dri/i965/gen6_hiz.c
> +++ b/src/mesa/drivers/dri/i965/gen6_hiz.c
> @@ -327,14 +327,32 @@ gen6_hiz_exec(struct intel_context *intel,
>       * The HiZ op doesn't use BLEND_STATE or COLOR_CALC_STATE.
>       */
>      {
> +      uint32_t blend_offset;
>         uint32_t depthstencil_offset;
> +      uint32_t cc_offset;
> +
> +      struct gen6_blend_state *blend_state;
> +      struct gen6_color_calc_state *cc_state;
> +
> +      /* Disable everything: color blending, alpha blending, and dithering. */
> +      blend_state = brw_state_batch(brw, AUB_TRACE_BLEND_STATE,
> +                                    sizeof(*blend_state), 64,
> +&blend_offset);
> +      memset(blend_state, 0, sizeof(*blend_state));

Technically, a Source Alpha Blend Factor of 0 is marked as Reserved.

That seems like slightly more evidence that it doesn't use these 
buffers.  Slightly.

> +      /* The HiZ op doesn't use the CC, so just zero-fill the state. */
> +      cc_state = brw_state_batch(brw, AUB_TRACE_CC_STATE,
> +                                 sizeof(*cc_state), 64,
> +&cc_offset);
> +      memset(cc_state, 0, sizeof(*cc_state));
> +
>         gen6_hiz_emit_depth_stencil_state(brw, op,&depthstencil_offset);
>
>         BEGIN_BATCH(4);
>         OUT_BATCH(_3DSTATE_CC_STATE_POINTERS<<  16 | (4 - 2));
> -      OUT_BATCH(1); /* BLEND_STATE offset */
> -      OUT_BATCH(depthstencil_offset | 1); /* DEPTH_STENCIL_STATE offset */
> -      OUT_BATCH(1); /* COLOR_CALC_STATE offset */
> +      OUT_BATCH(blend_offset | 1);
> +      OUT_BATCH(depthstencil_offset | 1);
> +      OUT_BATCH(cc_offset | 1);
>         ADVANCE_BATCH();
>      }