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

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

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@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!
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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx