This is a rework of [1] using genpd instead of 'reset' framework.
As per the recommended reset sequence of Adreno gpu, we should ensure that
gpucc-cx-gdsc has collapsed at hardware to reset gpu's internal hardware states.
Because this gdsc is implemented as 'votable', gdsc driver doesn't poll and
wait until its hw status says OFF.
So use the newly introduced genpd api (dev_pm_genpd_synced_poweroff()) to
provide a hint to the gdsc driver to poll for the hw status and use genpd
notifier to wait from adreno gpu driver until gdsc is turned OFF.
This series is rebased on top of linux-next (20221215) since the changes span
multiple drivers.
[1] https://patchwork.freedesktop.org/series/107507/
Changes in v3:
- Rename the var 'force_sync' to 'wait (Stephen)
Changes in v2:
- Minor formatting fix
- Select PM_GENERIC_DOMAINS from Kconfig
Akhil P Oommen (4):
clk: qcom: gdsc: Support 'synced_poweroff' genpd flag
drm/msm/a6xx: Vote for cx gdsc from gpu driver
drm/msm/a6xx: Remove cx gdsc polling using 'reset'
drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse
Ulf Hansson (1):
PM: domains: Allow a genpd consumer to require a synced power off
drivers/base/power/domain.c | 23 ++++++++++++++++++
drivers/clk/qcom/gdsc.c | 11 +++++----
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 ++++++++++++++++++++++++++++++++---
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 7 ++++++
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 13 +++++++---
drivers/gpu/drm/msm/msm_gpu.c | 4 ---
drivers/gpu/drm/msm/msm_gpu.h | 4 ---
include/linux/pm_domain.h | 5 ++++
9 files changed, 94 insertions(+), 20 deletions(-)
--
2.7.4
When a device has multiple power domains, dev->power_domain is left
empty during probe. That didn't cause any issue so far because we are
freeloading on smmu driver's vote on cx gdsc. Instead of that, create
a device_link between cx genpd device and gmu device to keep a vote from
gpu driver.
Before this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmu active 0
cx_gdsc on 0
/devices/platform/soc@0/3da0000.iommu active 0
After this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmu active 0
cx_gdsc on 0
/devices/platform/soc@0/3da0000.iommu active 0
/devices/genpd:0:3d6a000.gmu active 0
Signed-off-by: Akhil P Oommen <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 31 +++++++++++++++++++++++++++----
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 1 +
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 6484b97c5344..1580d0090f35 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1479,6 +1479,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
pm_runtime_force_suspend(gmu->dev);
+ /*
+ * Since cxpd is a virt device, the devlink with gmu-dev will be removed
+ * automatically when we do detach
+ */
+ dev_pm_domain_detach(gmu->cxpd, false);
+
if (!IS_ERR_OR_NULL(gmu->gxpd)) {
pm_runtime_disable(gmu->gxpd);
dev_pm_domain_detach(gmu->gxpd, false);
@@ -1605,8 +1611,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
if (adreno_is_a650_family(adreno_gpu)) {
gmu->rscc = a6xx_gmu_get_mmio(pdev, "rscc");
- if (IS_ERR(gmu->rscc))
+ if (IS_ERR(gmu->rscc)) {
+ ret = -ENODEV;
goto err_mmio;
+ }
} else {
gmu->rscc = gmu->mmio + 0x23000;
}
@@ -1615,8 +1623,22 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
gmu->hfi_irq = a6xx_gmu_get_irq(gmu, pdev, "hfi", a6xx_hfi_irq);
gmu->gmu_irq = a6xx_gmu_get_irq(gmu, pdev, "gmu", a6xx_gmu_irq);
- if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
+ if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0) {
+ ret = -ENODEV;
+ goto err_mmio;
+ }
+
+ gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
+ if (IS_ERR(gmu->cxpd)) {
+ ret = PTR_ERR(gmu->cxpd);
goto err_mmio;
+ }
+
+ if (!device_link_add(gmu->dev, gmu->cxpd,
+ DL_FLAG_PM_RUNTIME)) {
+ ret = -ENODEV;
+ goto detach_cxpd;
+ }
/*
* Get a link to the GX power domain to reset the GPU in case of GMU
@@ -1634,6 +1656,9 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
return 0;
+detach_cxpd:
+ dev_pm_domain_detach(gmu->cxpd, false);
+
err_mmio:
iounmap(gmu->mmio);
if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
@@ -1641,8 +1666,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
free_irq(gmu->gmu_irq, gmu);
free_irq(gmu->hfi_irq, gmu);
- ret = -ENODEV;
-
err_memory:
a6xx_gmu_memory_free(gmu);
err_put_device:
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index e034935b3986..5a42dd4dd31f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -56,6 +56,7 @@ struct a6xx_gmu {
int gmu_irq;
struct device *gxpd;
+ struct device *cxpd;
int idle_level;
--
2.7.4
From: Ulf Hansson <[email protected]>
Some genpd providers doesn't ensure that it has turned off at hardware.
This is fine until the consumer really requires during some special
scenarios that the power domain collapse at hardware before it is
turned ON again.
An example is the reset sequence of Adreno GPU which requires that the
'gpucc cx gdsc' power domain should move to OFF state in hardware at
least once before turning in ON again to clear the internal state.
Signed-off-by: Ulf Hansson <[email protected]>
Signed-off-by: Akhil P Oommen <[email protected]>
---
(no changes since v2)
Changes in v2:
- Minor formatting fix
drivers/base/power/domain.c | 23 +++++++++++++++++++++++
include/linux/pm_domain.h | 5 +++++
2 files changed, 28 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 967bcf9d415e..53524a102321 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -519,6 +519,28 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
+/*
+ * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
+ *
+ * @dev: A device that is attached to the genpd.
+ *
+ * Allows a consumer of the genpd to notify the provider that the next power off
+ * should be synchronous.
+ */
+void dev_pm_genpd_synced_poweroff(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+
+ genpd = dev_to_genpd_safe(dev);
+ if (!genpd)
+ return;
+
+ genpd_lock(genpd);
+ genpd->synced_poweroff = true;
+ genpd_unlock(genpd);
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -562,6 +584,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
out:
raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
+ genpd->synced_poweroff = false;
return 0;
err:
raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 1cd41bdf73cf..f776fb93eaa0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -136,6 +136,7 @@ struct generic_pm_domain {
unsigned int prepared_count; /* Suspend counter of prepared devices */
unsigned int performance_state; /* Aggregated max performance state */
cpumask_var_t cpus; /* A cpumask of the attached CPUs */
+ bool synced_poweroff; /* A consumer needs a synced poweroff */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
@@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
int dev_pm_genpd_remove_notifier(struct device *dev);
void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
+void dev_pm_genpd_synced_poweroff(struct device *dev);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -300,6 +302,9 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
{
return KTIME_MAX;
}
+static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
+{ }
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
--
2.7.4
On Tue, 20 Dec 2022 at 08:44, Akhil P Oommen <[email protected]> wrote:
>
> From: Ulf Hansson <[email protected]>
>
> Some genpd providers doesn't ensure that it has turned off at hardware.
> This is fine until the consumer really requires during some special
> scenarios that the power domain collapse at hardware before it is
> turned ON again.
>
> An example is the reset sequence of Adreno GPU which requires that the
> 'gpucc cx gdsc' power domain should move to OFF state in hardware at
> least once before turning in ON again to clear the internal state.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> Signed-off-by: Akhil P Oommen <[email protected]>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Minor formatting fix
>
> drivers/base/power/domain.c | 23 +++++++++++++++++++++++
> include/linux/pm_domain.h | 5 +++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 967bcf9d415e..53524a102321 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -519,6 +519,28 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
>
> +/*
> + * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
> + *
> + * @dev: A device that is attached to the genpd.
> + *
> + * Allows a consumer of the genpd to notify the provider that the next power off
> + * should be synchronous.
Nitpick; similar to other dev_pm_genpd_* function-descriptions, I
think it's important to add the below information.
"It is assumed that the users guarantee that the genpd wouldn't be
detached while this routine is getting called."
Can you please add that?
> + */
> +void dev_pm_genpd_synced_poweroff(struct device *dev)
> +{
> + struct generic_pm_domain *genpd;
> +
> + genpd = dev_to_genpd_safe(dev);
> + if (!genpd)
> + return;
> +
> + genpd_lock(genpd);
> + genpd->synced_poweroff = true;
> + genpd_unlock(genpd);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
> +
> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> {
> unsigned int state_idx = genpd->state_idx;
> @@ -562,6 +584,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>
> out:
> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
> + genpd->synced_poweroff = false;
> return 0;
> err:
> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 1cd41bdf73cf..f776fb93eaa0 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -136,6 +136,7 @@ struct generic_pm_domain {
> unsigned int prepared_count; /* Suspend counter of prepared devices */
> unsigned int performance_state; /* Aggregated max performance state */
> cpumask_var_t cpus; /* A cpumask of the attached CPUs */
> + bool synced_poweroff; /* A consumer needs a synced poweroff */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
> @@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
> int dev_pm_genpd_remove_notifier(struct device *dev);
> void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
> ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
> +void dev_pm_genpd_synced_poweroff(struct device *dev);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -300,6 +302,9 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
> {
> return KTIME_MAX;
> }
> +static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
> +{ }
> +
> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
> #endif
> --
> 2.7.4
>
Kind regards
Uffe
On 12/21/2022 8:13 PM, Ulf Hansson wrote:
> On Tue, 20 Dec 2022 at 08:44, Akhil P Oommen <[email protected]> wrote:
>> From: Ulf Hansson <[email protected]>
>>
>> Some genpd providers doesn't ensure that it has turned off at hardware.
>> This is fine until the consumer really requires during some special
>> scenarios that the power domain collapse at hardware before it is
>> turned ON again.
>>
>> An example is the reset sequence of Adreno GPU which requires that the
>> 'gpucc cx gdsc' power domain should move to OFF state in hardware at
>> least once before turning in ON again to clear the internal state.
>>
>> Signed-off-by: Ulf Hansson <[email protected]>
>> Signed-off-by: Akhil P Oommen <[email protected]>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - Minor formatting fix
>>
>> drivers/base/power/domain.c | 23 +++++++++++++++++++++++
>> include/linux/pm_domain.h | 5 +++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 967bcf9d415e..53524a102321 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -519,6 +519,28 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
>> }
>> EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
>>
>> +/*
>> + * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
>> + *
>> + * @dev: A device that is attached to the genpd.
>> + *
>> + * Allows a consumer of the genpd to notify the provider that the next power off
>> + * should be synchronous.
> Nitpick; similar to other dev_pm_genpd_* function-descriptions, I
> think it's important to add the below information.
>
> "It is assumed that the users guarantee that the genpd wouldn't be
> detached while this routine is getting called."
>
> Can you please add that?
Thanks. Fixed in revision 4.
-Akhil.
>
>> + */
>> +void dev_pm_genpd_synced_poweroff(struct device *dev)
>> +{
>> + struct generic_pm_domain *genpd;
>> +
>> + genpd = dev_to_genpd_safe(dev);
>> + if (!genpd)
>> + return;
>> +
>> + genpd_lock(genpd);
>> + genpd->synced_poweroff = true;
>> + genpd_unlock(genpd);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
>> +
>> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>> {
>> unsigned int state_idx = genpd->state_idx;
>> @@ -562,6 +584,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>
>> out:
>> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
>> + genpd->synced_poweroff = false;
>> return 0;
>> err:
>> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 1cd41bdf73cf..f776fb93eaa0 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -136,6 +136,7 @@ struct generic_pm_domain {
>> unsigned int prepared_count; /* Suspend counter of prepared devices */
>> unsigned int performance_state; /* Aggregated max performance state */
>> cpumask_var_t cpus; /* A cpumask of the attached CPUs */
>> + bool synced_poweroff; /* A consumer needs a synced poweroff */
>> int (*power_off)(struct generic_pm_domain *domain);
>> int (*power_on)(struct generic_pm_domain *domain);
>> struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
>> @@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
>> int dev_pm_genpd_remove_notifier(struct device *dev);
>> void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
>> ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
>> +void dev_pm_genpd_synced_poweroff(struct device *dev);
>>
>> extern struct dev_power_governor simple_qos_governor;
>> extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -300,6 +302,9 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
>> {
>> return KTIME_MAX;
>> }
>> +static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
>> +{ }
>> +
>> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
>> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
>> #endif
>> --
>> 2.7.4
>>
> Kind regards
> Uffe