[21/21] drm/i915/slpc: Fail intel_runtime_suspend if SLPC or RPS not active

Submitted by tom.orourke@intel.com on April 28, 2016, 1:11 a.m.

Details

Message ID 1461805865-212590-22-git-send-email-tom.orourke@intel.com
State New
Headers show
Series "Add support for GuC-based SLPC" ( rev: 4 ) in Intel GFX

Not browsing as part of any series.

Commit Message

tom.orourke@intel.com April 28, 2016, 1:11 a.m.
From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

intel_runtime_suspend failed with warning if RPS was disabled.
With SLPC enabled, RPS is disabled. With SLPC, warning is now changed
to consider SLPC active status as well. This will ensure runtime suspend
proceeds when SLPC enabled.

v2: Commit message update. (Tom)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cc22fa0..00a2713 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1474,7 +1474,8 @@  static int intel_runtime_suspend(struct device *device)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
+	if (WARN_ON_ONCE(!((dev_priv->rps.enabled || intel_slpc_active(dev)) &&
+			   intel_enable_rc6(dev))))
 		return -ENODEV;
 
 	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))

Comments

On Wed, Apr 27, 2016 at 06:11:05PM -0700, tom.orourke@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> intel_runtime_suspend failed with warning if RPS was disabled.
> With SLPC enabled, RPS is disabled. With SLPC, warning is now changed
> to consider SLPC active status as well. This will ensure runtime suspend
> proceeds when SLPC enabled.
> 
> v2: Commit message update. (Tom)
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index cc22fa0..00a2713 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1474,7 +1474,8 @@ static int intel_runtime_suspend(struct device *device)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
> +	if (WARN_ON_ONCE(!((dev_priv->rps.enabled || intel_slpc_active(dev)) &&
> +			   intel_enable_rc6(dev))))

The real question here is why does runtime suspend depend on either of
these being enabled (espcially rps!).

Imre?
-Chris
On to, 2016-04-28 at 07:56 +0100, Chris Wilson wrote:
> On Wed, Apr 27, 2016 at 06:11:05PM -0700, tom.orourke@intel.com wrote:
> > From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > 
> > intel_runtime_suspend failed with warning if RPS was disabled.
> > With SLPC enabled, RPS is disabled. With SLPC, warning is now changed
> > to consider SLPC active status as well. This will ensure runtime suspend
> > proceeds when SLPC enabled.
> > 
> > v2: Commit message update. (Tom)
> > 
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index cc22fa0..00a2713 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1474,7 +1474,8 @@ static int intel_runtime_suspend(struct device *device)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
> > +	if (WARN_ON_ONCE(!((dev_priv->rps.enabled || intel_slpc_active(dev)) &&
> > +			   intel_enable_rc6(dev))))
> 
> The real question here is why does runtime suspend depend on either of
> these being enabled (espcially rps!).

We need RC6 enabled across a runtime suspend/resume since we depend on
the RC6 context to be retained across these transitions. There is no
separate knob for RPS and we enable RC6 and RPS together, that's why
rps.enabled is checked.

--Imre
On Thu, Apr 28, 2016 at 10:57:20AM +0300, Imre Deak wrote:
> On to, 2016-04-28 at 07:56 +0100, Chris Wilson wrote:
> > On Wed, Apr 27, 2016 at 06:11:05PM -0700, tom.orourke@intel.com wrote:
> > > From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > 
> > > intel_runtime_suspend failed with warning if RPS was disabled.
> > > With SLPC enabled, RPS is disabled. With SLPC, warning is now changed
> > > to consider SLPC active status as well. This will ensure runtime suspend
> > > proceeds when SLPC enabled.
> > > 
> > > v2: Commit message update. (Tom)
> > > 
> > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index cc22fa0..00a2713 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1474,7 +1474,8 @@ static int intel_runtime_suspend(struct device *device)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int ret;
> > >  
> > > -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
> > > +	if (WARN_ON_ONCE(!((dev_priv->rps.enabled || intel_slpc_active(dev)) &&
> > > +			   intel_enable_rc6(dev))))
> > 
> > The real question here is why does runtime suspend depend on either of
> > these being enabled (espcially rps!).
> 
> We need RC6 enabled across a runtime suspend/resume since we depend on
> the RC6 context to be retained across these transitions. There is no
> separate knob for RPS and we enable RC6 and RPS together, that's why
> rps.enabled is checked.

So, from the standpoint of this series, we should be separating the two
and giving rc6 its own bit of bookkeeping.
-Chris
On to, 2016-04-28 at 09:00 +0100, Chris Wilson wrote:
> On Thu, Apr 28, 2016 at 10:57:20AM +0300, Imre Deak wrote:
> > On to, 2016-04-28 at 07:56 +0100, Chris Wilson wrote:
> > > On Wed, Apr 27, 2016 at 06:11:05PM -0700, tom.orourke@intel.com
> > > wrote:
> > > > From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > 
> > > > intel_runtime_suspend failed with warning if RPS was disabled.
> > > > With SLPC enabled, RPS is disabled. With SLPC, warning is now
> > > > changed
> > > > to consider SLPC active status as well. This will ensure
> > > > runtime suspend
> > > > proceeds when SLPC enabled.
> > > > 
> > > > v2: Commit message update. (Tom)
> > > > 
> > > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index cc22fa0..00a2713 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1474,7 +1474,8 @@ static int intel_runtime_suspend(struct
> > > > device *device)
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	int ret;
> > > >  
> > > > -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled &&
> > > > intel_enable_rc6(dev))))
> > > > +	if (WARN_ON_ONCE(!((dev_priv->rps.enabled ||
> > > > intel_slpc_active(dev)) &&
> > > > +			   intel_enable_rc6(dev))))
> > > 
> > > The real question here is why does runtime suspend depend on
> > > either of
> > > these being enabled (espcially rps!).
> > 
> > We need RC6 enabled across a runtime suspend/resume since we depend
> > on
> > the RC6 context to be retained across these transitions. There is
> > no
> > separate knob for RPS and we enable RC6 and RPS together, that's
> > why
> > rps.enabled is checked.
> 
> So, from the standpoint of this series, we should be separating the
> two
> and giving rc6 its own bit of bookkeeping.

Yes, separating RC6 and RPS enabling would be the clean way for this
and other purposes too. This patchset enables RC6
from intel_enable_gt_powersave() directly in case SLPC is enabled but
schedules a work for enabling RC6 if SLPC is not enabled. So while the
above works it could be done in a cleaner way.

--Imre