[Mesa-dev] [PATCH] [v3] i965: Split out gen8 push constant state upload

[Kenneth Graunke]

On Thursday, July 09, 2015 11:00:40 AM Ben Widawsky wrote:
> While implementing the workaround in the previous patch I noticed things were
> starting to get a bit messy. Since gen8 works differently enough from gen7, I
> thought splitting it out with be good.

IMHO this is still a bit messy.  What about separating the packet
emission from the decision-making about which of the 4 buffers to use?
Something along these lines (warning: doesn't compile):

#define OUT_RELOC_NULL(x) if (x != 0) { OUT_RELOC(x) } else { OUT_BATCH(x); }
#define OUT_RELOC64_NULL(x) if (x != 0) { OUT_RELOC64(x) } else { OUT_BATCH(x); }

static inline void
emit_3dstate_constant(struct brw_context *brw,
                      uint32_t opcode,
                      uint32_t mocs,
                      uint16_t read_length_0,
                      uint16_t read_length_1,
                      uint16_t read_length_2,
                      uint16_t read_length_3,
                      uint64_t ptr_0,
                      uint64_t ptr_1,
                      uint64_t ptr_2,
                      uint64_t ptr_3)
{
   // XXX: or in mocs wherever it goes
   if (brw->gen >= 8) {
      BEGIN_BATCH(11);
      OUT_BATCH(opcode << 16 | (11 - 2));
      OUT_BATCH(read_length_0 | read_length_1 << 16);
      OUT_BATCH(read_length_2 | read_length_3 << 16);
      OUT_BATCH64(ptr_0);
      OUT_RELOC64_NULL(ptr_1);
      OUT_RELOC64_NULL(ptr_2);
      OUT_RELOC64_NULL(ptr_3);
      ADVANCE_BATCH();
   } else if (brw->gen == 7) {
      /* XXX: we could even put asserts here about the buffers being enabled
       * in order, i.e. if you use 2 you have to use 0 and 1 also
       */
      BEGIN_BATCH(7);
      OUT_BATCH(opcode << 16 | (11 - 2));
      OUT_BATCH(read_length_0 | read_length_1 << 16);
      OUT_BATCH(read_length_2 | read_length_3 << 16);
      OUT_BATCH(ptr_0);
      OUT_RELOC_NULL(ptr_1);
      OUT_RELOC_NULL(ptr_2);
      OUT_RELOC_NULL(ptr_3);
      ADVANCE_BATCH();
   } else if (brw->gen == 6) {
      /* XXX: could probably do gen6 here too */
   } else {
      unreachable("unhandled gen in emit_3dstate_constant");
   }
}

void
gen7_upload_constant_state(struct brw_context *brw,
                           const struct brw_stage_state *stage_state,
                           bool active, unsigned opcode)
{
   uint32_t mocs = brw->gen < 8 ? GEN7_MOCS_L3 : 0;

   /* Disable if the shader stage is inactive or there are no push constants. */
   active = active && stage_state->push_const_size != 0;

   if (!active) {
      emit_3dstate_constant(brw, opcode, mocs, 0, 0, 0, 0, 0, 0, 0, 0);
   } else if (brw->gen >= 9) {
      /* Workaround for SKL+ (we use option #2 until we have a need for more
       * constant buffers). This comes from the documentation for 3DSTATE_CONSTANT_*
       *
       * The driver must ensure The following case does not occur without a flush
       * to the 3D engine: 3DSTATE_CONSTANT_* with buffer 3 read length equal to
       * zero committed followed by a 3DSTATE_CONSTANT_* with buffer 0 read length
       * not equal to zero committed. Possible ways to avoid this condition
       * include:
       *     1. always force buffer 3 to have a non zero read length
       *     2. always force buffer 0 to a zero read length
       */
      emit_3dstate_constant(brw, opcode, mocs,
                            0, stage_state->push_const_size, 0, 0,
                            0, stage_state->push_const_offset, 0, 0);
   } else {
      emit_3dstate_constant(brw, opcode, mocs,
                            stage_state->push_const_size, 0, 0, 0,
                            stage_state->push_const_offset, 0, 0, 0);
   }
   
   /* On SKL+ the new constants don't take effect until the next corresponding
    * 3DSTATE_BINDING_TABLE_POINTER_* command is parsed so we need to ensure
    * that is sent
    */
    if (brw->gen >= 9)
       brw->ctx.NewDriverState |= BRW_NEW_SURFACES;
}

By using a static inline, all the code for unused buffers *should* get
compiled away, seeing as it's all 0.  We might have to stuff it in a .h
file, or put all the gen6+ constbuf stuff in a single file, i.e.
gen6_push_constants.c.

Anyway, just an idea...

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150711/ba72d1ef/attachment.sig>