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

[Daniele Ceraolo Spurio]

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

LGTM.

Daniele