| Message ID | 20231019170241.2102037-2-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/mcr: Hold GT forcewake during steering operations | expand |
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Thursday, October 19, 2023 10:33 PM > To: intel-gfx@lists.freedesktop.org > Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Sripada, Radhakrishna > <radhakrishna.sripada@intel.com>; Cavitt, Jonathan <jonathan.cavitt@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@intel.com> > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock") > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@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(>->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
-----Original Message----- From: Sripada, Radhakrishna <radhakrishna.sripada@intel.com> Sent: Friday, October 20, 2023 2:32 PM To: Roper, Matthew D <matthew.d.roper@intel.com>; intel-gfx@lists.freedesktop.org Cc: Cavitt, Jonathan <jonathan.cavitt@intel.com> Subject: RE: [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations > > -----Original Message----- > > From: Roper, Matthew D <matthew.d.roper@intel.com> > > Sent: Thursday, October 19, 2023 10:33 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Sripada, Radhakrishna > > <radhakrishna.sripada@intel.com>; Cavitt, Jonathan <jonathan.cavitt@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@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@intel.com> -Jonathan Cavitt > > > > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock") > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@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(>->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 > >
Hi Matt, On Thu, Oct 19, 2023 at 10:02:42AM -0700, Matt Roper wrote: > 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. right... I did try this, but so fare we hold the forcewake when calling mcr_lock(). > v2: > - Move the forcewake inside the Xe_LPG-specific IP version check. This > should only be necessary on platforms that have a steering semaphore. > > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock") Is this the right Fixes tag? This is adding the spinlock around MCR, but the power domain needs to be taken independently from the lock... I think the right fix here is Fixes: 3100240bf846 ("drm/i915/mtl: Add hardware-level lock for steering") Cc: <stable@vger.kernel.org> # v6.3+ When the access to the hardware was added. BTW, given that currently we hold the forcewake already, is this really a fix or is this more looking at the future? Is the Fixes tag necessary? > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> In any case, Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi
On Tue, Oct 24, 2023 at 02:02:17PM +0200, Andi Shyti wrote: > Hi Matt, > > On Thu, Oct 19, 2023 at 10:02:42AM -0700, Matt Roper wrote: > > 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. > > right... I did try this, but so fare we hold the forcewake when > calling mcr_lock(). > > > v2: > > - Move the forcewake inside the Xe_LPG-specific IP version check. This > > should only be necessary on platforms that have a steering semaphore. > > > > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock") > > Is this the right Fixes tag? This is adding the spinlock around > MCR, but the power domain needs to be taken independently from > the lock... I think the right fix here is > > Fixes: 3100240bf846 ("drm/i915/mtl: Add hardware-level lock for steering") Yeah, good point, this is a better target. > Cc: <stable@vger.kernel.org> # v6.3+ No stable kernels support MTL (even if they have some of the commits, it's all dead code). We don't want to tag things for stable if they don't meet the stable kernel requirements. > > When the access to the hardware was added. > > BTW, given that currently we hold the forcewake already, is this > really a fix or is this more looking at the future? Is the Fixes > tag necessary? I'm not 100% sure we hold forcewake in all the cases where we work with MCR registers. For example, some of the big ones like wa_list_apply() don't grab forcewake until after we've already acquired the MCR semaphore. Matt > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > In any case, > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Andi
Hi Matt, ... > > Cc: <stable@vger.kernel.org> # v6.3+ > > No stable kernels support MTL (even if they have some of the commits, > it's all dead code). We don't want to tag things for stable if they > don't meet the stable kernel requirements. yes, right... how could I have missed that :-D > > When the access to the hardware was added. > > > > BTW, given that currently we hold the forcewake already, is this > > really a fix or is this more looking at the future? Is the Fixes > > tag necessary? > > I'm not 100% sure we hold forcewake in all the cases where we work with > MCR registers. For example, some of the big ones like wa_list_apply() > don't grab forcewake until after we've already acquired the MCR > semaphore. yeah... OK! > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> This stands. Thanks, Matt! Andi
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); + } } /**
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. Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock") Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)