Re: [PATCH] drm/i915: Update WaFlushCoherentL3CacheLinesAtContextSwitch

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

On 06/07/2015 12:52, Dave Gordon wrote:
> 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
true, but cmd parser is only upto GEN7.

regards
Arun

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

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