Re: [PATCH v3 6/6] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

[Dave Gordon <david.s.gordon@intel.com>]

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@intel.com>
>>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>>> ---
>>>   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx