[Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.
Paul Berry
stereotype441 at gmail.com
Fri Aug 31 13:16:40 PDT 2012
On 31 August 2012 12:52, Jordan Justen <jljusten at gmail.com> wrote:
> On Fri, Aug 31, 2012 at 12:08 PM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 31 August 2012 00:22, Kenneth Graunke <kenneth at 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 <kenneth at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120831/651f8f2f/attachment-0001.html>
More information about the mesa-dev
mailing list