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@xxxxxxxxxxxxxxx> > >>>>> > >>>>>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@xxxxxxxxx> > >>>>>Signed-off-by: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> > >>>>>Signed-off-by: Jeff McGee <jeff.mcgee@xxxxxxxxx> > >>>>>Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx> > >>>> > >>>><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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
