GENPD API improvements

---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/base/power/domain.c +5 -6

index 9a14eedacb92..a3e1bfc233d4 100644
@@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 			goto err;
 	}
 
-	ret = genpd->set_performance_state(genpd, state);
-	if (ret)
-		goto err;
+	if (genpd->set_performance_state) {
+		ret = genpd->set_performance_state(genpd, state);
+		if (ret)
+			goto err;
+	}
 
 	genpd->performance_state = state;
 	return 0;

@@ -399,9 +401,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	if (!genpd)
 		return -ENODEV;
 
-	if (unlikely(!genpd->set_performance_state))
-		return -EINVAL;
-
 	if (WARN_ON(!dev->power.subsys_data ||
 		     !dev->power.subsys_data->domain_data))
 		return -EINVAL;

Signature · 1 line

2.29.2

Viresh Kumar

Reviewed-by on v3 1/3

2021-01-18 07:29 UTC #2 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 21 lines · Dmitry Osipenko

Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.
Tested-by Peter Geis <[email protected]>
Tested-by Nicolas Chauvet <[email protected]>
Tested-by Matt Merhar <[email protected]>
Suggested-by Ulf Hansson <[email protected]>
Reviewed-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/base/power/domain.c +0 -0
index 9a14eedacb92..a3e1bfc233d4 100644
@@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 			goto err;

drivers/base/power/domain.c static int _genpd_set_performance_state(struct generic_pm_domain *genpd, · line 346

Quoted 10 lines

drivers/base/power/domain.c +5 -3

 	}
 
-	ret = genpd->set_performance_state(genpd, state);
-	if (ret)
-		goto err;
+	if (genpd->set_performance_state) {
+		ret = genpd->set_performance_state(genpd, state);
+		if (ret)
+			goto err;
+	}

Earlier in this routine we also have this:

if (!parent->set_performance_state)

        continue;

Should we change that too ?

drivers/base/power/domain.c int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) · line 406

Quoted 13 lines

drivers/base/power/domain.c +0 -3

 	genpd->performance_state = state;
 	return 0;

@@ -399,9 +401,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	if (!genpd)
 		return -ENODEV;
 
-	if (unlikely(!genpd->set_performance_state))
-		return -EINVAL;
-
 	if (WARN_ON(!dev->power.subsys_data ||
 		     !dev->power.subsys_data->domain_data))
 		return -EINVAL;

Signature · 1 line

viresh

Ulf Hansson

2021-01-18 11:02 UTC #3 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 36 lines · Viresh Kumar

Quoted 21 lines · Dmitry Osipenko
Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.
Tested-by Peter Geis <[email protected]>
Tested-by Nicolas Chauvet <[email protected]>
Tested-by Matt Merhar <[email protected]>
Suggested-by Ulf Hansson <[email protected]>
Reviewed-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/base/power/domain.c +0 -0
index 9a14eedacb92..a3e1bfc233d4 100644
@@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
                      goto err;
drivers/base/power/domain.c static int _genpd_set_performance_state(struct generic_pm_domain *genpd, · line 346
Quoted 10 lines
drivers/base/power/domain.c +5 -3
      }
 
-     ret = genpd->set_performance_state(genpd, state);
-     if (ret)
-             goto err;
+     if (genpd->set_performance_state) {
+             ret = genpd->set_performance_state(genpd, state);
+             if (ret)
+                     goto err;
+     }
Earlier in this routine we also have this:

Quoted 4 lines

if (!parent->set_performance_state)
        continue;
Should we change that too ?

Good point! I certainly overlooked that when reviewing. We need to reevaluate the new state when propagating to the parent(s).

To me, it looks like when doing the propagation we must check if the parent has the ->set_performance_state() callback assigned. If so, we should call dev_pm_opp_xlate_performance_state(), but otherwise just use the value of "state", when doing the reevaluation.

Does it make sense?

Quoted 17 lines

Quoted 5 lines

drivers/base/power/domain.c +0 -0

      genpd->performance_state = state;
      return 0;

drivers/base/power/domain.c int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) · line 406

Quoted 9 lines

drivers/base/power/domain.c +0 -3

@@ -399,9 +401,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
      if (!genpd)
              return -ENODEV;
 
-     if (unlikely(!genpd->set_performance_state))
-             return -EINVAL;
-
      if (WARN_ON(!dev->power.subsys_data ||
                   !dev->power.subsys_data->domain_data))
              return -EINVAL;

In reply to Dmitry Osipenko

Quoted 2 lines

--
viresh

Kind regards
Uffe

Viresh Kumar

2021-01-18 11:06 UTC #4 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 9 lines · Ulf Hansson

Good point! I certainly overlooked that when reviewing. We need to reevaluate the new state when propagating to the parent(s).
To me, it looks like when doing the propagation we must check if the parent has the ->set_performance_state() callback assigned. If so, we should call dev_pm_opp_xlate_performance_state(), but otherwise just use the value of "state", when doing the reevaluation.
Does it make sense?

That will work but I am wondering if there is a way to avoid the unnecessary propagation if we can somehow find out if someone above in hierarchy supports pstates or not ?

Signature · 1 line

viresh

Ulf Hansson

2021-01-18 12:48 UTC #5 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 10 lines · Viresh Kumar

Quoted 8 lines · Ulf Hansson
Good point! I certainly overlooked that when reviewing. We need to reevaluate the new state when propagating to the parent(s).
To me, it looks like when doing the propagation we must check if the parent has the ->set_performance_state() callback assigned. If so, we should call dev_pm_opp_xlate_performance_state(), but otherwise just use the value of "state", when doing the reevaluation.

Quoted 5 lines

Quoted 1 line
Does it make sense?
That will work but I am wondering if there is a way to avoid the unnecessary propagation if we can somehow find out if someone above in hierarchy supports pstates or not ?

You seem to be worried about latency/overhead while doing the propagation upwards in the hierarchy. That seems like a reasonable concern to me, especially as the genpd lock is taken at each level.

