On 05/06/15 14:57, Arun Siluvery wrote: > In Per context w/a batch buffer, > WaRsRestoreWithPerCtxtBb > > v2: This patches modifies definitions of MI_LOAD_REGISTER_MEM and > MI_LOAD_REGISTER_REG; Add GEN8 specific defines for these instructions > so as to not break any future users of existing definitions (Michel) > > Signed-off-by: Rafael Barbalho <rafael.barbalho@xxxxxxxxx> > Signed-off-by: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_reg.h | 26 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_lrc.c | 59 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 85 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 33b0ff1..6928162 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h [snip] > #define MI_LOAD_REGISTER_MEM MI_INSTR(0x29, 0) > #define MI_LOAD_REGISTER_REG MI_INSTR(0x2A, 0) > +#define MI_LOAD_REGISTER_MEM_GEN8 MI_INSTR(0x29, 2) > +#define MI_LRM_USE_GLOBAL_GTT (1<<22) > +#define MI_LRM_ASYNC_MODE_ENABLE (1<<21) > +#define MI_LOAD_REGISTER_REG_GEN8 MI_INSTR(0x2A, 1) Isn't the original definition of MI_LOAD_REGISTER_REG wrong anyway? It's a two-operand instruction, each of which is a one-word MMIO register address, hence always 3 words total. The length bias is 2, so the so-called 'flags' field must be 1. The original definition (where the second argument of the MI_INSTR macro is 0) shouldn't work. So just correct the original definition of MI_LOAD_REGISTER_REG; this isn't something that's actually changed on GEN8. While we're mentioning it, I think the above MI_LOAD_REGISTER_MEM is wrong too. The length should be 1 pre-GEN8, and 2 in GEN8+. > #define MI_RS_STORE_DATA_IMM MI_INSTR(0x2B, 0) > #define MI_LOAD_URB_MEM MI_INSTR(0x2C, 0) > #define MI_STORE_URB_MEM MI_INSTR(0x2D, 0) And these are wrong too! In fact all of these instructions have been added under a comment which says "Commands used only by the command parser". Looks like they were added as placeholders without the proper length fields, and then people have started using them as though they were complete definitions :( Time update them all, perhaps ... [snip] > + /* > + * BSpec says MI_LOAD_REGISTER_MEM, MI_LOAD_REGISTER_REG and > + * MI_BATCH_BUFFER_END instructions in this sequence need to be > + * in the same cacheline. > + */ > + while (((unsigned long) (cmd + index) % CACHELINE_BYTES) != 0) > + cmd[index++] = MI_NOOP; > + > + cmd[index++] = MI_LOAD_REGISTER_MEM_GEN8 | > + MI_LRM_USE_GLOBAL_GTT | > + MI_LRM_ASYNC_MODE_ENABLE; > + cmd[index++] = INSTPM; > + cmd[index++] = scratch_addr; > + cmd[index++] = 0; > + > + /* > + * BSpec says there should not be any commands programmed > + * between MI_LOAD_REGISTER_REG and MI_BATCH_BUFFER_END so > + * do not add any new commands > + */ > + cmd[index++] = MI_LOAD_REGISTER_REG_GEN8; > + cmd[index++] = GEN8_RS_PREEMPT_STATUS; > + cmd[index++] = GEN8_RS_PREEMPT_STATUS; > + > /* padding */ > while (index < end) > cmd[index++] = MI_NOOP; > Where's the MI_BATCH_BUFFER_END referred to in the comment? .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
