All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support
@ 2014-08-18 10:20 Imre Deak
  2014-08-19  2:52 ` Sagar Arun Kamble
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Imre Deak @ 2014-08-18 10:20 UTC (permalink / raw)
  To: intel-gfx; +Cc: Sagar Kamble

Before sharing common parts between the system and runtime s/r
handlers we WARNed if the runtime s/r handlers were called on GENs that
didn't support RPM. But this WARN is not correct if the same handler is
called from the system s/r path, since that can happen on any platform.
This also broke system s/r on old platforms.

The issue was introduced in

commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble <sagar.a.kamble@intel.com>
Date:   Wed Aug 13 23:07:06 2014 +0530

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 117f5c1..f646f34 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -499,7 +499,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 }
 
 
-static int intel_suspend_complete(struct drm_i915_private *dev_priv);
+static int intel_suspend_complete(struct drm_i915_private *dev_priv,
+				  bool rpm_suspend);
 static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 				bool rpm_resume);
 
@@ -909,7 +910,7 @@ static int i915_pm_suspend_late(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	ret = intel_suspend_complete(dev_priv);
+	ret = intel_suspend_complete(dev_priv, false);
 
 	if (ret)
 		DRM_ERROR("Suspend complete failed: %d\n", ret);
@@ -1410,7 +1411,7 @@ static int intel_runtime_suspend(struct device *device)
 	cancel_work_sync(&dev_priv->rps.work);
 	intel_runtime_pm_disable_interrupts(dev);
 
-	ret = intel_suspend_complete(dev_priv);
+	ret = intel_suspend_complete(dev_priv, true);
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_restore_interrupts(dev);
@@ -1471,10 +1472,11 @@ static int intel_runtime_resume(struct device *device)
  * This function implements common functionality of runtime and system
  * suspend sequence.
  */
-static int intel_suspend_complete(struct drm_i915_private *dev_priv)
+static int intel_suspend_complete(struct drm_i915_private *dev_priv,
+				  bool rpm_suspend)
 {
 	struct drm_device *dev = dev_priv->dev;
-	int ret;
+	int ret = 0;
 
 	if (IS_GEN6(dev)) {
 		ret = 0;
@@ -1482,7 +1484,7 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
 		ret = hsw_suspend_complete(dev_priv);
 	} else if (IS_VALLEYVIEW(dev)) {
 		ret = vlv_suspend_complete(dev_priv);
-	} else {
+	} else if (rpm_suspend) {
 		ret = -ENODEV;
 		WARN_ON(1);
 	}
@@ -1499,7 +1501,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 				bool rpm_resume)
 {
 	struct drm_device *dev = dev_priv->dev;
-	int ret;
+	int ret = 0;
 
 	if (IS_GEN6(dev)) {
 		ret = snb_resume_prepare(dev_priv, rpm_resume);
@@ -1507,7 +1509,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 		ret = hsw_resume_prepare(dev_priv, rpm_resume);
 	} else if (IS_VALLEYVIEW(dev)) {
 		ret = vlv_resume_prepare(dev_priv, rpm_resume);
-	} else {
+	} else if (rpm_resume) {
 		WARN_ON(1);
 		ret = -ENODEV;
 	}
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-18 10:20 [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support Imre Deak
@ 2014-08-19  2:52 ` Sagar Arun Kamble
  2014-08-26  7:44 ` Daniel Vetter
  2014-08-26 10:26 ` [PATCH v2] " Imre Deak
  2 siblings, 0 replies; 7+ messages in thread
From: Sagar Arun Kamble @ 2014-08-19  2:52 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

Oh. I also missed this side effect of paths converging.

Reviewed-by: Sagar Kamble <sagar.a.kamble@intel.com>

On Mon, 2014-08-18 at 13:20 +0300, Imre Deak wrote:
> Before sharing common parts between the system and runtime s/r
> handlers we WARNed if the runtime s/r handlers were called on GENs that
> didn't support RPM. But this WARN is not correct if the same handler is
> called from the system s/r path, since that can happen on any platform.
> This also broke system s/r on old platforms.
> 
> The issue was introduced in
> 
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 117f5c1..f646f34 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -499,7 +499,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  }
>  
> 
> -static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv,
> +				  bool rpm_suspend);
>  static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  				bool rpm_resume);
>  
> @@ -909,7 +910,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> -	ret = intel_suspend_complete(dev_priv);
> +	ret = intel_suspend_complete(dev_priv, false);
>  
>  	if (ret)
>  		DRM_ERROR("Suspend complete failed: %d\n", ret);
> @@ -1410,7 +1411,7 @@ static int intel_runtime_suspend(struct device *device)
>  	cancel_work_sync(&dev_priv->rps.work);
>  	intel_runtime_pm_disable_interrupts(dev);
>  
> -	ret = intel_suspend_complete(dev_priv);
> +	ret = intel_suspend_complete(dev_priv, true);
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>  		intel_runtime_pm_restore_interrupts(dev);
> @@ -1471,10 +1472,11 @@ static int intel_runtime_resume(struct device *device)
>   * This function implements common functionality of runtime and system
>   * suspend sequence.
>   */
> -static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv,
> +				  bool rpm_suspend)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> -	int ret;
> +	int ret = 0;
>  
>  	if (IS_GEN6(dev)) {
>  		ret = 0;
> @@ -1482,7 +1484,7 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
>  		ret = hsw_suspend_complete(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		ret = vlv_suspend_complete(dev_priv);
> -	} else {
> +	} else if (rpm_suspend) {
>  		ret = -ENODEV;
>  		WARN_ON(1);
>  	}
> @@ -1499,7 +1501,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  				bool rpm_resume)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> -	int ret;
> +	int ret = 0;
>  
>  	if (IS_GEN6(dev)) {
>  		ret = snb_resume_prepare(dev_priv, rpm_resume);
> @@ -1507,7 +1509,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  		ret = hsw_resume_prepare(dev_priv, rpm_resume);
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		ret = vlv_resume_prepare(dev_priv, rpm_resume);
> -	} else {
> +	} else if (rpm_resume) {
>  		WARN_ON(1);
>  		ret = -ENODEV;
>  	}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-18 10:20 [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support Imre Deak
  2014-08-19  2:52 ` Sagar Arun Kamble
@ 2014-08-26  7:44 ` Daniel Vetter
  2014-08-26  7:45   ` Daniel Vetter
  2014-08-26 10:26 ` [PATCH v2] " Imre Deak
  2 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2014-08-26  7:44 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx, Sagar Kamble

On Mon, Aug 18, 2014 at 01:20:06PM +0300, Imre Deak wrote:
> Before sharing common parts between the system and runtime s/r
> handlers we WARNed if the runtime s/r handlers were called on GENs that
> didn't support RPM. But this WARN is not correct if the same handler is
> called from the system s/r path, since that can happen on any platform.
> This also broke system s/r on old platforms.
> 
> The issue was introduced in
> 
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Adding boolean arguments to control warnings always feels a bit too much
like just shutting up the warnings. Can't we instead wrap the relevant
calls into HAS_RUNTIME_PM checks? Imo that would also lead to clearer code
by making the intention clear - with this you essentially have to git
blame to figure out why we sometimes disable the warning.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 117f5c1..f646f34 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -499,7 +499,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  }
>  
>  
> -static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv,
> +				  bool rpm_suspend);
>  static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  				bool rpm_resume);
>  
> @@ -909,7 +910,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> -	ret = intel_suspend_complete(dev_priv);
> +	ret = intel_suspend_complete(dev_priv, false);
>  
>  	if (ret)
>  		DRM_ERROR("Suspend complete failed: %d\n", ret);
> @@ -1410,7 +1411,7 @@ static int intel_runtime_suspend(struct device *device)
>  	cancel_work_sync(&dev_priv->rps.work);
>  	intel_runtime_pm_disable_interrupts(dev);
>  
> -	ret = intel_suspend_complete(dev_priv);
> +	ret = intel_suspend_complete(dev_priv, true);
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>  		intel_runtime_pm_restore_interrupts(dev);
> @@ -1471,10 +1472,11 @@ static int intel_runtime_resume(struct device *device)
>   * This function implements common functionality of runtime and system
>   * suspend sequence.
>   */
> -static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv,
> +				  bool rpm_suspend)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> -	int ret;
> +	int ret = 0;
>  
>  	if (IS_GEN6(dev)) {
>  		ret = 0;
> @@ -1482,7 +1484,7 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
>  		ret = hsw_suspend_complete(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		ret = vlv_suspend_complete(dev_priv);
> -	} else {
> +	} else if (rpm_suspend) {
>  		ret = -ENODEV;
>  		WARN_ON(1);
>  	}
> @@ -1499,7 +1501,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  				bool rpm_resume)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> -	int ret;
> +	int ret = 0;
>  
>  	if (IS_GEN6(dev)) {
>  		ret = snb_resume_prepare(dev_priv, rpm_resume);
> @@ -1507,7 +1509,7 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  		ret = hsw_resume_prepare(dev_priv, rpm_resume);
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		ret = vlv_resume_prepare(dev_priv, rpm_resume);
> -	} else {
> +	} else if (rpm_resume) {
>  		WARN_ON(1);
>  		ret = -ENODEV;
>  	}
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-26  7:44 ` Daniel Vetter
@ 2014-08-26  7:45   ` Daniel Vetter
  2014-08-26  9:01     ` Imre Deak
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2014-08-26  7:45 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx, Sagar Kamble

On Tue, Aug 26, 2014 at 09:44:42AM +0200, Daniel Vetter wrote:
> On Mon, Aug 18, 2014 at 01:20:06PM +0300, Imre Deak wrote:
> > Before sharing common parts between the system and runtime s/r
> > handlers we WARNed if the runtime s/r handlers were called on GENs that
> > didn't support RPM. But this WARN is not correct if the same handler is
> > called from the system s/r path, since that can happen on any platform.
> > This also broke system s/r on old platforms.
> > 
> > The issue was introduced in
> > 
> > commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> > Author: Sagar Kamble <sagar.a.kamble@intel.com>
> > Date:   Wed Aug 13 23:07:06 2014 +0530
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Adding boolean arguments to control warnings always feels a bit too much
> like just shutting up the warnings. Can't we instead wrap the relevant
> calls into HAS_RUNTIME_PM checks? Imo that would also lead to clearer code
> by making the intention clear - with this you essentially have to git
> blame to figure out why we sometimes disable the warning.

Also the patch subject is a bit misleading - we only shut up a wrong
warning, it's not a code fix.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-26  7:45   ` Daniel Vetter
@ 2014-08-26  9:01     ` Imre Deak
  0 siblings, 0 replies; 7+ messages in thread
From: Imre Deak @ 2014-08-26  9:01 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, Sagar Kamble


[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]

On Tue, 2014-08-26 at 09:45 +0200, Daniel Vetter wrote:
> On Tue, Aug 26, 2014 at 09:44:42AM +0200, Daniel Vetter wrote:
> > On Mon, Aug 18, 2014 at 01:20:06PM +0300, Imre Deak wrote:
> > > Before sharing common parts between the system and runtime s/r
> > > handlers we WARNed if the runtime s/r handlers were called on GENs that
> > > didn't support RPM. But this WARN is not correct if the same handler is
> > > called from the system s/r path, since that can happen on any platform.
> > > This also broke system s/r on old platforms.
> > > 
> > > The issue was introduced in
> > > 
> > > commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> > > Author: Sagar Kamble <sagar.a.kamble@intel.com>
> > > Date:   Wed Aug 13 23:07:06 2014 +0530
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Adding boolean arguments to control warnings always feels a bit too much
> > like just shutting up the warnings. Can't we instead wrap the relevant
> > calls into HAS_RUNTIME_PM checks?

I could instead remove the WARN from intel_suspend_complete/resume, and
do an early return from intel_runtime_suspend/resume for
!HAS_RUNTIME_PM(). Atm we only WARN there.

> > Imo that would also lead to clearer code
> > by making the intention clear - with this you essentially have to git
> > blame to figure out why we sometimes disable the warning.
> 
> Also the patch subject is a bit misleading - we only shut up a wrong
> warning, it's not a code fix.

We return -ENODEV for old GENs which breaks system suspend for them.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-18 10:20 [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support Imre Deak
  2014-08-19  2:52 ` Sagar Arun Kamble
  2014-08-26  7:44 ` Daniel Vetter
@ 2014-08-26 10:26 ` Imre Deak
  2014-08-26 10:53   ` Daniel Vetter
  2 siblings, 1 reply; 7+ messages in thread
From: Imre Deak @ 2014-08-26 10:26 UTC (permalink / raw)
  To: intel-gfx

Before sharing common parts between the system and runtime s/r
handlers we WARNed if the runtime s/r handlers were called on GENs that
didn't support RPM. But this WARN is not correct if the same handler is
called from the system s/r path, since that can happen on any platform.
This also broke system s/r on old platforms.

The issue was introduced in

commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble <sagar.a.kamble@intel.com>
Date:   Wed Aug 13 23:07:06 2014 +0530

v2:
- remove the WARN and depend on the HAS_RUNTIME_PM check in
  rutime_suspend/resume instead (Daniel)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index de1b664..b5fd1c5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1412,7 +1412,9 @@ static int intel_runtime_suspend(struct device *device)
 	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
 		return -ENODEV;
 
-	WARN_ON(!HAS_RUNTIME_PM(dev));
+	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))
+		return -ENODEV;
+
 	assert_force_wake_inactive(dev_priv);
 
 	DRM_DEBUG_KMS("Suspending device\n");
@@ -1480,7 +1482,8 @@ static int intel_runtime_resume(struct device *device)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	WARN_ON(!HAS_RUNTIME_PM(dev));
+	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))
+		return -ENODEV;
 
 	DRM_DEBUG_KMS("Resuming device\n");
 
@@ -1515,16 +1518,12 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	int ret;
 
-	if (IS_GEN6(dev)) {
-		ret = 0;
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		ret = hsw_suspend_complete(dev_priv);
-	} else if (IS_VALLEYVIEW(dev)) {
+	else if (IS_VALLEYVIEW(dev))
 		ret = vlv_suspend_complete(dev_priv);
-	} else {
-		ret = -ENODEV;
-		WARN_ON(1);
-	}
+	else
+		ret = 0;
 
 	return ret;
 }
@@ -1540,16 +1539,14 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	int ret;
 
-	if (IS_GEN6(dev)) {
+	if (IS_GEN6(dev))
 		ret = snb_resume_prepare(dev_priv, rpm_resume);
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		ret = hsw_resume_prepare(dev_priv, rpm_resume);
-	} else if (IS_VALLEYVIEW(dev)) {
+	else if (IS_VALLEYVIEW(dev))
 		ret = vlv_resume_prepare(dev_priv, rpm_resume);
-	} else {
-		WARN_ON(1);
-		ret = -ENODEV;
-	}
+	else
+		ret = 0;
 
 	return ret;
 }
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/i915: fix suspend/resume for GENs w/o runtime PM support
  2014-08-26 10:26 ` [PATCH v2] " Imre Deak
