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

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

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.
>
> [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.

regards
Arun

>
> .Dave.
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx