On 12/12/2018 14:36, Tvrtko Ursulin wrote:
On 12/12/2018 13:41, Chris Wilson wrote:
From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
SFC (Scaler & Format Converter) units are shared between VD and VEBoxes.
They also happen to have separate reset bits. So, whenever we want to
reset
one or more of the media engines, we have to make sure the SFCs do not
change owner in the process and, if this owner happens to be one of the
engines being reset, we need to reset the SFC as well.
This happens in 4 steps:
1) Tell the engine that a software reset is going to happen. The engine
will then try to force lock the SFC (if currently locked, it will
remain so; if currently unlocked, it will ignore this and all new lock
requests).
2) Poll the ack bit to make sure the hardware has received the forced
lock from the driver. Once this bit is set, it indicates SFC status
(lock or unlock) will not change anymore (until we tell the engine it
is safe to unlock again).
3) Check the usage bit to see if the SFC has ended up being locked to
the engine we want to reset. If this is the case, we have to reset
the SFC as well.
4) Unlock all the SFCs once the reset sequence is completed.
Obviously, if we are resetting the whole GPU, we don't have to worry
about all of this.
BSpec: 10989
BSpec: 10990
BSpec: 10954
BSpec: 10955
BSpec: 10956
BSpec: 19212
Signed-off-by: Tomasz Lis <tomasz.lis@xxxxxxxxx>
Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_reg.h | 18 +++++
drivers/gpu/drm/i915/intel_uncore.c | 115 ++++++++++++++++++++++++++--
2 files changed, 128 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 0a7d60509ca7..0796526dc10f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -347,6 +347,24 @@ static inline bool i915_mmio_reg_valid(i915_reg_t
reg)
#define GEN11_GRDOM_MEDIA4 (1 << 8)
#define GEN11_GRDOM_VECS (1 << 13)
#define GEN11_GRDOM_VECS2 (1 << 14)
+#define GEN11_GRDOM_SFC0 (1 << 17)
+#define GEN11_GRDOM_SFC1 (1 << 18)
+
+#define GEN11_VCS_SFC_RESET_BIT(instance) (GEN11_GRDOM_SFC0 <<
((instance) >> 1))
+#define GEN11_VECS_SFC_RESET_BIT(instance) (GEN11_GRDOM_SFC0 <<
(instance))
These two lines could be a bit of a hidden mine for the future. They
silently describe which video engines are connected to which SFC
instance. I guess we'll just have to remember to update this if/when it
changes.
+
+#define GEN11_VCS_SFC_FORCED_LOCK(engine)
_MMIO((engine)->mmio_base + 0x88C)
+#define GEN11_VCS_SFC_FORCED_LOCK_BIT (1 << 0)
+#define GEN11_VCS_SFC_LOCK_STATUS(engine)
_MMIO((engine)->mmio_base + 0x890)
+#define GEN11_VCS_SFC_USAGE_BIT (1 << 0)
+#define GEN11_VCS_SFC_LOCK_ACK_BIT (1 << 1)
+
+#define GEN11_VECS_SFC_FORCED_LOCK(engine)
_MMIO((engine)->mmio_base + 0x201C)
+#define GEN11_VECS_SFC_FORCED_LOCK_BIT (1 << 0)
+#define GEN11_VECS_SFC_LOCK_ACK(engine)
_MMIO((engine)->mmio_base + 0x2018)
+#define GEN11_VECS_SFC_LOCK_ACK_BIT (1 << 0)
+#define GEN11_VECS_SFC_USAGE(engine) _MMIO((engine)->mmio_base
+ 0x2014)
+#define GEN11_VECS_SFC_USAGE_BIT (1 << 0)
#define RING_PP_DIR_BASE(engine) _MMIO((engine)->mmio_base + 0x228)
#define RING_PP_DIR_BASE_READ(engine) _MMIO((engine)->mmio_base +
0x518)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 9289515108c3..408692b88c98 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1931,6 +1931,103 @@ static int gen6_reset_engines(struct
drm_i915_private *dev_priv,
return gen6_hw_domain_reset(dev_priv, hw_mask);
}
+static u32 gen11_lock_sfc(struct drm_i915_private *dev_priv,
+ struct intel_engine_cs *engine)
+{
+ u8 vdbox_sfc_access = INTEL_INFO(dev_priv)->vdbox_sfc_access;
Only a single use site so could get away with a local copy.
+ i915_reg_t sfc_forced_lock, sfc_forced_lock_ack;
+ u32 sfc_forced_lock_bit, sfc_forced_lock_ack_bit;
+ i915_reg_t sfc_usage;
+ u32 sfc_usage_bit;
+ u32 sfc_reset_bit;
+
+ switch (engine->class) {
+ case VIDEO_DECODE_CLASS:
+ if ((BIT(engine->instance) & vdbox_sfc_access) == 0)
+ return 0;
+
+ sfc_forced_lock = GEN11_VCS_SFC_FORCED_LOCK(engine);
+ sfc_forced_lock_bit = GEN11_VCS_SFC_FORCED_LOCK_BIT;
+
+ sfc_forced_lock_ack = GEN11_VCS_SFC_LOCK_STATUS(engine);
+ sfc_forced_lock_ack_bit = GEN11_VCS_SFC_LOCK_ACK_BIT;
+
+ sfc_usage = GEN11_VCS_SFC_LOCK_STATUS(engine);
+ sfc_usage_bit = GEN11_VCS_SFC_USAGE_BIT;
+ sfc_reset_bit = GEN11_VCS_SFC_RESET_BIT(engine->instance);
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ sfc_forced_lock = GEN11_VECS_SFC_FORCED_LOCK(engine);
+ sfc_forced_lock_bit = GEN11_VECS_SFC_FORCED_LOCK_BIT;
+
+ sfc_forced_lock_ack = GEN11_VECS_SFC_LOCK_ACK(engine);
+ sfc_forced_lock_ack_bit = GEN11_VECS_SFC_LOCK_ACK_BIT;
+
+ sfc_usage = GEN11_VECS_SFC_USAGE(engine);
+ sfc_usage_bit = GEN11_VECS_SFC_USAGE_BIT;
+ sfc_reset_bit = GEN11_VECS_SFC_RESET_BIT(engine->instance);
+ break;
+
+ default:
+ return 0;
+ }
+
+ /*
+ * Tell the engine that a software reset is going to happen. The
engine
+ * will then try to force lock the SFC (if currently locked, it will
+ * remain so until we tell the engine it is safe to unlock; if
currently
+ * unlocked, it will ignore this and all new lock requests). If SFC
+ * ends up being locked to the engine we want to reset, we have
to reset
+ * it as well (we will unlock it once the reset sequence is
completed).
+ */
+ I915_WRITE_FW(sfc_forced_lock,
+ I915_READ_FW(sfc_forced_lock) | sfc_forced_lock_bit);
+
+ if (__intel_wait_for_register_fw(dev_priv,
+ sfc_forced_lock_ack,
+ sfc_forced_lock_ack_bit,
+ sfc_forced_lock_ack_bit,
+ 1000, 0, NULL)) {
+ DRM_DEBUG_DRIVER("Wait for SFC forced lock ack failed\n");
+ return 0;
+ }
+
+ if ((I915_READ_FW(sfc_usage) & sfc_usage_bit) == sfc_usage_bit)
"== sfc_usage_bit" parts is redundant.
+ return sfc_reset_bit;
+
+ return 0;
+}
+
+static void gen11_unlock_sfc(struct drm_i915_private *dev_priv,
+ struct intel_engine_cs *engine)
+{
+ u8 vdbox_sfc_access = INTEL_INFO(dev_priv)->vdbox_sfc_access;
+ i915_reg_t sfc_forced_lock;
+ u32 sfc_forced_lock_bit;
+
+ switch (engine->class) {
+ case VIDEO_DECODE_CLASS:
+ if ((BIT(engine->instance) & vdbox_sfc_access) == 0)
+ return;
+
+ sfc_forced_lock = GEN11_VCS_SFC_FORCED_LOCK(engine);
+ sfc_forced_lock_bit = GEN11_VCS_SFC_FORCED_LOCK_BIT;
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ sfc_forced_lock = GEN11_VECS_SFC_FORCED_LOCK(engine);
+ sfc_forced_lock_bit = GEN11_VECS_SFC_FORCED_LOCK_BIT;
+ break;
+
+ default:
+ return;
+ }
+
+ I915_WRITE_FW(sfc_forced_lock,
+ I915_READ_FW(sfc_forced_lock) & ~sfc_forced_lock_bit);
+}
+
/**
* gen11_reset_engines - reset individual engines
* @dev_priv: i915 device
@@ -1947,7 +2044,6 @@ static int gen6_reset_engines(struct
drm_i915_private *dev_priv,
static int gen11_reset_engines(struct drm_i915_private *dev_priv,
unsigned int engine_mask)
{
- struct intel_engine_cs *engine;
const u32 hw_engine_mask[I915_NUM_ENGINES] = {
[RCS] = GEN11_GRDOM_RENDER,
[BCS] = GEN11_GRDOM_BLT,
@@ -1958,21 +2054,30 @@ static int gen11_reset_engines(struct
drm_i915_private *dev_priv,
[VECS] = GEN11_GRDOM_VECS,
[VECS2] = GEN11_GRDOM_VECS2,
};
+ struct intel_engine_cs *engine;
+ unsigned int tmp;
u32 hw_mask;
+ int ret;
BUILD_BUG_ON(VECS2 + 1 != I915_NUM_ENGINES);
if (engine_mask == ALL_ENGINES) {
hw_mask = GEN11_GRDOM_FULL;
} else {
- unsigned int tmp;
-
hw_mask = 0;
- for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
+ for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
hw_mask |= hw_engine_mask[engine->id];
+ hw_mask |= gen11_lock_sfc(dev_priv, engine);
Presumably point No. 3 from the commit message does not mean there is a
separate way to reset the SFC, just that the act of resetting the engine
to which the SFC is locked to, will reset SFC as well?
My bad, gen11_lock_sfc is returning the bit which resets the SFC.
+ }
}
- return gen6_hw_domain_reset(dev_priv, hw_mask);
+ ret = gen6_hw_domain_reset(dev_priv, hw_mask);
+
+ if (engine_mask != ALL_ENGINES)
+ for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
+ gen11_unlock_sfc(dev_priv, engine);
And this presumably means it is okay to unlock engines which we haven't
lock in the previous loop?
For this I found no clear information. Presumably it is okay to clear
what hasn't been set in the first place...
+
+ return ret;
}
/**
I'll go to look for answers to the commit message question before
applying the r-b.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
