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

[Andi Shyti]

On Thu, Oct 05, 2023 at 04:46:49PM +0000, Yang, Fei wrote:
> > 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.

I thought no one reads the trybot :-)

Indeed it doesn't make sense but I wanted to test whether this
could have been a possible solution and then figure out. But
apparently it's not as Jonathan tested it locally, as well.

Andi