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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre