[Intel-gfx] [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.
Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
Cc: Radhakrishna Sripada
Cc: Jonathan Cavitt
Signed-off-by: Matt Roper
---
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
Re: [Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
> -Original Message-
> From: Roper, Matthew D
> Sent: Thursday, October 19, 2023 10:33 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Roper, Matthew D ; Sripada, Radhakrishna
> ; Cavitt, Jonathan
> 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
> Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
> Cc: Radhakrishna Sripada
> Cc: Jonathan Cavitt
> Signed-off-by: Matt Roper
> ---
> 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
Re: [Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
-Original Message-
From: Sripada, Radhakrishna
Sent: Friday, October 20, 2023 2:32 PM
To: Roper, Matthew D ;
intel-gfx@lists.freedesktop.org
Cc: Cavitt, Jonathan
Subject: RE: [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering
operations
> > -Original Message-
> > From: Roper, Matthew D
> > Sent: Thursday, October 19, 2023 10:33 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Roper, Matthew D ; Sripada, Radhakrishna
> > ; Cavitt, Jonathan
> >
> > 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
Concurred. While a WA number would be preferrable here, we can add once the WA
is finalized:
Reviewed-by: Jonathan Cavitt
-Jonathan Cavitt
>
>
> > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
> > Cc: Radhakrishna Sripada
> > Cc: Jonathan Cavitt
> > Signed-off-by: Matt Roper
> > ---
> > 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
>
>
Re: [Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
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: # 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
> Cc: Jonathan Cavitt
> Signed-off-by: Matt Roper
In any case,
Reviewed-by: Andi Shyti
Andi
Re: [Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
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: # 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
> > Cc: Jonathan Cavitt
> > Signed-off-by: Matt Roper
>
> In any case,
>
> Reviewed-by: Andi Shyti
>
> Andi
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
Re: [Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations
Hi Matt, ... > > Cc: # 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 This stands. Thanks, Matt! Andi
