[Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.
Paul Berry
stereotype441 at gmail.com
Fri Aug 31 12:08:53 PDT 2012
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?
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.
>
>
> --
> 1.7.11.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120831/2ce95c06/attachment.html>
More information about the mesa-dev
mailing list