[Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

[Michel Thierry]

On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:
>
>
> On 20/04/17 04:33, Joonas Lahtinen wrote:
>> On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:
>>> From: Arun Siluvery <arun.siluvery at linux.intel.com>
>>>
>>> GuC expects a list of registers from the driver which are saved/restored
>>> during engine reset. The type of value to be saved is controlled by
>>> flags. We provide a minimal set of registers that we want GuC to save
>>> and
>>> restore. This is not an issue in case of engine reset as driver
>>> initializes
>>> most of them following an engine reset, but in case of media reset (aka
>>> watchdog reset) which is completely internal to GuC (including
>>> resubmission
>>> of hung workload), it is necessary to provide this list, otherwise
>>> GuC won't
>>> be able to schedule further workloads after a reset. This is the minimal
>>> set of registers identified for things to work as expected but if we see
>>> any new issues, this register list can be expanded.
>>>
>>> In order to not loose any existing workarounds, we have to let GuC know
>>> the registers and its values. These will be reapplied after the reset.
>>> Note that we can't just read the current value because most of these
>>> registers are masked (so we have a workaround for a workaround for a
>>> workaround).
>>>
>>> v2: REGSET_MASKED is too difficult for GuC, use
>>> REGSET_SAVE_DEFAULT_VALUE
>>> and current value from RING_MODE reg instead; no need to preserve
>>> head/tail either, be extra paranoid and save whitelisted registers
>>> (Daniele).
>>>
>>> v3: Workarounds added only once during _init_workarounds also have to
>>> been restored, or we risk loosing them after internal GuC reset
>>> (Daniele).
>>>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Signed-off-by: Arun Siluvery <arun.siluvery at linux.intel.com>
>>> Signed-off-by: Jeff McGee <jeff.mcgee at intel.com>
>>> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
>>
>> <SNIP>
>>
>>> @@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
>>> intel_engine_cs *engine)
>>>>      int ret;
>>>
>>>      /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
>>> -    I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
>>> _MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
>>> +    I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
>>> +
>>> _MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
>>
>> To make grepping easier, how about?
>>
>>     I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
>>            ...);
>>
>> Regards, Joonas
>>
>
> GUC_REG makes it sound like it is somehow related to GuC, while it
> isn't, we just want GuC to restore its value. What about GUC_REG_RESTORE?
>

Honestly, I dont care about names, pick one and I add it.
Just a reminder, we not only need the reg offset, we want to save the 
value too.

-Michel