[Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations

[Cavitt, Jonathan]

-----Original Message-----
From: Sripada, Radhakrishna <radhakrishna.sripada at intel.com> 
Sent: Friday, October 20, 2023 2:32 PM
To: Roper, Matthew D <matthew.d.roper at intel.com>; intel-gfx at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: RE: [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > Sent: Thursday, October 19, 2023 10:33 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Roper, Matthew D <matthew.d.roper at intel.com>; Sripada, Radhakrishna
> > <radhakrishna.sripada at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
> > Subject: [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
> > 
> > The steering control and semaphore registers are inside an "always on"
> > power domain with respect to RC6.  However there are some issues if
> > higher-level platform sleep states are entering/exiting at the same time
> > these registers are accessed.  Grabbing GT forcewake and holding it over
> > the entire lock/steer/unlock cycle ensures that those sleep states have
> > been fully exited before we access these registers.
> > 
> > This is expected to become a formally documented/numbered workaround
> > soon.
> > 
> > Note that this patch alone isn't expected to have an immediately
> > noticeable impact on MCR (mis)behavior; an upcoming pcode firmware
> > update will also be necessary to provide the other half of this
> > workaround.
> > 
> > v2:
> >  - Move the forcewake inside the Xe_LPG-specific IP version check.  This
> >    should only be necessary on platforms that have a steering semaphore.
> > 
> LGTM,
> Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>

Concurred.  While a WA number would be preferrable here, we can add once the WA
is finalized:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

> 
> 
> > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
> > Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > Cc: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 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..34913912d8ae 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > @@ -376,9 +376,26 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long
> > *flags)
> >  	 * driver threads, but also with hardware/firmware agents.  A dedicated
> >  	 * locking register is used.
> >  	 */
> > -	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> > +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70)) {
> > +		/*
> > +		 * The steering control and semaphore registers are inside an
> > +		 * "always on" power domain with respect to RC6.  However
> > there
> > +		 * are some issues if higher-level platform sleep states are
> > +		 * entering/exiting at the same time these registers are
> > +		 * accessed.  Grabbing GT forcewake and holding it over the
> > +		 * entire lock/steer/unlock cycle ensures that those sleep
> > +		 * states have been fully exited before we access these
> > +		 * registers.  This wakeref will be released in the unlock
> > +		 * routine.
> > +		 *
> > +		 * This is expected to become a formally documented/numbered
> > +		 * workaround soon.
> > +		 */
> > +		intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_GT);
> > +
> >  		err = wait_for(intel_uncore_read_fw(gt->uncore,
> >  						    MTL_STEER_SEMAPHORE)
> > == 0x1, 100);
> > +	}
> > 
> >  	/*
> >  	 * Even on platforms with a hardware lock, we'll continue to grab
> > @@ -415,8 +432,11 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned
> > long flags)
> >  {
> >  	spin_unlock_irqrestore(&gt->mcr_lock, flags);
> > 
> > -	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> > +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70)) {
> >  		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE,
> > 0x1);
> > +
> > +		intel_uncore_forcewake_put(gt->uncore, FORCEWAKE_GT);
> > +	}
> >  }
> > 
> >  /**
> > --
> > 2.41.0
> 
>