[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 10:29, Michel Thierry wrote:
>
>
> 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.
>

I915_WRITE_GUC_RESTORE(reg, value) ?

That would be inline to the others we have, e.g. I915_WRITE_FW, 
I915_WRITE_CTL, I915_WRITE_HEAD/TAIL.

-Michel