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 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 <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_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? > > 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. > > >> 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. > > Good catch. > > -Jordan >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev