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

2012-02-11 Thread 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 Versacechad.vers...@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();
 }

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


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

2012-02-10 Thread Chad Versace
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.

Signed-off-by: Chad Versace chad.vers...@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));
+
+  /* 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();
}
 
-- 
1.7.7.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev