[PATCH] drm/i915/gt: Swap the order of mcr lock and spinlock

[Yang, Fei]

> Until now we first have taken the mcr steering semaphore in hardware and then the mcr_lock spinlock in KMD.
>
> But when performing a big amount of MCR lock/unlock, like during TLB invalidation, the time between MCR steering semaphore lock and the spinlock is liable to a locking contention in the steer semaphore register; especially considering that there is a waiting timeout for the MCR lock.
>
> Swap the order of the locks to ensure proper serialization of the MCR steering semaphore lock access.
>
> Suggested-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index 326c2ed1d99b..e33b33f8deb2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -371,6 +371,14 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
>
>       lockdep_assert_not_held(&gt->uncore->lock);
>
> +     /*
> +      * 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);
> +
>       /*
>        * Starting with MTL, we need to coordinate not only with other
>        * driver threads, but also with hardware/firmware agents.  A dedicated
> @@ -380,14 +388,6 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
>               err = wait_for(intel_uncore_read_fw(gt->uncore,
>                                                   MTL_STEER_SEMAPHORE) == 0x1, 100);

Sleeping with a spinlock held? It looks wrong to me unless wait_for is a
busy wait, but even that holding a spinlock for a long time doesn't seem
to be a good practice either.

> -     /*
> -      * 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;
>
>       /*
> @@ -413,10 +413,10 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)  void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
>       __releases(&gt->mcr_lock)
>  {
> -     spin_unlock_irqrestore(&gt->mcr_lock, flags);
> -
>       if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
>               intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> +
> +     spin_unlock_irqrestore(&gt->mcr_lock, flags);
>  }
>
>  /**
> --
> 2.40.1
>