On 31 August 2012 00:22, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 0xffffffff. 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> --- 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_SHIFT 0 +#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) { </blockquote><div> 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? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; </blockquote><div> Whatever technique we choose to address the problem in brw_emit_index_buffer() should be applied here too. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + BEGIN_BATCH(2); + OUT_BATCH(_3DSTATE_VF << 16 | cut_index_setting | (2 - 2)); + OUT_BATCH(ctx->Array.RestartIndex); </blockquote><div> The bspec says that this value will be compared to the "fetched (and possibly-sign-extended) vertex index". Do you know if we need to worry about sign extending this value? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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 --- a/src/mesa/drivers/dri/i965/brw_state_upload.c +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c @@ -246,6 +246,8 @@ const struct brw_tracked_state *gen7_atoms[] = &brw_indices, &brw_index_buffer, &brw_vertices, + + &haswell_cut_index, }; </blockquote><div> From my reading of the bspec, Haswell supports hardware primitive restart for more primitive types than Ivy Bridge (e.g. triangle fan), so I think can_cut_index_handle_prims() should also be updated. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -- 1.7.11.4 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>