[PATCH v2 1/2] PM / Domains: Allow genpd to power on during the system PM phase

2016-05-13 Thread Kevin Hilman
Ulf Hansson  writes:

> On 11 May 2016 at 23:25, Rafael J. Wysocki  wrote:
>> On Wed, May 11, 2016 at 10:00 AM, Ulf Hansson  
>> wrote:
>>> If the PM domain is powered off when the first device in the domain starts
>>> its system PM prepare phase, genpd prevents any further attempts to power
>>> on the PM domain during the system PM phase. This constraint affects not
>>> only the current device which is being prepared, but all devices within
>>> the same PM domain.
>>
>> What exactly do you mean by "the system PM phase"?
>
> In between system PM device prepare and system PM device complete.
>
> Do you want me to clarify this in a better way in the change log?

It might be more clear if you something like "during system suspend/resume"

Kevin


[PATCH v2 1/2] PM / Domains: Allow genpd to power on during the system PM phase

2016-05-12 Thread Ulf Hansson
On 11 May 2016 at 23:25, Rafael J. Wysocki  wrote:
> On Wed, May 11, 2016 at 10:00 AM, Ulf Hansson  
> wrote:
>> If the PM domain is powered off when the first device in the domain starts
>> its system PM prepare phase, genpd prevents any further attempts to power
>> on the PM domain during the system PM phase. This constraint affects not
>> only the current device which is being prepared, but all devices within
>> the same PM domain.
>
> What exactly do you mean by "the system PM phase"?

In between system PM device prepare and system PM device complete.

Do you want me to clarify this in a better way in the change log?

Kind regards
Uffe


[PATCH v2 1/2] PM / Domains: Allow genpd to power on during the system PM phase

2016-05-11 Thread Rafael J. Wysocki
On Wed, May 11, 2016 at 10:00 AM, Ulf Hansson  wrote:
> If the PM domain is powered off when the first device in the domain starts
> its system PM prepare phase, genpd prevents any further attempts to power
> on the PM domain during the system PM phase. This constraint affects not
> only the current device which is being prepared, but all devices within
> the same PM domain.

What exactly do you mean by "the system PM phase"?


[PATCH v2 1/2] PM / Domains: Allow genpd to power on during the system PM phase

2016-05-11 Thread Ulf Hansson
If the PM domain is powered off when the first device in the domain starts
its system PM prepare phase, genpd prevents any further attempts to power
on the PM domain during the system PM phase. This constraint affects not
only the current device which is being prepared, but all devices within
the same PM domain.

This behaviour needs to be changed, as a subsystem/driver for a device in
the same PM domain may still need to be able to serve requests in the
system PM phase. Accordingly, it may need to runtime resume its device and
thus also request the corresponding PM domain to be powered on.

To deal with these scenarios, let's make the device operational in the
system PM prepare phase by runtime resuming it, no matter if the PM domain
is powered on or off. Changing this also enables us to remove genpd's
suspend_power_off flag, as it's being used to track this condition.
Additionally, we must allow the PM domain to be powered on via runtime PM
during the system PM phase.

This change also requires a fix in the AMD ACP (Audio CoProcessor) drm
driver. It registers a genpd to model the ACP as a PM domain, but
unfortunately it's also abuses genpd's "internal" suspend_power_off flag
to deal with a corner case at system PM resume.

More precisely, the so called SMU block powers on the ACP at system PM
resume, unconditionally if it's being used or not. This may lead to that
genpd's internal status of the power state, may not correctly reflect the
power state of the HW after a system PM resume.

Because of changing the behaviour of genpd, by runtime resuming devices in
the prepare phase, the AMD ACP drm driver no longer have to deal with this
corner case. So let's just drop the related code in this driver.

Cc: David Airlie 
Cc: Alex Deucher 
Cc: Christian König 
Cc: Maruthi Srinivas Bayyavarapu 
Cc: dri-devel at lists.freedesktop.org
Signed-off-by: Ulf Hansson 
---

Changes in v2:
- Updated changelog.
- Added a fix in the AMD ACP (Audio CoProcessor) drm driver, which
registers a genpd. The fix removes the usage of genpd's internal
suspend_power_off flag as it's not needed after this change. Because of
this change I am also requesting an ack from the drm driver maintainer.

---
 drivers/base/power/domain.c | 84 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 23 -
 include/linux/pm_domain.h   |  1 -
 3 files changed, 31 insertions(+), 77 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index de23b64..a608f52 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -187,8 +187,7 @@ static int genpd_poweron(struct generic_pm_domain *genpd, 
unsigned int depth)
struct gpd_link *link;
int ret = 0;

-   if (genpd->status == GPD_STATE_ACTIVE
-   || (genpd->prepared_count > 0 && genpd->suspend_power_off))
+   if (genpd->status == GPD_STATE_ACTIVE)
return 0;

/*
@@ -735,21 +734,22 @@ static int pm_genpd_prepare(struct device *dev)

mutex_lock(&genpd->lock);

-   if (genpd->prepared_count++ == 0) {
+   if (genpd->prepared_count++ == 0)
genpd->suspended_count = 0;
-   genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
-   }

mutex_unlock(&genpd->lock);

-   if (genpd->suspend_power_off)
-   return 0;
-
/*
-* The PM domain must be in the GPD_STATE_ACTIVE state at this point,
-* so genpd_poweron() will return immediately, but if the device
-* is suspended (e.g. it's been stopped by genpd_stop_dev()), we need
-* to make it operational.
+* Even if the PM domain is powered off at this point, we can't expect
+* it to remain in that state during the entire system PM suspend
+* phase. Any subsystem/driver for a device in the PM domain, may still
+* need to serve a request which may require the device to be runtime
+* resumed and its PM domain to be powered.
+*
+* As we are disabling runtime PM at this point, we are preventing the
+* subsystem/driver to decide themselves. For that reason, we need to
+* make sure the device is operational as it may be required in some
+* cases.
 */
pm_runtime_resume(dev);
__pm_runtime_disable(dev, false);
@@ -758,8 +758,7 @@ static int pm_genpd_prepare(struct device *dev)
if (ret) {
mutex_lock(&genpd->lock);

-   if (--genpd->prepared_count == 0)
-   genpd->suspend_power_off = false;
+   genpd->prepared_count--;

mutex_unlock(&genpd->lock);
pm_runtime_enable(dev);
@@ -786,7 +785,7 @@ static int pm_genpd_suspend(struct device *dev)
if (IS_ERR(genpd))
return -EINVAL;

-   return genpd->suspend_power_off ? 0 : pm_generic_suspend(dev);
+