[Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Kenneth Graunke 
Haswell moved the Cut Index Enable bit from the INDEX_BUFFER packet to
a new 3DSTATE_VF packet, so we need to emit that.  Also, it requires us
to specify the cut index rather than assuming it's 0x.

This adds a new Haswell-specific tracked state atom to gen7_atoms.
Normally, we would create a new generation-specific atom list, but since
there's only one difference over Ivybridge so far, I chose to simply
make it return without doing any work on non-Haswell systems.

Fixes five piglit tests:
- general/primitive-restart-DISABLE_VBO
- general/primitive-restart-VBO_COMBINED_VERTEX_AND_INDEX
- general/primitive-restart-VBO_INDEX_ONLY
- general/primitive-restart-VBO_SEPARATE_VERTEX_AND_INDEX
- general/primitive-restart-VBO_VERTEX_ONLY

Signed-off-by: Kenneth Graunke kenn...@whitecape.org
---
 src/mesa/drivers/dri/i965/brw_defines.h   |  3 ++
 src/mesa/drivers/dri/i965/brw_draw_upload.c   |  2 +-
 src/mesa/drivers/dri/i965/brw_primitive_restart.c | 36 +++
 src/mesa/drivers/dri/i965/brw_state.h |  1 +
 src/mesa/drivers/dri/i965/brw_state_upload.c  |  2 ++
 5 files changed, 43 insertions(+), 1 deletion(-)

I could have sworn I sent this out, but I can't find it in my inbox, so I
guess I must not have been connected to the internet at the time...oops.

diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
b/src/mesa/drivers/dri/i965/brw_defines.h
index 3605c18..6dc4707 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1037,6 +1037,9 @@ enum brw_message_target {
 # define GEN6_URB_GS_ENTRIES_SHIFT 8
 # define GEN6_URB_GS_SIZE_SHIFT0
 
+#define _3DSTATE_VF 0x780c /* GEN7.5+ */
+#define HSW_CUT_INDEX_ENABLE(1  8)
+
 #define _3DSTATE_URB_VS 0x7830 /* GEN7+ */
 #define _3DSTATE_URB_HS 0x7831 /* GEN7+ */
 #define _3DSTATE_URB_DS 0x7832 /* GEN7+ */
diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c 
b/src/mesa/drivers/dri/i965/brw_draw_upload.c
index e40c7d5..33cce8f 100644
--- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
+++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
@@ -930,7 +930,7 @@ static void brw_emit_index_buffer(struct brw_context *brw)
if (index_buffer == NULL)
   return;
 
-   if (brw-prim_restart.enable_cut_index) {
+   if (brw-prim_restart.enable_cut_index  !intel-is_haswell) {
   cut_index_setting = BRW_CUT_INDEX_ENABLE;
} else {
   cut_index_setting = 0;
diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c 
b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
index 02deba4..38b5243 100644
--- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
+++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
@@ -29,8 +29,11 @@
 #include main/bufferobj.h
 
 #include brw_context.h
+#include brw_defines.h
 #include brw_draw.h
 
+#include intel_batchbuffer.h
+
 /**
  * Check if the hardware's cut index support can handle the primitive
  * restart index value.
@@ -39,6 +42,12 @@ static bool
 can_cut_index_handle_restart_index(struct gl_context *ctx,
const struct _mesa_index_buffer *ib)
 {
+   struct intel_context *intel = intel_context(ctx);
+
+   /* Haswell supports an arbitrary cut index. */
+   if (intel-is_haswell)
+  return true;
+
bool cut_index_will_work;
 
switch (ib-type) {
@@ -176,3 +185,30 @@ brw_handle_primitive_restart(struct gl_context *ctx,
return GL_TRUE;
 }
 
+static void
+haswell_upload_cut_index(struct brw_context *brw)
+{
+   struct intel_context *intel = brw-intel;
+   struct gl_context *ctx = intel-ctx;
+
+   /* Don't trigger on Ivybridge */
+   if (!intel-is_haswell)
+  return;
+
+   const unsigned cut_index_setting =
+  ctx-Array.PrimitiveRestart ? HSW_CUT_INDEX_ENABLE : 0;
+
+   BEGIN_BATCH(2);
+   OUT_BATCH(_3DSTATE_VF  16 | cut_index_setting | (2 - 2));
+   OUT_BATCH(ctx-Array.RestartIndex);
+   ADVANCE_BATCH();
+}
+
+const struct brw_tracked_state haswell_cut_index = {
+   .dirty = {
+  .mesa  = _NEW_TRANSFORM,
+  .brw   = 0,
+  .cache = 0,
+   },
+   .emit = haswell_upload_cut_index,
+};
diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
b/src/mesa/drivers/dri/i965/brw_state.h
index a80ee86..99fa088 100644
--- a/src/mesa/drivers/dri/i965/brw_state.h
+++ b/src/mesa/drivers/dri/i965/brw_state.h
@@ -133,6 +133,7 @@ extern const struct brw_tracked_state gen7_wm_constants;
 extern const struct brw_tracked_state gen7_wm_constant_surface;
 extern const struct brw_tracked_state gen7_wm_state;
 extern const struct brw_tracked_state gen7_wm_surfaces;
+extern const struct brw_tracked_state haswell_cut_index;
 
 /* brw_misc_state.c */
 uint32_t
diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c 
b/src/mesa/drivers/dri/i965/brw_state_upload.c
index 7291988..ec0f765 100644
---

Re: [Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Paul Berry 
On 31 August 2012 00:22, Kenneth Graunke kenn...@whitecape.org wrote:

 Haswell moved the Cut Index Enable bit from the INDEX_BUFFER packet to
 a new 3DSTATE_VF packet, so we need to emit that.  Also, it requires us
 to specify the cut index rather than assuming it's 0x.

 This adds a new Haswell-specific tracked state atom to gen7_atoms.
 Normally, we would create a new generation-specific atom list, but since
 there's only one difference over Ivybridge so far, I chose to simply
 make it return without doing any work on non-Haswell systems.

 Fixes five piglit tests:
 - general/primitive-restart-DISABLE_VBO
 - general/primitive-restart-VBO_COMBINED_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_INDEX_ONLY
 - general/primitive-restart-VBO_SEPARATE_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_VERTEX_ONLY

 Signed-off-by: Kenneth Graunke kenn...@whitecape.org
 ---
  src/mesa/drivers/dri/i965/brw_defines.h   |  3 ++
  src/mesa/drivers/dri/i965/brw_draw_upload.c   |  2 +-
  src/mesa/drivers/dri/i965/brw_primitive_restart.c | 36
 +++
  src/mesa/drivers/dri/i965/brw_state.h |  1 +
  src/mesa/drivers/dri/i965/brw_state_upload.c  |  2 ++
  5 files changed, 43 insertions(+), 1 deletion(-)

 I could have sworn I sent this out, but I can't find it in my inbox, so I
 guess I must not have been connected to the internet at the time...oops.

 diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
 b/src/mesa/drivers/dri/i965/brw_defines.h
 index 3605c18..6dc4707 100644
 --- a/src/mesa/drivers/dri/i965/brw_defines.h
 +++ b/src/mesa/drivers/dri/i965/brw_defines.h
 @@ -1037,6 +1037,9 @@ enum brw_message_target {
  # define GEN6_URB_GS_ENTRIES_SHIFT 8
  # define GEN6_URB_GS_SIZE_SHIFT0

 +#define _3DSTATE_VF 0x780c /* GEN7.5+ */
 +#define HSW_CUT_INDEX_ENABLE(1  8)
 +
  #define _3DSTATE_URB_VS 0x7830 /* GEN7+ */
  #define _3DSTATE_URB_HS 0x7831 /* GEN7+ */
  #define _3DSTATE_URB_DS 0x7832 /* GEN7+ */
 diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c
 b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 index e40c7d5..33cce8f 100644
 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
 +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 @@ -930,7 +930,7 @@ static void brw_emit_index_buffer(struct brw_context
 *brw)
 if (index_buffer == NULL)
return;

 -   if (brw-prim_restart.enable_cut_index) {
 +   if (brw-prim_restart.enable_cut_index  !intel-is_haswell) {


I'm worried that we may have a pre-existing bug here.
brw-prim_restart.enable_cut_index depends not only on primitive restart
settings (tracked under _NEW_TRANSFORM) but also on whether we are doing
primitive restart in hardware or software; pre-Haswell that depends on the
type of primitive (BRW_NEW_PRIMITIVE) and whether we are counting
primitives (_NEW_DEPTH I think, ugh).  But brw_emit_index_buffer is only
emitted when BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER is flagged.

I suspect we could come up with a state change that would cause
brw-prim_restart.enable_cut_index to flip from false to true without
setting BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER; if that happens, then
brw_emit_index_buffer() won't get called, so hardware primitive restart
won't get switched on when it needs to be.

Possible solutions:

1. Update brw_index_buffer so that it will also be triggered by
_NEW_TRANSFORM, and change the condition to if
(ctx-Array.PrimitiveRestart  !intel-is_haswell).  As a side effect,
this will enable hardware primitive restart whenever software primitive
restart is in use, but I think that should be harmless (since software
primitive restart should prevent the cut index from getting sent into the
pipeline).

2. Update brw_index_buffer so that it will also be triggered by
_NEW_TRANSFORM, BRW_NEW_PRIMITIVE, and _NEW_DEPTH.

3. Define a new BRW bit that will be flagged when
brw-prim_restart.enable_cut_index is changed, and update brw_index_buffer
so that it will be triggered by that bit.

I'm leaning toward #1.  What do you think?

   cut_index_setting = BRW_CUT_INDEX_ENABLE;
 } else {
cut_index_setting = 0;
 diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 index 02deba4..38b5243 100644
 --- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 +++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 @@ -29,8 +29,11 @@
  #include main/bufferobj.h

  #include brw_context.h
 +#include brw_defines.h
  #include brw_draw.h

 +#include intel_batchbuffer.h
 +
  /**
   * Check if the hardware's cut index support can handle the primitive
   * restart index value.
 @@ -39,6 +42,12 @@ static bool
  can_cut_index_handle_restart_index(struct gl_context *ctx,
 const struct _mesa_index_buffer *ib)
  {
 +   struct

Re: [Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Jordan Justen 
On Fri, Aug 31, 2012 at 12:08 PM, Paul Berry stereotype...@gmail.com wrote:
 On 31 August 2012 00:22, Kenneth Graunke kenn...@whitecape.org wrote:

 Haswell moved the Cut Index Enable bit from the INDEX_BUFFER packet to
 a new 3DSTATE_VF packet, so we need to emit that.  Also, it requires us
 to specify the cut index rather than assuming it's 0x.

 This adds a new Haswell-specific tracked state atom to gen7_atoms.
 Normally, we would create a new generation-specific atom list, but since
 there's only one difference over Ivybridge so far, I chose to simply
 make it return without doing any work on non-Haswell systems.

 Fixes five piglit tests:
 - general/primitive-restart-DISABLE_VBO
 - general/primitive-restart-VBO_COMBINED_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_INDEX_ONLY
 - general/primitive-restart-VBO_SEPARATE_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_VERTEX_ONLY

 Signed-off-by: Kenneth Graunke kenn...@whitecape.org
 ---
  src/mesa/drivers/dri/i965/brw_defines.h   |  3 ++
  src/mesa/drivers/dri/i965/brw_draw_upload.c   |  2 +-
  src/mesa/drivers/dri/i965/brw_primitive_restart.c | 36
 +++
  src/mesa/drivers/dri/i965/brw_state.h |  1 +
  src/mesa/drivers/dri/i965/brw_state_upload.c  |  2 ++
  5 files changed, 43 insertions(+), 1 deletion(-)

 I could have sworn I sent this out, but I can't find it in my inbox, so I
 guess I must not have been connected to the internet at the time...oops.

 diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
 b/src/mesa/drivers/dri/i965/brw_defines.h
 index 3605c18..6dc4707 100644
 --- a/src/mesa/drivers/dri/i965/brw_defines.h
 +++ b/src/mesa/drivers/dri/i965/brw_defines.h
 @@ -1037,6 +1037,9 @@ enum brw_message_target {
  # define GEN6_URB_GS_ENTRIES_SHIFT 8
  # define GEN6_URB_GS_SIZE_SHIFT0

 +#define _3DSTATE_VF 0x780c /* GEN7.5+ */
 +#define HSW_CUT_INDEX_ENABLE(1  8)
 +
  #define _3DSTATE_URB_VS 0x7830 /* GEN7+ */
  #define _3DSTATE_URB_HS 0x7831 /* GEN7+ */
  #define _3DSTATE_URB_DS 0x7832 /* GEN7+ */
 diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c
 b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 index e40c7d5..33cce8f 100644
 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
 +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 @@ -930,7 +930,7 @@ static void brw_emit_index_buffer(struct brw_context
 *brw)
 if (index_buffer == NULL)
return;

 -   if (brw-prim_restart.enable_cut_index) {
 +   if (brw-prim_restart.enable_cut_index  !intel-is_haswell) {


 I'm worried that we may have a pre-existing bug here.
 brw-prim_restart.enable_cut_index depends not only on primitive restart
 settings (tracked under _NEW_TRANSFORM) but also on whether we are doing
 primitive restart in hardware or software; pre-Haswell that depends on the
 type of primitive (BRW_NEW_PRIMITIVE) and whether we are counting primitives
 (_NEW_DEPTH I think, ugh).  But brw_emit_index_buffer is only emitted when
 BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER is flagged.

 I suspect we could come up with a state change that would cause
 brw-prim_restart.enable_cut_index to flip from false to true without
 setting BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER; if that happens, then
 brw_emit_index_buffer() won't get called, so hardware primitive restart
 won't get switched on when it needs to be.

 Possible solutions:

 1. Update brw_index_buffer so that it will also be triggered by
 _NEW_TRANSFORM, and change the condition to if (ctx-Array.PrimitiveRestart
  !intel-is_haswell).  As a side effect, this will enable hardware
 primitive restart whenever software primitive restart is in use, but I think
 that should be harmless (since software primitive restart should prevent the
 cut index from getting sent into the pipeline).

 2. Update brw_index_buffer so that it will also be triggered by
 _NEW_TRANSFORM, BRW_NEW_PRIMITIVE, and _NEW_DEPTH.

 3. Define a new BRW bit that will be flagged when
 brw-prim_restart.enable_cut_index is changed, and update brw_index_buffer
 so that it will be triggered by that bit.

 I'm leaning toward #1.  What do you think?

Since this is a pre-existing bug, we should probably address it
separately, right?

cut_index_setting = BRW_CUT_INDEX_ENABLE;
 } else {
cut_index_setting = 0;
 diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 index 02deba4..38b5243 100644
 --- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 +++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 @@ -29,8 +29,11 @@
  #include main/bufferobj.h

  #include brw_context.h
 +#include brw_defines.h
  #include brw_draw.h

 +#include intel_batchbuffer.h
 +
  /**
   * Check if the hardware's cut index support can handle the primitive
   * restart index value.
 @@

Re: [Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Jordan Justen 
Reviewed-by: Jordan Justen jordan.l.jus...@intel.com

2 minor questions below...

On Fri, Aug 31, 2012 at 12:22 AM, Kenneth Graunke kenn...@whitecape.org wrote:
 Haswell moved the Cut Index Enable bit from the INDEX_BUFFER packet to
 a new 3DSTATE_VF packet, so we need to emit that.  Also, it requires us
 to specify the cut index rather than assuming it's 0x.

 This adds a new Haswell-specific tracked state atom to gen7_atoms.
 Normally, we would create a new generation-specific atom list, but since
 there's only one difference over Ivybridge so far, I chose to simply
 make it return without doing any work on non-Haswell systems.

 Fixes five piglit tests:
 - general/primitive-restart-DISABLE_VBO
 - general/primitive-restart-VBO_COMBINED_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_INDEX_ONLY
 - general/primitive-restart-VBO_SEPARATE_VERTEX_AND_INDEX
 - general/primitive-restart-VBO_VERTEX_ONLY

 Signed-off-by: Kenneth Graunke kenn...@whitecape.org
 ---
  src/mesa/drivers/dri/i965/brw_defines.h   |  3 ++
  src/mesa/drivers/dri/i965/brw_draw_upload.c   |  2 +-
  src/mesa/drivers/dri/i965/brw_primitive_restart.c | 36 
 +++
  src/mesa/drivers/dri/i965/brw_state.h |  1 +
  src/mesa/drivers/dri/i965/brw_state_upload.c  |  2 ++
  5 files changed, 43 insertions(+), 1 deletion(-)

 I could have sworn I sent this out, but I can't find it in my inbox, so I
 guess I must not have been connected to the internet at the time...oops.

 diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
 b/src/mesa/drivers/dri/i965/brw_defines.h
 index 3605c18..6dc4707 100644
 --- a/src/mesa/drivers/dri/i965/brw_defines.h
 +++ b/src/mesa/drivers/dri/i965/brw_defines.h
 @@ -1037,6 +1037,9 @@ enum brw_message_target {
  # define GEN6_URB_GS_ENTRIES_SHIFT 8
  # define GEN6_URB_GS_SIZE_SHIFT0

 +#define _3DSTATE_VF 0x780c /* GEN7.5+ */
 +#define HSW_CUT_INDEX_ENABLE(1  8)
 +
  #define _3DSTATE_URB_VS 0x7830 /* GEN7+ */
  #define _3DSTATE_URB_HS 0x7831 /* GEN7+ */
  #define _3DSTATE_URB_DS 0x7832 /* GEN7+ */
 diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c 
 b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 index e40c7d5..33cce8f 100644
 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
 +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
 @@ -930,7 +930,7 @@ static void brw_emit_index_buffer(struct brw_context *brw)
 if (index_buffer == NULL)
return;

 -   if (brw-prim_restart.enable_cut_index) {
 +   if (brw-prim_restart.enable_cut_index  !intel-is_haswell) {
cut_index_setting = BRW_CUT_INDEX_ENABLE;
 } else {
cut_index_setting = 0;
 diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c 
 b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 index 02deba4..38b5243 100644
 --- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 +++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c
 @@ -29,8 +29,11 @@
  #include main/bufferobj.h

  #include brw_context.h
 +#include brw_defines.h
  #include brw_draw.h

 +#include intel_batchbuffer.h
 +
  /**
   * Check if the hardware's cut index support can handle the primitive
   * restart index value.
 @@ -39,6 +42,12 @@ static bool
  can_cut_index_handle_restart_index(struct gl_context *ctx,
 const struct _mesa_index_buffer *ib)
  {
 +   struct intel_context *intel = intel_context(ctx);
 +
 +   /* Haswell supports an arbitrary cut index. */
 +   if (intel-is_haswell)
 +  return true;
 +
 bool cut_index_will_work;

 switch (ib-type) {
 @@ -176,3 +185,30 @@ brw_handle_primitive_restart(struct gl_context *ctx,
 return GL_TRUE;
  }

 +static void
 +haswell_upload_cut_index(struct brw_context *brw)

I see hsw above. Should we use hsw similar to brw?

 +{
 +   struct intel_context *intel = brw-intel;
 +   struct gl_context *ctx = intel-ctx;
 +
 +   /* Don't trigger on Ivybridge */

Should this say 'Don't trigger for previous generations'?

 +   if (!intel-is_haswell)
 +  return;
 +
 +   const unsigned cut_index_setting =
 +  ctx-Array.PrimitiveRestart ? HSW_CUT_INDEX_ENABLE : 0;
 +
 +   BEGIN_BATCH(2);
 +   OUT_BATCH(_3DSTATE_VF  16 | cut_index_setting | (2 - 2));
 +   OUT_BATCH(ctx-Array.RestartIndex);
 +   ADVANCE_BATCH();
 +}
 +
 +const struct brw_tracked_state haswell_cut_index = {
 +   .dirty = {
 +  .mesa  = _NEW_TRANSFORM,
 +  .brw   = 0,
 +  .cache = 0,
 +   },
 +   .emit = haswell_upload_cut_index,
 +};
 diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
 b/src/mesa/drivers/dri/i965/brw_state.h
 index a80ee86..99fa088 100644
 --- a/src/mesa/drivers/dri/i965/brw_state.h
 +++ b/src/mesa/drivers/dri/i965/brw_state.h
 @@ -133,6 +133,7 @@ extern const struct brw_tracked_state gen7_wm_constants;
  extern const struct brw_tracked_state

Re: [Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Paul Berry 
On 31 August 2012 12:52, Jordan Justen jljus...@gmail.com wrote:

 On Fri, Aug 31, 2012 at 12:08 PM, Paul Berry stereotype...@gmail.com
 wrote:
  On 31 August 2012 00:22, Kenneth Graunke kenn...@whitecape.org wrote:
 
  Haswell moved the Cut Index Enable bit from the INDEX_BUFFER packet to
  a new 3DSTATE_VF packet, so we need to emit that.  Also, it requires us
  to specify the cut index rather than assuming it's 0x.
 
  This adds a new Haswell-specific tracked state atom to gen7_atoms.
  Normally, we would create a new generation-specific atom list, but since
  there's only one difference over Ivybridge so far, I chose to simply
  make it return without doing any work on non-Haswell systems.
 
  Fixes five piglit tests:
  - general/primitive-restart-DISABLE_VBO
  - general/primitive-restart-VBO_COMBINED_VERTEX_AND_INDEX
  - general/primitive-restart-VBO_INDEX_ONLY
  - general/primitive-restart-VBO_SEPARATE_VERTEX_AND_INDEX
  - general/primitive-restart-VBO_VERTEX_ONLY
 
  Signed-off-by: Kenneth Graunke kenn...@whitecape.org
  ---
   src/mesa/drivers/dri/i965/brw_defines.h   |  3 ++
   src/mesa/drivers/dri/i965/brw_draw_upload.c   |  2 +-
   src/mesa/drivers/dri/i965/brw_primitive_restart.c | 36
  +++
   src/mesa/drivers/dri/i965/brw_state.h |  1 +
   src/mesa/drivers/dri/i965/brw_state_upload.c  |  2 ++
   5 files changed, 43 insertions(+), 1 deletion(-)
 
  I could have sworn I sent this out, but I can't find it in my inbox, so
 I
  guess I must not have been connected to the internet at the time...oops.
 
  diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
  b/src/mesa/drivers/dri/i965/brw_defines.h
  index 3605c18..6dc4707 100644
  --- a/src/mesa/drivers/dri/i965/brw_defines.h
  +++ b/src/mesa/drivers/dri/i965/brw_defines.h
  @@ -1037,6 +1037,9 @@ enum brw_message_target {
   # define GEN6_URB_GS_ENTRIES_SHIFT 8
   # define GEN6_URB_GS_SIZE_SHIFT0
 
  +#define _3DSTATE_VF 0x780c /* GEN7.5+ */
  +#define HSW_CUT_INDEX_ENABLE(1  8)
  +
   #define _3DSTATE_URB_VS 0x7830 /* GEN7+ */
   #define _3DSTATE_URB_HS 0x7831 /* GEN7+ */
   #define _3DSTATE_URB_DS 0x7832 /* GEN7+ */
  diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c
  b/src/mesa/drivers/dri/i965/brw_draw_upload.c
  index e40c7d5..33cce8f 100644
  --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
  +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
  @@ -930,7 +930,7 @@ static void brw_emit_index_buffer(struct brw_context
  *brw)
  if (index_buffer == NULL)
 return;
 
  -   if (brw-prim_restart.enable_cut_index) {
  +   if (brw-prim_restart.enable_cut_index  !intel-is_haswell) {
 
 
  I'm worried that we may have a pre-existing bug here.
  brw-prim_restart.enable_cut_index depends not only on primitive restart
  settings (tracked under _NEW_TRANSFORM) but also on whether we are doing
  primitive restart in hardware or software; pre-Haswell that depends on
 the
  type of primitive (BRW_NEW_PRIMITIVE) and whether we are counting
 primitives
  (_NEW_DEPTH I think, ugh).  But brw_emit_index_buffer is only emitted
 when
  BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER is flagged.
 
  I suspect we could come up with a state change that would cause
  brw-prim_restart.enable_cut_index to flip from false to true without
  setting BRW_NEW_BATCH or BRW_NEW_INDEX_BUFFER; if that happens, then
  brw_emit_index_buffer() won't get called, so hardware primitive restart
  won't get switched on when it needs to be.
 
  Possible solutions:
 
  1. Update brw_index_buffer so that it will also be triggered by
  _NEW_TRANSFORM, and change the condition to if
 (ctx-Array.PrimitiveRestart
   !intel-is_haswell).  As a side effect, this will enable hardware
  primitive restart whenever software primitive restart is in use, but I
 think
  that should be harmless (since software primitive restart should prevent
 the
  cut index from getting sent into the pipeline).
 
  2. Update brw_index_buffer so that it will also be triggered by
  _NEW_TRANSFORM, BRW_NEW_PRIMITIVE, and _NEW_DEPTH.
 
  3. Define a new BRW bit that will be flagged when
  brw-prim_restart.enable_cut_index is changed, and update
 brw_index_buffer
  so that it will be triggered by that bit.
 
  I'm leaning toward #1.  What do you think?

 Since this is a pre-existing bug, we should probably address it
 separately, right?


Yeah, good point.  I'd be ok with that.  Keeping in mind, of course, that
after this patch lands, the bug will be in two places rather than one :)

It also occurs to me that another state change we should worry about is if
the client program switches between glDrawArrays() and glDrawElements().  I
don't know if BRW_NEW_INDEX_BUFFER gets flagged when that happens.  If not,
that might cause a problem for my proposal #2 above.

Re: [Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

2012-08-31 Thread Paul Berry 
On 31 August 2012 13:16, Paul Berry stereotype...@gmail.com wrote:


 Yeah, good point.  I'd be ok with that.  Keeping in mind, of course, that
 after this patch lands, the bug will be in two places rather than one :)


Scratch that.  Ken's patch implements my suggestion #1 for Haswell, so the
bug only needs to be addressed for previous chip generations.  Sorry for
the confusion.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev