Re: [Intel-gfx] [PATCH] drm/i915: don't enable autosuspend on platforms without RPM support

[David Weinehall]

On Thu, Dec 17, 2015 at 07:04:33PM +0200, Imre Deak wrote:
> Since
> 
> commit 357597e51dd1aa3cf764d322abf89217e3dcd7bb
> Author: Imre Deak 
> Date:   Tue Nov 10 06:12:22 2015 +0200
> 
> drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
> 
> this file is writeable also on platforms without RPM support, but
> userspace (at least IGT) depends on this file being unchangable to
> determine whether the device supports autosuspend. So restore the old
> behavior.
> 
> This gets rid of igt/pm_rpm failures on old platforms without RPM
> support, where the test should be skipped.
> 
> Testcase: igt/pm_rpm/basic-rte
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index a0b9eaf..ddbdbff 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2309,18 +2309,21 @@ void intel_runtime_pm_enable(struct drm_i915_private 
> *dev_priv)
>   struct drm_device *dev = dev_priv->dev;
>   struct device *device = >pdev->dev;
>  
> + pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> + pm_runtime_mark_last_busy(device);
> +
>   /*
>* Take a permanent reference to disable the RPM functionality and drop
>* it only when unloading the driver. Use the low level get/put helpers,
>* so the driver's own RPM reference tracking asserts also work on
>* platforms without RPM support.
>*/
> - if (!HAS_RUNTIME_PM(dev))
> + if (!HAS_RUNTIME_PM(dev)) {
> + pm_runtime_dont_use_autosuspend(device);
>   pm_runtime_get_sync(device);
> -
> - pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> - pm_runtime_mark_last_busy(device);
> - pm_runtime_use_autosuspend(device);
> + } else {
> + pm_runtime_use_autosuspend(device);
> + }
>  
>   /*
>* The core calls the driver load handler with an RPM reference held.

Reviewed-by: David Weinehall 


Kind regards, David Weinehall
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: don't enable autosuspend on platforms without RPM support

[Imre Deak]

On pe, 2015-12-18 at 12:32 +0200, David Weinehall wrote:
> On Thu, Dec 17, 2015 at 07:04:33PM +0200, Imre Deak wrote:
> > Since
> > 
> > commit 357597e51dd1aa3cf764d322abf89217e3dcd7bb
> > Author: Imre Deak 
> > Date:   Tue Nov 10 06:12:22 2015 +0200
> > 
> > drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert
> > helpers
> > 
> > this file is writeable also on platforms without RPM support, but
> > userspace (at least IGT) depends on this file being unchangable to
> > determine whether the device supports autosuspend. So restore the
> > old
> > behavior.
> > 
> > This gets rid of igt/pm_rpm failures on old platforms without RPM
> > support, where the test should be skipped.
> > 
> > Testcase: igt/pm_rpm/basic-rte
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index a0b9eaf..ddbdbff 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2309,18 +2309,21 @@ void intel_runtime_pm_enable(struct
> > drm_i915_private *dev_priv)
> >     struct drm_device *dev = dev_priv->dev;
> >     struct device *device = >pdev->dev;
> >  
> > +   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> > +   pm_runtime_mark_last_busy(device);
> > +
> >     /*
> >      * Take a permanent reference to disable the RPM
> > functionality and drop
> >      * it only when unloading the driver. Use the low level
> > get/put helpers,
> >      * so the driver's own RPM reference tracking asserts also
> > work on
> >      * platforms without RPM support.
> >      */
> > -   if (!HAS_RUNTIME_PM(dev))
> > +   if (!HAS_RUNTIME_PM(dev)) {
> > +   pm_runtime_dont_use_autosuspend(device);
> >     pm_runtime_get_sync(device);
> > -
> > -   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> > -   pm_runtime_mark_last_busy(device);
> > -   pm_runtime_use_autosuspend(device);
> > +   } else {
> > +   pm_runtime_use_autosuspend(device);
> > +   }
> >  
> >     /*
> >      * The core calls the driver load handler with an RPM
> > reference held.
> 
> Reviewed-by: David Weinehall 

Thanks for the review, I pushed the patch to dinq.
While applying I clarified the commit message about which sysfs file
this change is about and fixed the reference to the regressing commit,
it was one before the commit I mentioned.

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

Re: [Intel-gfx] [PATCH] drm/i915: don't enable autosuspend on platforms without RPM support

[Imre Deak]

On to, 2015-12-17 at 17:19 +, Chris Wilson wrote:
> On Thu, Dec 17, 2015 at 07:04:33PM +0200, Imre Deak wrote:
> > Since
> > 
> > commit 357597e51dd1aa3cf764d322abf89217e3dcd7bb
> > Author: Imre Deak 
> > Date:   Tue Nov 10 06:12:22 2015 +0200
> > 
> > drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert
> > helpers
> > 
> > this file is writeable also on platforms without RPM support, but
> 
> Which file?

sysfs power/autosuspend_delay_ms is only writeable by userspace if we
call pm_runtime_use_autosuspend().

> 
> > userspace (at least IGT) depends on this file being unchangable to
> > determine whether the device supports autosuspend. So restore the
> > old
> > behavior.
> > 
> > This gets rid of igt/pm_rpm failures on old platforms without RPM
> > support, where the test should be skipped.
> > 
> > Testcase: igt/pm_rpm/basic-rte
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index a0b9eaf..ddbdbff 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2309,18 +2309,21 @@ void intel_runtime_pm_enable(struct
> > drm_i915_private *dev_priv)
> >     struct drm_device *dev = dev_priv->dev;
> >     struct device *device = >pdev->dev;
> >  
> > +   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> > +   pm_runtime_mark_last_busy(device);
> > +
> >     /*
> >      * Take a permanent reference to disable the RPM
> > functionality and drop
> >      * it only when unloading the driver. Use the low level
> > get/put helpers,
> >      * so the driver's own RPM reference tracking asserts also
> > work on
> >      * platforms without RPM support.
> >      */
> > -   if (!HAS_RUNTIME_PM(dev))
> > +   if (!HAS_RUNTIME_PM(dev)) {
> > +   pm_runtime_dont_use_autosuspend(device);
> >     pm_runtime_get_sync(device);
> > -
> > -   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> > -   pm_runtime_mark_last_busy(device);
> > -   pm_runtime_use_autosuspend(device);
> > +   } else {
> > +   pm_runtime_use_autosuspend(device);
> > +   }
> 
> This seems to make sense to me.
> -Chris
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: don't enable autosuspend on platforms without RPM support

[Chris Wilson]

On Thu, Dec 17, 2015 at 07:04:33PM +0200, Imre Deak wrote:
> Since
> 
> commit 357597e51dd1aa3cf764d322abf89217e3dcd7bb
> Author: Imre Deak 
> Date:   Tue Nov 10 06:12:22 2015 +0200
> 
> drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
> 
> this file is writeable also on platforms without RPM support, but

Which file?

> userspace (at least IGT) depends on this file being unchangable to
> determine whether the device supports autosuspend. So restore the old
> behavior.
> 
> This gets rid of igt/pm_rpm failures on old platforms without RPM
> support, where the test should be skipped.
> 
> Testcase: igt/pm_rpm/basic-rte
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index a0b9eaf..ddbdbff 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2309,18 +2309,21 @@ void intel_runtime_pm_enable(struct drm_i915_private 
> *dev_priv)
>   struct drm_device *dev = dev_priv->dev;
>   struct device *device = >pdev->dev;
>  
> + pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> + pm_runtime_mark_last_busy(device);
> +
>   /*
>* Take a permanent reference to disable the RPM functionality and drop
>* it only when unloading the driver. Use the low level get/put helpers,
>* so the driver's own RPM reference tracking asserts also work on
>* platforms without RPM support.
>*/
> - if (!HAS_RUNTIME_PM(dev))
> + if (!HAS_RUNTIME_PM(dev)) {
> + pm_runtime_dont_use_autosuspend(device);
>   pm_runtime_get_sync(device);
> -
> - pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
> - pm_runtime_mark_last_busy(device);
> - pm_runtime_use_autosuspend(device);
> + } else {
> + pm_runtime_use_autosuspend(device);
> + }

This seems to make sense to me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: don't enable autosuspend on platforms without RPM support

[Imre Deak]

Since

commit 357597e51dd1aa3cf764d322abf89217e3dcd7bb
Author: Imre Deak 
Date:   Tue Nov 10 06:12:22 2015 +0200

drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

this file is writeable also on platforms without RPM support, but
userspace (at least IGT) depends on this file being unchangable to
determine whether the device supports autosuspend. So restore the old
behavior.

This gets rid of igt/pm_rpm failures on old platforms without RPM
support, where the test should be skipped.

Testcase: igt/pm_rpm/basic-rte
Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index a0b9eaf..ddbdbff 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2309,18 +2309,21 @@ void intel_runtime_pm_enable(struct drm_i915_private 
*dev_priv)
struct drm_device *dev = dev_priv->dev;
struct device *device = >pdev->dev;
 
+   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
+   pm_runtime_mark_last_busy(device);
+
/*
 * Take a permanent reference to disable the RPM functionality and drop
 * it only when unloading the driver. Use the low level get/put helpers,
 * so the driver's own RPM reference tracking asserts also work on
 * platforms without RPM support.
 */
-   if (!HAS_RUNTIME_PM(dev))
+   if (!HAS_RUNTIME_PM(dev)) {
+   pm_runtime_dont_use_autosuspend(device);
pm_runtime_get_sync(device);
-
-   pm_runtime_set_autosuspend_delay(device, 1); /* 10s */
-   pm_runtime_mark_last_busy(device);
-   pm_runtime_use_autosuspend(device);
+   } else {
+   pm_runtime_use_autosuspend(device);
+   }
 
/*
 * The core calls the driver load handler with an RPM reference held.
-- 
2.5.0

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