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! And therefore also, these instructions do *not* all end up in the same cacheline, thus contradicting the first comment above. Padding *after* a BB_END would be redundant. .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
