Re: [PATCH 07/19] drm/i915/icl: Mind the SFC units when resetting VD or VEBox engines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2018-12-13 08:20:58)
> 
> 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.

Hmm, that sounds like the type of info we put into the tables for
intel_engine_cs.

Will we ever notice a bug here? What test do we actually need to perform
to hit an error. Presumably more than just a spinner on a media engine.

That strays back into the topic of a lack of real world test samples for
GPU reset handling.
> 
> >> +
> >> +#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...

Yeah, that was the reasoning I came to as well. So long as we do not
broaden engine_mask so that we can concurrently operate on disjoint sets
the RMW should be safe.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux