[RESEND PATCH v3 0/2] PM / Domains: Second step in improving system PM code in genpd

[RESEND PATCH v3 0/2] PM / Domains: Second step in improving system PM code in genpd

[Ulf Hansson]

Changes in v3:
	- Changelog of patch 1/2.

Changes in v2:
	- Updated changelogs for both patches according to comments from Kevin.
	- Updated patch 1/2, as I realized one genpd client driver, (ab)uses
	genpd's suspend_power_off flag.

This is the second step in improving the system PM code in genpd. The first
patch is actually fixing a bug while it also enables for further clean-ups,
which is done in the second patch.

The next step of improving system PM code in genpd will move it to the part of
trying to optimize the behaviour, such as avoid waking up devices unless it's
really needed.

Ulf Hansson (2):
  PM / Domains: Allow genpd to power on during the system PM phases
  PM / Domains: Remove redundant wrapper functions for system PM

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

-- 
1.9.1

[RESEND PATCH v3 1/2] PM / Domains: Allow genpd to power on during the system PM phases

[Ulf Hansson]

If the PM domain is powered off when the first device starts its system PM
prepare phase, genpd prevents any further attempts to power on the PM
domain during the following system PM phases. Not until the system PM
complete phase is finalized for all devices in the PM domain, genpd again
allows it to be powered on.

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 some of
the system PM phases. 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 phases.

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 <airlied@linux.ie>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- Updated changelog.

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);
+	return pm_generic_suspend(dev);
 }
 
 /**
@@ -807,7 +806,7 @@ static int pm_genpd_suspend_late(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_suspend_late(dev);
+	return pm_generic_suspend_late(dev);
 }
 
 /**
@@ -827,8 +826,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (genpd->suspend_power_off
-	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
+	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
 	genpd_stop_dev(genpd, dev);
@@ -860,8 +858,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (genpd->suspend_power_off
-	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
+	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
 	/*
@@ -894,7 +891,7 @@ static int pm_genpd_resume_early(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_resume_early(dev);
+	return pm_generic_resume_early(dev);
 }
 
 /**
@@ -915,7 +912,7 @@ static int pm_genpd_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_resume(dev);
+	return pm_generic_resume(dev);
 }
 
 /**
@@ -936,7 +933,7 @@ static int pm_genpd_freeze(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_freeze(dev);
+	return pm_generic_freeze(dev);
 }
 
 /**
@@ -958,7 +955,7 @@ static int pm_genpd_freeze_late(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_freeze_late(dev);
+	return pm_generic_freeze_late(dev);
 }
 
 /**
@@ -980,7 +977,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : genpd_stop_dev(genpd, dev);
+	return genpd_stop_dev(genpd, dev);
 }
 
 /**
@@ -1000,8 +997,7 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ?
-		0 : genpd_start_dev(genpd, dev);
+	return genpd_start_dev(genpd, dev);
 }
 
 /**
@@ -1023,7 +1019,7 @@ static int pm_genpd_thaw_early(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_thaw_early(dev);
+	return pm_generic_thaw_early(dev);
 }
 
 /**
@@ -1044,7 +1040,7 @@ static int pm_genpd_thaw(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : pm_generic_thaw(dev);
+	return pm_generic_thaw(dev);
 }
 
 /**
@@ -1072,26 +1068,13 @@ static int pm_genpd_restore_noirq(struct device *dev)
 	 * At this point suspended_count == 0 means we are being run for the
 	 * first time for the given domain in the present cycle.
 	 */
-	if (genpd->suspended_count++ == 0) {
+	if (genpd->suspended_count++ == 0)
 		/*
 		 * The boot kernel might put the domain into arbitrary state,
 		 * so make it appear as powered off to pm_genpd_sync_poweron(),
 		 * so that it tries to power it on in case it was really off.
 		 */
 		genpd->status = GPD_STATE_POWER_OFF;
-		if (genpd->suspend_power_off) {
-			/*
-			 * If the domain was off before the hibernation, make
-			 * sure it will be off going forward.
-			 */
-			genpd_power_off(genpd, true);
-
-			return 0;
-		}
-	}
-
-	if (genpd->suspend_power_off)
-		return 0;
 
 	pm_genpd_sync_poweron(genpd, true);
 