@ 2014-08-26 10:53   ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2014-08-26 10:53 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

On Tue, Aug 26, 2014 at 01:26:56PM +0300, Imre Deak wrote:
> Before sharing common parts between the system and runtime s/r
> handlers we WARNed if the runtime s/r handlers were called on GENs that
> didn't support RPM. But this WARN is not correct if the same handler is
> called from the system s/r path, since that can happen on any platform.
> This also broke system s/r on old platforms.
> 
> The issue was introduced in
> 
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530

Aside: I just realized that this patch adds an rpm_resume paramter to the
platform resume functions. Somehow I've been blind and didn't spot that
one. Not a good idea imo, since that still means we have piles of
arbitrary differences between the different platforms and code paths.

> v2:
> - remove the WARN and depend on the HAS_RUNTIME_PM check in
>   rutime_suspend/resume instead (Daniel)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82751
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index de1b664..b5fd1c5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1412,7 +1412,9 @@ static int intel_runtime_suspend(struct device *device)
>  	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6(dev))))
>  		return -ENODEV;
>  
> -	WARN_ON(!HAS_RUNTIME_PM(dev));
> +	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))
> +		return -ENODEV;
> +
>  	assert_force_wake_inactive(dev_priv);
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
> @@ -1480,7 +1482,8 @@ static int intel_runtime_resume(struct device *device)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	WARN_ON(!HAS_RUNTIME_PM(dev));
> +	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))
> +		return -ENODEV;
>  
>  	DRM_DEBUG_KMS("Resuming device\n");
>  
> @@ -1515,16 +1518,12 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	int ret;
>  
> -	if (IS_GEN6(dev)) {
> -		ret = 0;
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		ret = hsw_suspend_complete(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> +	else if (IS_VALLEYVIEW(dev))
>  		ret = vlv_suspend_complete(dev_priv);
> -	} else {
> -		ret = -ENODEV;
> -		WARN_ON(1);
> -	}
> +	else
> +		ret = 0;
>  
>  	return ret;
>  }
> @@ -1540,16 +1539,14 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  	struct drm_device *dev = dev_priv->dev;
>  	int ret;
>  
> -	if (IS_GEN6(dev)) {
> +	if (IS_GEN6(dev))
>  		ret = snb_resume_prepare(dev_priv, rpm_resume);
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		ret = hsw_resume_prepare(dev_priv, rpm_resume);
> -	} else if (IS_VALLEYVIEW(dev)) {
> +	else if (IS_VALLEYVIEW(dev))
>  		ret = vlv_resume_prepare(dev_priv, rpm_resume);
> -	} else {
> -		WARN_ON(1);
> -		ret = -ENODEV;
> -	}
> +	else
> +		ret = 0;
>  
>  	return ret;
>  }
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-08-26 10:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-18 10:20 [PATCH] drm/i915: fix suspend/resume for GENs w/o runtime PM support Imre Deak
2014-08-19  2:52 ` Sagar Arun Kamble
2014-08-26  7:44 ` Daniel Vetter
2014-08-26  7:45   ` Daniel Vetter
2014-08-26  9:01     ` Imre Deak
2014-08-26 10:26 ` [PATCH v2] " Imre Deak
2014-08-26 10:53   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.