[Intel-gfx] [PATCH 07/19] drm/i915/icl: Mind the SFC units when resetting VD or VEBox engines
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Dec 13 08:20:58 UTC 2018
On 12/12/2018 14:36, Tvrtko Ursulin wrote:
>
> On 12/12/2018 13:41, Chris Wilson wrote:
>> From: Oscar Mateo <oscar.mateo at intel.com>
>>
>> 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 at intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
>> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
>> Cc: Michel Thierry <michel.thierry at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> 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 at intel.com>
Regards,
Tvrtko
>
> Regards,
>
> Tvrtko
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list