diff mbox series

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

Message ID 20210118011330.4145-2-digetx@gmail.com
State New
Headers show
Series GENPD API improvements | expand

Commit Message

Dmitry Osipenko Jan. 18, 2021, 1:13 a.m. UTC
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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Viresh Kumar Jan. 18, 2021, 7:28 a.m. UTC | #1
On 18-01-21, 04:13, Dmitry Osipenko wrote:
> 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 <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/base/power/domain.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 9a14eedacb92..a3e1bfc233d4 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -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;
> +	}

Earlier in this routine we also have this:

if (!parent->set_performance_state)
        continue;

Should we change that too ?

>  
>  	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;

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Ulf Hansson Jan. 18, 2021, 10:59 a.m. UTC | #2
On Mon, 18 Jan 2021 at 08:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 04:13, Dmitry Osipenko wrote:
> > 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 <pgwipeout@gmail.com>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/base/power/domain.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 9a14eedacb92..a3e1bfc233d4 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -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;
> > +     }
>
> Earlier in this routine we also have this:
>
> 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?

>
> >
> >       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;
>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> --
> viresh

Kind regards
Uffe
Viresh Kumar Jan. 18, 2021, 11:05 a.m. UTC | #3
On 18-01-21, 11:59, Ulf Hansson wrote:
> 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 ?
Ulf Hansson Jan. 18, 2021, 12:46 p.m. UTC | #4
On Mon, 18 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 11:59, Ulf Hansson wrote:
> > 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 ?

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
Dmitry Osipenko Jan. 18, 2021, 6:44 p.m. UTC | #5
18.01.2021 13:59, Ulf Hansson пишет:
> On Mon, 18 Jan 2021 at 08:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> On 18-01-21, 04:13, Dmitry Osipenko wrote:
>>> 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 <pgwipeout@gmail.com>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/base/power/domain.c | 11 +++++------
>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 9a14eedacb92..a3e1bfc233d4 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -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;
>>> +     }
>>
>> Earlier in this routine we also have this:
>>
>> 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?

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.
Viresh Kumar Jan. 19, 2021, 3:44 a.m. UTC | #6
On 18-01-21, 13:46, Ulf Hansson wrote:
> 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.

> 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.

> 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.
Ulf Hansson Jan. 19, 2021, 9:52 a.m. UTC | #7
On Tue, 19 Jan 2021 at 04:44, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 13:46, Ulf Hansson wrote:
> > 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.

>
> > 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.

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.

>
> > 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 Jan. 19, 2021, 9:55 a.m. UTC | #8
On 19-01-21, 10:52, Ulf Hansson wrote:
> 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.

> >
> > > 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.
diff mbox series

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 9a14eedacb92..a3e1bfc233d4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -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;