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

[Michel Thierry]

On 21/04/17 13:21, Chris Wilson wrote:
> On Fri, Apr 21, 2017 at 01:10:37PM -0700, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 21/04/17 13:07, Michel Thierry wrote:
>>>
>>>
>>> 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.
>
> I915_WRITE_FW is not the same class as I915_WRITE_CTL/HEAD/TAIL, and I
> can say from experience the I915_*_CTL/HEAD/TAIL were a mistake (special
> casing one particular access to the ring mmio, but we often deviate from
> that pattern).
>
> Looking at the above I see you are falling for the same trap as the ring
> shorthand... So are you sure the convenience will not be lost later? And
> in particular avoid using I915_WRITE_*() naming style as I would rather
> that was earmarked for the different mmio accessors.

Ok, then can follow the pattern of the other workarounds & whitelist reg 
code?

E.g. WA_REG_GUC_RESTORE or WA_MMIO_REG_GUC_RESTORE (to make it clearer 
that these are not registers in the ctx image).