However, to mitigate this can be rather messy. In principle, we would need to walk the hierarchy upwards, each time a new subdomain is added in genpd_add_subdomain(). While doing this, we would also need to keep track on what level we set to continue the propagation of the performance states for. Even if this can be done in non-latency sensitive paths, I don't think it's worth it because of complexity (I haven't even thought of what happens when removing a subdomain).

So, maybe we should simply just stick to the existing code, forcing the parent to have a ->set_performance() callback assigned if propagation should continue?

Kind regards
Uffe

Viresh Kumar

2021-01-19 03:45 UTC #6 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 3 lines · Ulf Hansson

You seem to be worried about latency/overhead while doing the propagation upwards in the hierarchy. That seems like a reasonable concern to me, especially as the genpd lock is taken at each level.

I am not sure how many levels of domains we have normally, but unless the number is big it won't be a very big problem.

Quoted 7 lines

However, to mitigate this can be rather messy. In principle, we would need to walk the hierarchy upwards, each time a new subdomain is added in genpd_add_subdomain(). While doing this, we would also need to keep track on what level we set to continue the propagation of the performance states for. Even if this can be done in non-latency sensitive paths, I don't think it's worth it because of complexity (I haven't even thought of what happens when removing a subdomain).

What about a new field in the domain structure like 'can-handle-pstates', and then whenever sub-domain gets added it just needs to check its parent's field and set his own.

Quoted 3 lines

So, maybe we should simply just stick to the existing code, forcing the parent to have a ->set_performance() callback assigned if propagation should continue?

I think it would be better to fix the issue even if we aren't fully optimized and making the change to make sure we keep propagating is rather important.

Signature · 1 line

viresh

Ulf Hansson

2021-01-19 10:26 UTC #7 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 8 lines · Viresh Kumar

Quoted 3 lines · Ulf Hansson
You seem to be worried about latency/overhead while doing the propagation upwards in the hierarchy. That seems like a reasonable concern to me, especially as the genpd lock is taken at each level.
I am not sure how many levels of domains we have normally, but unless the number is big it won't be a very big problem.

It depends on the SoC topology, so obviously it differs. But more than a couple is unusual, I would say.

However, I think it may also depend on how many devices that are hooked up to the domains and how actively these are being used.

Quoted 9 lines

Quoted 8 lines
However, to mitigate this can be rather messy. In principle, we would need to walk the hierarchy upwards, each time a new subdomain is added in genpd_add_subdomain(). While doing this, we would also need to keep track on what level we set to continue the propagation of the performance states for. Even if this can be done in non-latency sensitive paths, I don't think it's worth it because of complexity (I haven't even thought of what happens when removing a subdomain).

Quoted 3 lines

What about a new field in the domain structure like 'can-handle-pstates', and then whenever sub-domain gets added it just needs to check its parent's field and set his own.

That would work if the topology is built from top to bottom, but I don't think we can rely on that.

For example, when a domain A is added as a child to domain B, domain B doesn't have a parent yet (and the "can-handle-pstates" don't get set for neither domain A or domain B). Next, domain B is added as child domain to domain C. Domain C has the "can-handle-pstates" set, which means domain B gets the "can-handle-pstates" set as well. This means domain A, will not have "can-handle-pstates" set, while it probably should have.

Quoted 7 lines

Quoted 4 lines
So, maybe we should simply just stick to the existing code, forcing the parent to have a ->set_performance() callback assigned if propagation should continue?
I think it would be better to fix the issue even if we aren't fully optimized and making the change to make sure we keep propagating is rather important.

Alright, let's continue with Dmitry's patches and discuss this further when v4 is out, as he seems to have it almost ready.

Kind regards
Uffe

Viresh Kumar

2021-01-19 10:26 UTC #8 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 10 lines · Ulf Hansson

That would work if the topology is built from top to bottom, but I don't think we can rely on that.
For example, when a domain A is added as a child to domain B, domain B doesn't have a parent yet (and the "can-handle-pstates" don't get set for neither domain A or domain B). Next, domain B is added as child domain to domain C. Domain C has the "can-handle-pstates" set, which means domain B gets the "can-handle-pstates" set as well. This means domain A, will not have "can-handle-pstates" set, while it probably should have.

Okay, I missed that part.

Quoted 10 lines

Quoted 7 lines
Quoted 4 lines
So, maybe we should simply just stick to the existing code, forcing the parent to have a ->set_performance() callback assigned if propagation should continue?
I think it would be better to fix the issue even if we aren't fully optimized and making the change to make sure we keep propagating is rather important.
Alright, let's continue with Dmitry's patches and discuss this further when v4 is out, as he seems to have it almost ready.

Right.

Signature · 1 line

viresh

Dmitry Osipenko

author

2021-01-18 18:45 UTC #9 lore raw

Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

In reply to Ulf Hansson

18.01.2021 13:59, Ulf Hansson пишет:

Quoted 45 lines · Viresh Kumar

Quoted 36 lines · Dmitry Osipenko
Quoted 21 lines
Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.
Tested-by Peter Geis <[email protected]>
Tested-by Nicolas Chauvet <[email protected]>
Tested-by Matt Merhar <[email protected]>
Suggested-by Ulf Hansson <[email protected]>
Reviewed-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/base/power/domain.c +0 -0
index 9a14eedacb92..a3e1bfc233d4 100644
@@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
                      goto err;
drivers/base/power/domain.c static int _genpd_set_performance_state(struct generic_pm_domain *genpd, · line 346
Quoted 10 lines
drivers/base/power/domain.c +5 -3
      }
 
-     ret = genpd->set_performance_state(genpd, state);
-     if (ret)
-             goto err;
+     if (genpd->set_performance_state) {
+             ret = genpd->set_performance_state(genpd, state);
+             if (ret)
+                     goto err;
+     }
Earlier in this routine we also have this:
Quoted 4 lines
if (!parent->set_performance_state)
        continue;
Should we change that too ?
Good point! I certainly overlooked that when reviewing. We need to reevaluate the new state when propagating to the parent(s).

Quoted 6 lines

To me, it looks like when doing the propagation we must check if the parent has the ->set_performance_state() callback assigned. If so, we should call dev_pm_opp_xlate_performance_state(), but otherwise just use the value of "state", when doing the reevaluation.
Does it make sense?

I played a tad with the power domains by creating a couple dummy domains and putting them into a parent->child chain. Yours suggestion works well for the case where intermediate domain doesn't implement the set_performance_state() callback, i.e. the performance state is propagated through the whole chain instead of stopping on the domain that doesn't implement the callback.

I'll make a v4, thanks.

Dmitry Osipenko

author

2021-01-18 01:16 UTC #10 lore raw

PATCH v3 2/3 PM: domains: Add "performance" column to debug summary

Add "performance" column to debug summary which shows performance state of all power domains and theirs devices.

Tested-by Peter Geis <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Matt Merhar <[email protected]>

---
 drivers/base/power/domain.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

drivers/base/power/domain.c +15 -4

index a3e1bfc233d4..2ab77aa516eb 100644
@@ -2951,7 +2951,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
 	else
 		WARN_ON(1);
 
-	seq_puts(s, p);
+	seq_printf(s, "%-25s  ", p);
+}
+
+static void perf_status_str(struct seq_file *s, struct device *dev)
+{
+	struct generic_pm_domain_data *gpd_data;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	seq_put_decimal_ull(s, "", gpd_data->performance_state);
 }
 
 static int genpd_summary_one(struct seq_file *s,

@@ -2979,7 +2987,7 @@ static int genpd_summary_one(struct seq_file *s,
 	else
 		snprintf(state, sizeof(state), "%s",
 			 status_lookup[genpd->status]);
-	seq_printf(s, "%-30s  %-15s ", genpd->name, state);
+	seq_printf(s, "%-30s  %-50s %u", genpd->name, state, genpd->performance_state);
 
 	/*
 	 * Modifications on the list require holding locks on both

@@ -2987,6 +2995,8 @@ static int genpd_summary_one(struct seq_file *s,
 	 * Also genpd->name is immutable.
 	 */
 	list_for_each_entry(link, &genpd->parent_links, parent_node) {
+		if (list_is_first(&link->parent_node, &genpd->parent_links))
+			seq_printf(s, "\n%48s", " ");
 		seq_printf(s, "%s", link->child->name);
 		if (!list_is_last(&link->parent_node, &genpd->parent_links))
 			seq_puts(s, ", ");

@@ -3001,6 +3011,7 @@ static int genpd_summary_one(struct seq_file *s,
 
 		seq_printf(s, "\n    %-50s  ", kobj_path);
 		rtpm_status_str(s, pm_data->dev);
+		perf_status_str(s, pm_data->dev);
 		kfree(kobj_path);
 	}

@@ -3016,9 +3027,9 @@ static int summary_show(struct seq_file *s, void *data)
 	struct generic_pm_domain *genpd;
 	int ret = 0;
 
-	seq_puts(s, "domain                          status          children\n");
+	seq_puts(s, "domain                          status          children                           performance\n");
 	seq_puts(s, "    /device                                             runtime status\n");
-	seq_puts(s, "----------------------------------------------------------------------\n");
+	seq_puts(s, "----------------------------------------------------------------------------------------------\n");
 
 	ret = mutex_lock_interruptible(&gpd_list_lock);
 	if (ret)

Signature · 1 line

2.29.2

Viresh Kumar

Reviewed-by on v3 2/3

2021-01-18 07:32 UTC #11 lore raw

Re: [PATCH v3 2/3] PM: domains: Add "performance" column to debug summary

Quoted 61 lines · Dmitry Osipenko

Add "performance" column to debug summary which shows performance state of all power domains and theirs devices.

Tested-by Peter Geis <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Matt Merhar <[email protected]>

---
 drivers/base/power/domain.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

drivers/base/power/domain.c +13 -2

index a3e1bfc233d4..2ab77aa516eb 100644
@@ -2951,7 +2951,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
 	else
 		WARN_ON(1);
 
-	seq_puts(s, p);
+	seq_printf(s, "%-25s  ", p);
+}
+
+static void perf_status_str(struct seq_file *s, struct device *dev)
+{
+	struct generic_pm_domain_data *gpd_data;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	seq_put_decimal_ull(s, "", gpd_data->performance_state);
 }
 
 static int genpd_summary_one(struct seq_file *s,

@@ -2979,7 +2987,7 @@ static int genpd_summary_one(struct seq_file *s,
 	else
 		snprintf(state, sizeof(state), "%s",
 			 status_lookup[genpd->status]);
-	seq_printf(s, "%-30s  %-15s ", genpd->name, state);
+	seq_printf(s, "%-30s  %-50s %u", genpd->name, state, genpd->performance_state);
 
 	/*
 	 * Modifications on the list require holding locks on both

@@ -2987,6 +2995,8 @@ static int genpd_summary_one(struct seq_file *s,
 	 * Also genpd->name is immutable.
 	 */
 	list_for_each_entry(link, &genpd->parent_links, parent_node) {
+		if (list_is_first(&link->parent_node, &genpd->parent_links))
+			seq_printf(s, "\n%48s", " ");
 		seq_printf(s, "%s", link->child->name);
 		if (!list_is_last(&link->parent_node, &genpd->parent_links))
 			seq_puts(s, ", ");

@@ -3001,6 +3011,7 @@ static int genpd_summary_one(struct seq_file *s,
 
 		seq_printf(s, "\n    %-50s  ", kobj_path);
 		rtpm_status_str(s, pm_data->dev);
+		perf_status_str(s, pm_data->dev);
 		kfree(kobj_path);
 	}

@@ -3016,9 +3027,9 @@ static int summary_show(struct seq_file *s, void *data)
 	struct generic_pm_domain *genpd;

drivers/base/power/domain.c static int summary_show(struct seq_file *s, void *data) · line 3035

Quoted 10 lines

drivers/base/power/domain.c +2 -2

 	int ret = 0;
 
-	seq_puts(s, "domain                          status          children\n");
+	seq_puts(s, "domain                          status          children                           performance\n");
 	seq_puts(s, "    /device                                             runtime status\n");
-	seq_puts(s, "----------------------------------------------------------------------\n");
+	seq_puts(s, "----------------------------------------------------------------------------------------------\n");
 
 	ret = mutex_lock_interruptible(&gpd_list_lock);
 	if (ret)

In reply to Dmitry Osipenko

Signature · 1 line

viresh

Dmitry Osipenko

author

2021-01-18 01:17 UTC #12 lore raw

PATCH v3 3/3 PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER

Driver of a power domain provider may not be ready at the time of of_genpd_add_subdomain() invocation. Make this function to return -EPROBE_DEFER instead of -ENOENT in order to remove a need from power domain drivers to handle the error code specially.

---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/domain.c +1 -1

index 2ab77aa516eb..7ef6764ee30f 100644
@@ -2462,7 +2462,7 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 out:
 	mutex_unlock(&gpd_list_lock);
 
-	return ret;
+	return ret == -ENOENT ? -EPROBE_DEFER : ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);

Signature · 1 line

2.29.2

Viresh Kumar

Reviewed-by on v3 3/3

2021-01-18 07:33 UTC #13 lore raw

Re: [PATCH v3 3/3] PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER

Quoted 16 lines · Dmitry Osipenko

Driver of a power domain provider may not be ready at the time of of_genpd_add_subdomain() invocation. Make this function to return -EPROBE_DEFER instead of -ENOENT in order to remove a need from power domain drivers to handle the error code specially.
Suggested-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/base/power/domain.c +0 -0
index 2ab77aa516eb..7ef6764ee30f 100644

drivers/base/power/domain.c int of_genpd_add_subdomain(struct of_phandle_args *parent_spec, · line 2468

Quoted 8 lines

drivers/base/power/domain.c +1 -1

@@ -2462,7 +2462,7 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 out:
 	mutex_unlock(&gpd_list_lock);
 
-	return ret;
+	return ret == -ENOENT ? -EPROBE_DEFER : ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);

In reply to Dmitry Osipenko

Signature · 1 line

viresh

Ulf Hansson

Reviewed-by on v3 3/3

2021-01-18 10:48 UTC #14 lore raw

Re: [PATCH v3 3/3] PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER

Quoted 8 lines · Dmitry Osipenko

Driver of a power domain provider may not be ready at the time of of_genpd_add_subdomain() invocation. Make this function to return -EPROBE_DEFER instead of -ENOENT in order to remove a need from power domain drivers to handle the error code specially.
Suggested-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>