On 09/06/2015 19:43, Dave Gordon wrote:
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.
I did notice that the original instructions are odd but thought I might
be wrong hence I created new ones to not disturb the original ones.
ok I will just correct original one and reuse it.
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+.
ok.
#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 ...
these are not related to this patch, so it can be taken up as a
different patch.
[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?
MI_BATCH_BUFFER_END is just below while loop, it is in patch [v3 1/6].
Since the diff context is only few lines it didn't showup in the diff.
regards
Arun
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
