On 12/06/2015 18:03, Dave Gordon wrote:
On 12/06/15 12:58, Siluvery, Arun wrote:
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.
As a minimum, you should move these updated #defines out of the section
under the comment "Commands used only by the command parser" and put
them in the appropriate place in the regular list of MI_ commnds,
preferably in numerical order. Then the ones that are genuinely only
used by the command parser could be left for another 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.
The second comment above says "no commands between LOAD_REG_REG and
BB_END", so the point of my comment was that the BB_END is *NOT*
immediately after the LOAD_REG_REG -- there are a bunch of no-ops there!
true, but they are no-ops so they shouldn't really affect anything. I
guess the spec means no valid commands.
And therefore also, these instructions do *not* all end up in the same
cacheline, thus contradicting the first comment above.
I don't understand why. As per the requirement the commands from the
first MI_LOAD_REGISTER_MEM_GEN8 (after while) through BB_END will be
part of same cacheline (in this case second line).
Padding *after* a BB_END would be redundant.
yes, I just wanted to keep MI_BATCH_BUFFER_END at the end instead of
abruptly terminating the batch which is why I am padding with no-ops, I
can change this if that is preferred.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
