[Intel-gfx] [PATCH v3 6/6] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround
Siluvery, Arun
arun.siluvery at linux.intel.com
Mon Jun 15 07:10:46 PDT 2015
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 at intel.com>
>>>> Signed-off-by: Arun Siluvery <arun.siluvery at 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.
>
>
More information about the Intel-gfx
mailing list