Re: [PATCH] drm/i915: Update WaFlushCoherentL3CacheLinesAtContextSwitch

Dave Gordon
2015-07-06T11:52:53Z

On 03/07/15 16:42, Chris Wilson wrote:
> On Fri, Jul 03, 2015 at 02:27:31PM +0100, Arun Siluvery wrote:
>> In this WA we need to set GEN8_L3SQCREG4[21:21] and reset it after PIPE_CONTROL
>> instruction but there is a slight complication as this is applied in WA batch
>> where the values are only initialized once.
>> Dave identified an issue with the current implementation where the register value
>> is read once at the beginning and it is reused; this patch corrects this by saving
>> the register value to memory, update register with the bit of our interest and
>> restore it back with original value.
>>
>> This implementation uses MI_LOAD_REGISTER_MEM which is currently only used
>> by command parser and was using a default length of 0. This is now updated
>> with correct length and moved to appropriate place.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +--
>> drivers/gpu/drm/i915/i915_reg.h | 3 +-
>> drivers/gpu/drm/i915/intel_lrc.c | 72 +++++++++++++++++++++++++---------
>> 3 files changed, 58 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> index 306d9e4..430571b 100644
>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> @@ -131,7 +131,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
>> .mask = MI_GLOBAL_GTT,
>> .expected = 0,
>> }}, ),
>> - CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W | B,
>> + CMD( MI_LOAD_REGISTER_MEM(1), SMI, !F, 0xFF, W | B,
>> .reg = { .offset = 1, .mask = 0x007FFFFC },
>> .bits = {{
>> .offset = 0,
>> @@ -1021,7 +1021,7 @@ static bool check_cmd(const struct intel_engine_cs *ring,
>> * only MI_LOAD_REGISTER_IMM commands.
>> */
>> if (reg_addr == OACONTROL) {
>> - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
>> + if (desc->cmd.value == MI_LOAD_REGISTER_MEM(1)) {
>
> I had a double take here, but it all comes out in the wash. For one
> moment, I thought the cmd matching had changed, but that has the length
> masked out.
>
> Reviewed-by: Chris Wilson <chris@cris-wilson.co.uk>
>
> Who will start to complain about all the extra frequent register writes,
> probably into common power wells....
> -Chris

Hmm ... that is quite confusing, especially as the actual opcode in the
instruction stream will be MI_LOAD_REGISTER_MEM(2) on GEN8+. It might
almost be better to use MI_LOAD_REGISTER_MEM(0) to emphasise that the
length field is a wildcard and not something that will be matched exactly.

.Dave.