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@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.
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).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
