-----Original Message-----
From: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
Sent: Friday, October 20, 2023 2:32 PM
To: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
Subject: RE: [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> > Sent: Thursday, October 19, 2023 10:33 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Sripada, Radhakrishna
> > <radhakrishna.sripada@xxxxxxxxx>; Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
> > 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@xxxxxxxxx>
Concurred. While a WA number would be preferrable here, we can add once the WA
is finalized:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
-Jonathan Cavitt
>
>
> > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > Cc: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > 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(>->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
>
>