@@ -1110,7 +1093,6 @@ static int pm_genpd_restore_noirq(struct device *dev)
 static void pm_genpd_complete(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
-	bool run_complete;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -1120,18 +1102,14 @@ static void pm_genpd_complete(struct device *dev)
 
 	mutex_lock(&genpd->lock);
 
-	run_complete = !genpd->suspend_power_off;
-	if (--genpd->prepared_count == 0)
-		genpd->suspend_power_off = false;
+	genpd->prepared_count--;
 
 	mutex_unlock(&genpd->lock);
 
-	if (run_complete) {
-		pm_generic_complete(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-		pm_request_idle(dev);
-	}
+	pm_generic_complete(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_request_idle(dev);
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index b7b583c..a905f1f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -421,29 +421,6 @@ static int acp_suspend(void *handle)
 
 static int acp_resume(void *handle)
 {
-	int i, ret;
-	struct acp_pm_domain *apd;
-	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
-
-	/* return early if no ACP */
-	if (!adev->acp.acp_genpd)
-		return 0;
-
-	/* SMU block will power on ACP irrespective of ACP runtime status.
-	 * Power off explicitly based on genpd ACP runtime status so that ACP
-	 * hw and ACP-genpd status are in sync.
-	 * 'suspend_power_off' represents "Power status before system suspend"
-	*/
-	if (adev->acp.acp_genpd->gpd.suspend_power_off == true) {
-		apd = container_of(&adev->acp.acp_genpd->gpd,
-					struct acp_pm_domain, gpd);
-
-		for (i = 4; i >= 0 ; i--) {
-			ret = acp_suspend_tile(apd->cgs_dev, ACP_TILE_P1 + i);
-			if (ret)
-				pr_err("ACP tile %d tile suspend failed\n", i);
-		}
-	}
 	return 0;
 }
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 39285c7..dd5b044 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -57,7 +57,6 @@ struct generic_pm_domain {
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
 	unsigned int prepared_count;	/* Suspend counter of prepared devices */
-	bool suspend_power_off;	/* Power status before system suspend */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
 	struct gpd_dev_ops dev_ops;
-- 
1.9.1

[RESEND PATCH v3 2/2] PM / Domains: Remove redundant wrapper functions for system PM

[Ulf Hansson]

Due to the previous changes to genpd, which removed the suspend_power_off
flag, several of the system PM callbacks is no longer doing any additional
checks but only invoking a corresponding pm_generic_* helper function.

To clean up the code let's remove these wrapper functions as they have
become redundant. Instead assign the system PM callbacks directly to
the pm_generic_*() helper function.

While changing this, it became clear that some of the current system PM
callbacks in genpd, invokes the wrong callback at the driver level. For
example, genpd's ->restore() callback invokes pm_generic_resume(), while
it should be pm_generic_restore(). Let's fix this as well.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- None.

Changes in v2:
        - Updated changelog.

---
 drivers/base/power/domain.c | 203 +++-----------------------------------------
 1 file changed, 12 insertions(+), 191 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a608f52..658eb1b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -768,48 +768,6 @@ static int pm_genpd_prepare(struct device *dev)
 }
 
 /**
- * pm_genpd_suspend - Suspend a device belonging to an I/O PM domain.
- * @dev: Device to suspend.
- *
- * Suspend a device under the assumption that its pm_domain field points to the
- * domain member of an object of type struct generic_pm_domain representing
- * a PM domain consisting of I/O devices.
- */
-static int pm_genpd_suspend(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_suspend(dev);
-}
-
-/**
- * pm_genpd_suspend_late - Late suspend of a device from an I/O PM domain.
- * @dev: Device to suspend.
- *
- * Carry out a late suspend of a device under the assumption that its
- * pm_domain field points to the domain member of an object of type
- * struct generic_pm_domain representing a PM domain consisting of I/O devices.
- */
-static int pm_genpd_suspend_late(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_suspend_late(dev);
-}
-
-/**
  * pm_genpd_suspend_noirq - Completion of suspend of device in an I/O PM domain.
  * @dev: Device to suspend.
  *
@@ -873,92 +831,6 @@ static int pm_genpd_resume_noirq(struct device *dev)
 }
 
 /**
- * pm_genpd_resume_early - Early resume of a device in an I/O PM domain.
- * @dev: Device to resume.
- *
- * Carry out an early resume of a device under the assumption that its
- * pm_domain field points to the domain member of an object of type
- * struct generic_pm_domain representing a power domain consisting of I/O
- * devices.
- */
-static int pm_genpd_resume_early(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_resume_early(dev);
-}
-
-/**
- * pm_genpd_resume - Resume of device in an I/O PM domain.
- * @dev: Device to resume.
- *
- * Resume a device under the assumption that its pm_domain field points to the
- * domain member of an object of type struct generic_pm_domain representing
- * a power domain consisting of I/O devices.
- */
-static int pm_genpd_resume(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_resume(dev);
-}
-
-/**
- * pm_genpd_freeze - Freezing a device in an I/O PM domain.
- * @dev: Device to freeze.
- *
- * Freeze a device under the assumption that its pm_domain field points to the
- * domain member of an object of type struct generic_pm_domain representing
- * a power domain consisting of I/O devices.
- */
-static int pm_genpd_freeze(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_freeze(dev);
-}
-
-/**
- * pm_genpd_freeze_late - Late freeze of a device in an I/O PM domain.
- * @dev: Device to freeze.
- *
- * Carry out a late freeze of a device under the assumption that its
- * pm_domain field points to the domain member of an object of type
- * struct generic_pm_domain representing a power domain consisting of I/O
- * devices.
- */
-static int pm_genpd_freeze_late(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_freeze_late(dev);
-}
-
-/**
  * pm_genpd_freeze_noirq - Completion of freezing a device in an I/O PM domain.
  * @dev: Device to freeze.
  *
@@ -1001,49 +873,6 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 }
 
 /**
- * pm_genpd_thaw_early - Early thaw of device in an I/O PM domain.
- * @dev: Device to thaw.
- *
- * Carry out an early thaw of a device under the assumption that its
- * pm_domain field points to the domain member of an object of type
- * struct generic_pm_domain representing a power domain consisting of I/O
- * devices.
- */
-static int pm_genpd_thaw_early(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_thaw_early(dev);
-}
-
-/**
- * pm_genpd_thaw - Thaw a device belonging to an I/O power domain.
- * @dev: Device to thaw.
- *
- * Thaw a device under the assumption that its pm_domain field points to the
- * domain member of an object of type struct generic_pm_domain representing
- * a power domain consisting of I/O devices.
- */
-static int pm_genpd_thaw(struct device *dev)
-{
-	struct generic_pm_domain *genpd;
-
-	dev_dbg(dev, "%s()\n", __func__);
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	return pm_generic_thaw(dev);
-}
-
-/**
  * pm_genpd_restore_noirq - Start of restore of device in an I/O PM domain.
  * @dev: Device to resume.
  *
@@ -1151,18 +980,10 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 #else /* !CONFIG_PM_SLEEP */
 
 #define pm_genpd_prepare		NULL
-#define pm_genpd_suspend		NULL
-#define pm_genpd_suspend_late		NULL
 #define pm_genpd_suspend_noirq		NULL
-#define pm_genpd_resume_early		NULL
 #define pm_genpd_resume_noirq		NULL
-#define pm_genpd_resume			NULL
-#define pm_genpd_freeze			NULL
-#define pm_genpd_freeze_late		NULL
 #define pm_genpd_freeze_noirq		NULL
-#define pm_genpd_thaw_early		NULL
 #define pm_genpd_thaw_noirq		NULL
-#define pm_genpd_thaw			NULL
 #define pm_genpd_restore_noirq		NULL
 #define pm_genpd_complete		NULL
 
@@ -1454,24 +1275,24 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = genpd_runtime_resume;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
-	genpd->domain.ops.suspend = pm_genpd_suspend;
-	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
+	genpd->domain.ops.suspend = pm_generic_suspend;
+	genpd->domain.ops.suspend_late = pm_generic_suspend_late;
 	genpd->domain.ops.suspend_noirq = pm_genpd_suspend_noirq;
 	genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq;
-	genpd->domain.ops.resume_early = pm_genpd_resume_early;
-	genpd->domain.ops.resume = pm_genpd_resume;
-	genpd->domain.ops.freeze = pm_genpd_freeze;
-	genpd->domain.ops.freeze_late = pm_genpd_freeze_late;
+	genpd->domain.ops.resume_early = pm_generic_resume_early;
+	genpd->domain.ops.resume = pm_generic_resume;
+	genpd->domain.ops.freeze = pm_generic_freeze;
+	genpd->domain.ops.freeze_late = pm_generic_freeze_late;
 	genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq;
 	genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq;
-	genpd->domain.ops.thaw_early = pm_genpd_thaw_early;
-	genpd->domain.ops.thaw = pm_genpd_thaw;
-	genpd->domain.ops.poweroff = pm_genpd_suspend;
-	genpd->domain.ops.poweroff_late = pm_genpd_suspend_late;
+	genpd->domain.ops.thaw_early = pm_generic_thaw_early;
+	genpd->domain.ops.thaw = pm_generic_thaw;
+	genpd->domain.ops.poweroff = pm_generic_poweroff;
+	genpd->domain.ops.poweroff_late = pm_generic_poweroff_late;
 	genpd->domain.ops.poweroff_noirq = pm_genpd_suspend_noirq;
 	genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq;
-	genpd->domain.ops.restore_early = pm_genpd_resume_early;
-	genpd->domain.ops.restore = pm_genpd_resume;
+	genpd->domain.ops.restore_early = pm_generic_restore_early;
+	genpd->domain.ops.restore = pm_generic_restore;
 	genpd->domain.ops.complete = pm_genpd_complete;
 
 	if (genpd->flags & GENPD_FLAG_PM_CLK) {
-- 
1.9.1

Re: [RESEND PATCH v3 0/2] PM / Domains: Second step in improving system PM code in genpd

[Ulf Hansson]

On 30 May 2016 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Changes in v3:
>         - Changelog of patch 1/2.
>
> Changes in v2:
>         - Updated changelogs for both patches according to comments from Kevin.
>         - Updated patch 1/2, as I realized one genpd client driver, (ab)uses
>         genpd's suspend_power_off flag.
>
> This is the second step in improving the system PM code in genpd. The first
> patch is actually fixing a bug while it also enables for further clean-ups,
> which is done in the second patch.
>
> The next step of improving system PM code in genpd will move it to the part of
> trying to optimize the behaviour, such as avoid waking up devices unless it's
> really needed.
>
> Ulf Hansson (2):
>   PM / Domains: Allow genpd to power on during the system PM phases
>   PM / Domains: Remove redundant wrapper functions for system PM
>
>  drivers/base/power/domain.c             | 271 +++++---------------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c |  23 ---
>  include/linux/pm_domain.h               |   1 -
>  3 files changed, 35 insertions(+), 260 deletions(-)
>
> --
> 1.9.1
>

Rafael, Kevin - any comments on this?

Kind regards
Uffe

Re: [RESEND PATCH v3 1/2] PM / Domains: Allow genpd to power on during the system PM phases

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> If the PM domain is powered off when the first device starts its system PM
> prepare phase, genpd prevents any further attempts to power on the PM
> domain during the following system PM phases. Not until the system PM
> complete phase is finalized for all devices in the PM domain, genpd again
> allows it to be powered on.
>
> 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 some of
> the system PM phases. 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 phases.
>
> 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 <airlied@linux.ie>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v3:
> 	- Updated changelog.
>
> 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 -


For the PM core part:

Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Acked-by: Kevin Hilman <khilman@baylibre.com>

For the AMD DRM driver, the changes look right too, but I'm not
confident enough about the intent of that to be sure.

Kevin

Re: [RESEND PATCH v3 2/2] PM / Domains: Remove redundant wrapper functions for system PM

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Due to the previous changes to genpd, which removed the suspend_power_off
> flag, several of the system PM callbacks is no longer doing any additional
> checks but only invoking a corresponding pm_generic_* helper function.
>
> To clean up the code let's remove these wrapper functions as they have
> become redundant. Instead assign the system PM callbacks directly to
> the pm_generic_*() helper function.
>
> While changing this, it became clear that some of the current system PM
> callbacks in genpd, invokes the wrong callback at the driver level. For
> example, genpd's ->restore() callback invokes pm_generic_resume(), while
> it should be pm_generic_restore(). Let's fix this as well.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Acked-by: Kevin Hilman <khilman@baylibre.com>

Kevin

Re: [RESEND PATCH v3 1/2] PM / Domains: Allow genpd to power on during the system PM phases

[maruthi srinivas]

On Mon, May 30, 2016 at 3:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> If the PM domain is powered off when the first device starts its system PM
> prepare phase, genpd prevents any further attempts to power on the PM
> domain during the following system PM phases. Not until the system PM
> complete phase is finalized for all devices in the PM domain, genpd again
> allows it to be powered on.
>
> 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 some of
> the system PM phases. 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 phases.
>
> 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 <airlied@linux.ie>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v3:
>         - Updated changelog.
>
> 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(-)
>

For the ACP part of DRM driver :

Acked-by: Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel