Re: [Intel-gfx] [PATCH] drm/i915: Remove runtime suspended boolean from intel_runtime_pm struct
On Wed, 2023-09-13 at 18:01 +0300, Imre Deak wrote: > On Wed, Sep 13, 2023 at 03:02:58PM +0300, Jani Nikula wrote: > > On Wed, 13 Sep 2023, Jouni Högander > > wrote: > > > It's not necessary to carry separate suspended status information > > > in > > > intel_runtime_pm struct as this information is already in > > > underlying device > > > structure. Remove it and use pm_runtime_suspended() to obtain > > > suspended > > > status information when needed. > > > > I started wondering if this is racy, and my conclusion is that it's > > "less" racy than the original. rpm->suspended gets toggled in the > > middle > > of the suspend/resume sequences. So it could be halfway. Dunno if > > anything *after* those toggles depends on the state having been > > changed > > already; didn't find any. Maybe Imre has a better idea. > > > > Also, pm_runtime_suspended() seems more reliable when > > suspend/resume > > fails. > > The flag was added in the initial runtime PM enabling patch to detect > MMIO accesses while runtime suspended, looks like > pm_runtime_suspended() > would've worked ok already at that point. > > I wondered how the above detection would change in the system suspend > / > resume handlers but that looks ok as well with the disable_depth > check > in pm_runtime_suspended(): > Reviewed-by: Imre Deak > > > Acked-by: Jani Nikula > > Thank you Imre and Jani for checking my patch. This is now merged. BR, Jouni Högander > > > > > > > > Cc: Jani Nikula > > > Cc: Imre Deak > > > > > > Signed-off-by: Jouni Högander > > > --- > > > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +- > > > drivers/gpu/drm/i915/i915_driver.c | 3 --- > > > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 1 - > > > drivers/gpu/drm/i915/intel_runtime_pm.h | 4 ++-- > > > 5 files changed, 4 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c > > > b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 68cf5e6b0b46..889bb26009a2 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -216,7 +216,7 @@ bool __intel_display_power_is_enabled(struct > > > drm_i915_private *dev_priv, > > > struct i915_power_well *power_well; > > > bool is_enabled; > > > > > > - if (dev_priv->runtime_pm.suspended) > > > + if (pm_runtime_suspended(dev_priv->drm.dev)) > > > return false; > > > > > > is_enabled = true; > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > > b/drivers/gpu/drm/i915/i915_driver.c > > > index f8dbee7a5af7..cd98ee740976 100644 > > > --- a/drivers/gpu/drm/i915/i915_driver.c > > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > > @@ -1569,8 +1569,6 @@ static int intel_runtime_suspend(struct > > > device *kdev) > > > if (root_pdev) > > > pci_d3cold_disable(root_pdev); > > > > > > - rpm->suspended = true; > > > - > > > /* > > > * FIXME: We really should find a document that > > > references the arguments > > > * used below! > > > @@ -1621,7 +1619,6 @@ static int intel_runtime_resume(struct > > > device *kdev) > > > disable_rpm_wakeref_asserts(rpm); > > > > > > intel_opregion_notify_adapter(dev_priv, PCI_D0); > > > - rpm->suspended = false; > > > > > > root_pdev = pcie_find_root_port(pdev); > > > if (root_pdev) > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > index 4008bb09fdb5..a60bab177c55 100644 > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > @@ -1972,7 +1972,7 @@ static void capture_gen(struct > > > i915_gpu_coredump *error) > > > struct drm_i915_private *i915 = error->i915; > > > > > > error->wakelock = atomic_read( > > > >runtime_pm.wakeref_count); > > > - error->suspended = i915->runtime_pm.suspended; > > > + error->suspended = pm_runtime_suspended(i915->drm.dev); > > > > > > error->iommu = i915_vtd_active(i915); > > > error->reset_count = i915_reset_count(>gpu_error); > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index 6d8e5e5c0cba..8743153fad87 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -652,7 +652,6 @@ void intel_runtime_pm_init_early(struct > > > intel_runtime_pm *rpm) > > > > > > rpm->kdev = kdev; > > > rpm->available = HAS_RUNTIME_PM(i915); > > > - rpm->suspended = false; > > > atomic_set(>wakeref_count, 0); > > > > > > init_intel_runtime_pm_wakeref(rpm); > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h > > > b/drivers/gpu/drm/i915/intel_runtime_pm.h >
Re: [Intel-gfx] [PATCH] drm/i915: Remove runtime suspended boolean from intel_runtime_pm struct
On Wed, Sep 13, 2023 at 03:02:58PM +0300, Jani Nikula wrote:
> On Wed, 13 Sep 2023, Jouni Högander wrote:
> > It's not necessary to carry separate suspended status information in
> > intel_runtime_pm struct as this information is already in underlying device
> > structure. Remove it and use pm_runtime_suspended() to obtain suspended
> > status information when needed.
>
> I started wondering if this is racy, and my conclusion is that it's
> "less" racy than the original. rpm->suspended gets toggled in the middle
> of the suspend/resume sequences. So it could be halfway. Dunno if
> anything *after* those toggles depends on the state having been changed
> already; didn't find any. Maybe Imre has a better idea.
>
> Also, pm_runtime_suspended() seems more reliable when suspend/resume
> fails.
The flag was added in the initial runtime PM enabling patch to detect
MMIO accesses while runtime suspended, looks like pm_runtime_suspended()
would've worked ok already at that point.
I wondered how the above detection would change in the system suspend /
resume handlers but that looks ok as well with the disable_depth check
in pm_runtime_suspended():
Reviewed-by: Imre Deak
> Acked-by: Jani Nikula
>
>
> >
> > Cc: Jani Nikula
> > Cc: Imre Deak
> >
> > Signed-off-by: Jouni Högander
> > ---
> > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
> > drivers/gpu/drm/i915/i915_driver.c | 3 ---
> > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> > drivers/gpu/drm/i915/intel_runtime_pm.c| 1 -
> > drivers/gpu/drm/i915/intel_runtime_pm.h| 4 ++--
> > 5 files changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 68cf5e6b0b46..889bb26009a2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -216,7 +216,7 @@ bool __intel_display_power_is_enabled(struct
> > drm_i915_private *dev_priv,
> > struct i915_power_well *power_well;
> > bool is_enabled;
> >
> > - if (dev_priv->runtime_pm.suspended)
> > + if (pm_runtime_suspended(dev_priv->drm.dev))
> > return false;
> >
> > is_enabled = true;
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index f8dbee7a5af7..cd98ee740976 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1569,8 +1569,6 @@ static int intel_runtime_suspend(struct device *kdev)
> > if (root_pdev)
> > pci_d3cold_disable(root_pdev);
> >
> > - rpm->suspended = true;
> > -
> > /*
> > * FIXME: We really should find a document that references the arguments
> > * used below!
> > @@ -1621,7 +1619,6 @@ static int intel_runtime_resume(struct device *kdev)
> > disable_rpm_wakeref_asserts(rpm);
> >
> > intel_opregion_notify_adapter(dev_priv, PCI_D0);
> > - rpm->suspended = false;
> >
> > root_pdev = pcie_find_root_port(pdev);
> > if (root_pdev)
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 4008bb09fdb5..a60bab177c55 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1972,7 +1972,7 @@ static void capture_gen(struct i915_gpu_coredump
> > *error)
> > struct drm_i915_private *i915 = error->i915;
> >
> > error->wakelock = atomic_read(>runtime_pm.wakeref_count);
> > - error->suspended = i915->runtime_pm.suspended;
> > + error->suspended = pm_runtime_suspended(i915->drm.dev);
> >
> > error->iommu = i915_vtd_active(i915);
> > error->reset_count = i915_reset_count(>gpu_error);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 6d8e5e5c0cba..8743153fad87 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -652,7 +652,6 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm)
> >
> > rpm->kdev = kdev;
> > rpm->available = HAS_RUNTIME_PM(i915);
> > - rpm->suspended = false;
> > atomic_set(>wakeref_count, 0);
> >
> > init_intel_runtime_pm_wakeref(rpm);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index 764b183ae452..f79cda7a2503 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -6,6 +6,7 @@
> > #ifndef __INTEL_RUNTIME_PM_H__
> > #define __INTEL_RUNTIME_PM_H__
> >
> > +#include
> > #include
> >
> > #include "intel_wakeref.h"
> > @@ -43,7 +44,6 @@ struct intel_runtime_pm {
> > atomic_t wakeref_count;
> > struct device *kdev; /* points to i915->drm.dev */
> > bool available;
> > - bool suspended;
> > bool irqs_enabled;
> >
Re: [Intel-gfx] [PATCH] drm/i915: Remove runtime suspended boolean from intel_runtime_pm struct
On Wed, 13 Sep 2023, Jouni Högander wrote:
> It's not necessary to carry separate suspended status information in
> intel_runtime_pm struct as this information is already in underlying device
> structure. Remove it and use pm_runtime_suspended() to obtain suspended
> status information when needed.
I started wondering if this is racy, and my conclusion is that it's
"less" racy than the original. rpm->suspended gets toggled in the middle
of the suspend/resume sequences. So it could be halfway. Dunno if
anything *after* those toggles depends on the state having been changed
already; didn't find any. Maybe Imre has a better idea.
Also, pm_runtime_suspended() seems more reliable when suspend/resume
fails.
Acked-by: Jani Nikula
>
> Cc: Jani Nikula
> Cc: Imre Deak
>
> Signed-off-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
> drivers/gpu/drm/i915/i915_driver.c | 3 ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> drivers/gpu/drm/i915/intel_runtime_pm.c| 1 -
> drivers/gpu/drm/i915/intel_runtime_pm.h| 4 ++--
> 5 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 68cf5e6b0b46..889bb26009a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -216,7 +216,7 @@ bool __intel_display_power_is_enabled(struct
> drm_i915_private *dev_priv,
> struct i915_power_well *power_well;
> bool is_enabled;
>
> - if (dev_priv->runtime_pm.suspended)
> + if (pm_runtime_suspended(dev_priv->drm.dev))
> return false;
>
> is_enabled = true;
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index f8dbee7a5af7..cd98ee740976 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1569,8 +1569,6 @@ static int intel_runtime_suspend(struct device *kdev)
> if (root_pdev)
> pci_d3cold_disable(root_pdev);
>
> - rpm->suspended = true;
> -
> /*
>* FIXME: We really should find a document that references the arguments
>* used below!
> @@ -1621,7 +1619,6 @@ static int intel_runtime_resume(struct device *kdev)
> disable_rpm_wakeref_asserts(rpm);
>
> intel_opregion_notify_adapter(dev_priv, PCI_D0);
> - rpm->suspended = false;
>
> root_pdev = pcie_find_root_port(pdev);
> if (root_pdev)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 4008bb09fdb5..a60bab177c55 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1972,7 +1972,7 @@ static void capture_gen(struct i915_gpu_coredump *error)
> struct drm_i915_private *i915 = error->i915;
>
> error->wakelock = atomic_read(>runtime_pm.wakeref_count);
> - error->suspended = i915->runtime_pm.suspended;
> + error->suspended = pm_runtime_suspended(i915->drm.dev);
>
> error->iommu = i915_vtd_active(i915);
> error->reset_count = i915_reset_count(>gpu_error);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6d8e5e5c0cba..8743153fad87 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -652,7 +652,6 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm
> *rpm)
>
> rpm->kdev = kdev;
> rpm->available = HAS_RUNTIME_PM(i915);
> - rpm->suspended = false;
> atomic_set(>wakeref_count, 0);
>
> init_intel_runtime_pm_wakeref(rpm);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index 764b183ae452..f79cda7a2503 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -6,6 +6,7 @@
> #ifndef __INTEL_RUNTIME_PM_H__
> #define __INTEL_RUNTIME_PM_H__
>
> +#include
> #include
>
> #include "intel_wakeref.h"
> @@ -43,7 +44,6 @@ struct intel_runtime_pm {
> atomic_t wakeref_count;
> struct device *kdev; /* points to i915->drm.dev */
> bool available;
> - bool suspended;
> bool irqs_enabled;
> bool no_wakeref_tracking;
>
> @@ -110,7 +110,7 @@ intel_rpm_wakelock_count(int wakeref_count)
> static inline void
> assert_rpm_device_not_suspended(struct intel_runtime_pm *rpm)
> {
> - WARN_ONCE(rpm->suspended,
> + WARN_ONCE(pm_runtime_suspended(rpm->kdev),
> "Device suspended during HW access\n");
> }
--
Jani Nikula, Intel
[Intel-gfx] [PATCH] drm/i915: Remove runtime suspended boolean from intel_runtime_pm struct
It's not necessary to carry separate suspended status information in
intel_runtime_pm struct as this information is already in underlying device
structure. Remove it and use pm_runtime_suspended() to obtain suspended
status information when needed.
Cc: Jani Nikula
Cc: Imre Deak
Signed-off-by: Jouni Högander
---
drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
drivers/gpu/drm/i915/i915_driver.c | 3 ---
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/intel_runtime_pm.c| 1 -
drivers/gpu/drm/i915/intel_runtime_pm.h| 4 ++--
5 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
b/drivers/gpu/drm/i915/display/intel_display_power.c
index 68cf5e6b0b46..889bb26009a2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -216,7 +216,7 @@ bool __intel_display_power_is_enabled(struct
drm_i915_private *dev_priv,
struct i915_power_well *power_well;
bool is_enabled;
- if (dev_priv->runtime_pm.suspended)
+ if (pm_runtime_suspended(dev_priv->drm.dev))
return false;
is_enabled = true;
diff --git a/drivers/gpu/drm/i915/i915_driver.c
b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..cd98ee740976 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1569,8 +1569,6 @@ static int intel_runtime_suspend(struct device *kdev)
if (root_pdev)
pci_d3cold_disable(root_pdev);
- rpm->suspended = true;
-
/*
* FIXME: We really should find a document that references the arguments
* used below!
@@ -1621,7 +1619,6 @@ static int intel_runtime_resume(struct device *kdev)
disable_rpm_wakeref_asserts(rpm);
intel_opregion_notify_adapter(dev_priv, PCI_D0);
- rpm->suspended = false;
root_pdev = pcie_find_root_port(pdev);
if (root_pdev)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 4008bb09fdb5..a60bab177c55 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1972,7 +1972,7 @@ static void capture_gen(struct i915_gpu_coredump *error)
struct drm_i915_private *i915 = error->i915;
error->wakelock = atomic_read(>runtime_pm.wakeref_count);
- error->suspended = i915->runtime_pm.suspended;
+ error->suspended = pm_runtime_suspended(i915->drm.dev);
error->iommu = i915_vtd_active(i915);
error->reset_count = i915_reset_count(>gpu_error);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6d8e5e5c0cba..8743153fad87 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -652,7 +652,6 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm
*rpm)
rpm->kdev = kdev;
rpm->available = HAS_RUNTIME_PM(i915);
- rpm->suspended = false;
atomic_set(>wakeref_count, 0);
init_intel_runtime_pm_wakeref(rpm);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
b/drivers/gpu/drm/i915/intel_runtime_pm.h
index 764b183ae452..f79cda7a2503 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -6,6 +6,7 @@
#ifndef __INTEL_RUNTIME_PM_H__
#define __INTEL_RUNTIME_PM_H__
+#include
#include
#include "intel_wakeref.h"
@@ -43,7 +44,6 @@ struct intel_runtime_pm {
atomic_t wakeref_count;
struct device *kdev; /* points to i915->drm.dev */
bool available;
- bool suspended;
bool irqs_enabled;
bool no_wakeref_tracking;
@@ -110,7 +110,7 @@ intel_rpm_wakelock_count(int wakeref_count)
static inline void
assert_rpm_device_not_suspended(struct intel_runtime_pm *rpm)
{
- WARN_ONCE(rpm->suspended,
+ WARN_ONCE(pm_runtime_suspended(rpm->kdev),
"Device suspended during HW access\n");
}
--
2.34.1
