[Mesa-dev] [PATCH] i965: Fix primitive restart on Haswell.

Jordan Justen jljusten at gmail.com
Fri Aug 31 12:52:41 PDT 2012


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?

>>        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


More information about the mesa-dev mailing list