<div class="gmail_quote">On 31 August 2012 12:52, Jordan Justen <<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">On Fri, Aug 31, 2012 at 12:08 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > On 31 August 2012 00:22, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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 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) { > > > 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></div>Since this is a pre-existing bug, we should probably address it separately, right? </blockquote><div> </div><div>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 :)</div><div> </div><div>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.</div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> >> 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; > > > Whatever technique we choose to address the problem in > brw_emit_index_buffer() should be applied here too. > >> >> + >> + BEGIN_BATCH(2); >> + OUT_BATCH(_3DSTATE_VF << 16 | cut_index_setting | (2 - 2)); >> + OUT_BATCH(ctx->Array.RestartIndex); > > > 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? > >> >> + 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, >> }; > > > 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></div>Good catch. -Jordan </blockquote></div>