[Intel-gfx] [PATCH v2 4/5] drm/i915/mtl: Add hardware-level lock for steering

[Matt Roper]

On Mon, Dec 05, 2022 at 08:58:16AM +0000, Tvrtko Ursulin wrote:
> 
> On 28/11/2022 23:30, Matt Roper wrote:
> > Starting with MTL, the driver needs to not only protect the steering
> > control register from simultaneous software accesses, but also protect
> > against races with hardware/firmware agents.  The hardware provides a
> > dedicated locking mechanism to support this via the MTL_STEER_SEMAPHORE
> > register.  Reading the register acts as a 'trylock' operation; the read
> > will return 0x1 if the lock is acquired or 0x0 if something else is
> > already holding the lock; once acquired, writing 0x1 to the register
> > will release the lock.
> > 
> > We'll continue to grab the software lock as well, just so lockdep can
> > track our locking; assuming the hardware lock is behaving properly,
> > there should never be any contention on the software lock in this case.
> > 
> > v2:
> >   - Extend hardware semaphore timeout and add a taint for CI if it ever
> >     happens (this would imply misbehaving hardware/firmware).  (Mika)
> >   - Add "MTL_" prefix to new steering semaphore register.  (Mika)
> > 
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt_mcr.c  | 38 ++++++++++++++++++++++---
> >   drivers/gpu/drm/i915/gt/intel_gt_regs.h |  1 +
> >   2 files changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > index aa070ae57f11..087e4ac5b68d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > @@ -347,10 +347,9 @@ static u32 rw_with_mcr_steering(struct intel_gt *gt,
> >    * @flags: storage to save IRQ flags to
> >    *
> >    * Performs locking to protect the steering for the duration of an MCR
> > - * operation.  Depending on the platform, this may be a software lock
> > - * (gt->mcr_lock) or a hardware lock (i.e., a register that synchronizes
> > - * access not only for the driver, but also for external hardware and
> > - * firmware agents).
> > + * operation.  On MTL and beyond, a hardware lock will also be taken to
> > + * serialize access not only for the driver, but also for external hardware and
> > + * firmware agents.
> >    *
> >    * Context: Takes gt->mcr_lock.  uncore->lock should *not* be held when this
> >    *          function is called, although it may be acquired after this
> > @@ -359,12 +358,40 @@ static u32 rw_with_mcr_steering(struct intel_gt *gt,
> >   void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
> >   {
> >   	unsigned long __flags;
> > +	int err = 0;
> >   	lockdep_assert_not_held(&gt->uncore->lock);
> > +	/*
> > +	 * Starting with MTL, we need to coordinate not only with other
> > +	 * driver threads, but also with hardware/firmware agents.  A dedicated
> > +	 * locking register is used.
> > +	 */
> > +	if (GRAPHICS_VER(gt->i915) >= IP_VER(12, 70))
> > +		err = wait_for(intel_uncore_read_fw(gt->uncore,
> > +						    MTL_STEER_SEMAPHORE) == 0x1, 100);
> > +
> 
> If two i915 threads enter here what happens? (Given hw locking is done
> before the spinlock.)

The second thread will see a '0' when it reads the register, indicating
that something else (sw, fw, or hw) already has it locked.  As soon as
the first thread drops the lock, the next read will return '1' and allow
the second thread to proceed.


Matt

> 
> Regards,
> 
> Tvrtko
> 
> > +	/*
> > +	 * Even on platforms with a hardware lock, we'll continue to grab
> > +	 * a software spinlock too for lockdep purposes.  If the hardware lock
> > +	 * was already acquired, there should never be contention on the
> > +	 * software lock.
> > +	 */
> >   	spin_lock_irqsave(&gt->mcr_lock, __flags);
> >   	*flags = __flags;
> > +
> > +	/*
> > +	 * In theory we should never fail to acquire the HW semaphore; this
> > +	 * would indicate some hardware/firmware is misbehaving and not
> > +	 * releasing it properly.
> > +	 */
> > +	if (err == -ETIMEDOUT) {
> > +		drm_err_ratelimited(&gt->i915->drm,
> > +				    "GT%u hardware MCR steering semaphore timed out",
> > +				    gt->info.id);
> > +		add_taint_for_CI(gt->i915, TAINT_WARN);  /* CI is now unreliable */
> > +	}
> >   }
> >   /**
> > @@ -379,6 +406,9 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
> >   void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
> >   {
> >   	spin_unlock_irqrestore(&gt->mcr_lock, flags);
> > +
> > +	if (GRAPHICS_VER(gt->i915) >= IP_VER(12, 70))
> > +		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> >   }
> >   /**
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 784152548472..1618d46cb8c7 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -67,6 +67,7 @@
> >   #define GMD_ID_MEDIA				_MMIO(MTL_MEDIA_GSI_BASE + 0xd8c)
> >   #define MCFG_MCR_SELECTOR			_MMIO(0xfd0)
> > +#define MTL_STEER_SEMAPHORE			_MMIO(0xfd0)
> >   #define MTL_MCR_SELECTOR			_MMIO(0xfd4)
> >   #define SF_MCR_SELECTOR				_MMIO(0xfd8)
> >   #define GEN8_MCR_SELECTOR			_MMIO(0xfdc)

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation