| Message ID | 20240117104116.2055349-5-quic_sibis@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: arm_scmi: Register and handle limits change notification | expand |
On Wed, Jan 17, 2024 at 04:11:16PM +0530, Sibi Sankar wrote: > Register for limit change notifications if supported with the help of > perf_notify_support interface and determine the throttled frequency > using the perf_freq_xlate to apply HW pressure. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > > v2: > * Export cpufreq_update_pressure and use it directly [Lukasz] > > drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c > index 4ee23f4ebf4a..e0aa85764451 100644 > --- a/drivers/cpufreq/scmi-cpufreq.c > +++ b/drivers/cpufreq/scmi-cpufreq.c > @@ -25,9 +25,13 @@ struct scmi_data { > int domain_id; > int nr_opp; > struct device *cpu_dev; > + struct cpufreq_policy *policy; > cpumask_var_t opp_shared_cpus; > + struct notifier_block limit_notify_nb; > }; > > +const struct scmi_handle *handle; > +static struct scmi_device *scmi_dev; > static struct scmi_protocol_handle *ph; > static const struct scmi_perf_proto_ops *perf_ops; > > @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power, > return 0; > } > > +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data) > +{ > + unsigned long freq_hz; > + struct scmi_perf_limits_report *limit_notify = data; > + struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb); > + struct cpufreq_policy *policy = priv->policy; > + > + if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz)) > + return NOTIFY_OK; > + > + policy->max = freq_hz / HZ_PER_KHZ; > + cpufreq_update_pressure(policy); > + > + return NOTIFY_OK; > +} > + > static int scmi_cpufreq_init(struct cpufreq_policy *policy) > { > int ret, nr_opp, domain; > @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) > struct device *cpu_dev; > struct scmi_data *priv; > struct cpufreq_frequency_table *freq_table; > + struct scmi_perf_notify_info info = {}; > > cpu_dev = get_cpu_device(policy->cpu); > if (!cpu_dev) { > @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) > policy->fast_switch_possible = > perf_ops->fast_switch_possible(ph, domain); > > + ret = perf_ops->perf_notify_support(ph, domain, &info); > + if (ret) > + dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret); > + > + if (info.perf_limit_notify) { > + priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb; > + ret = handle->notify_ops->devm_event_notifier_register(scmi_dev, SCMI_PROTOCOL_PERF, > + SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED, > + &domain, > + &priv->limit_notify_nb); > + if (ret) { > + dev_err(cpu_dev, "Error in registering limit change notifier for domain %d\n", > + domain); > + return ret; > + } Is there a reason to fail completely here if it was not possible to register the notifier ? (even though expected to succeed given perf_limit_notify was true...) Maybe a big fat warn that the system perf could be degraded, but carrying on ? Or maybe you have in mind a good reason to fail like you did, so please explain in that case in a comment. Thanks, Cristian
On 1/29/24 21:29, Cristian Marussi wrote: > On Wed, Jan 17, 2024 at 04:11:16PM +0530, Sibi Sankar wrote: >> Register for limit change notifications if supported with the help of >> perf_notify_support interface and determine the throttled frequency >> using the perf_freq_xlate to apply HW pressure. >> Christian, Thanks for taking time to review the series. >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> >> v2: >> * Export cpufreq_update_pressure and use it directly [Lukasz] >> >> drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++- >> 1 file changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c >> index 4ee23f4ebf4a..e0aa85764451 100644 >> --- a/drivers/cpufreq/scmi-cpufreq.c >> +++ b/drivers/cpufreq/scmi-cpufreq.c >> @@ -25,9 +25,13 @@ struct scmi_data { >> int domain_id; >> int nr_opp; >> struct device *cpu_dev; >> + struct cpufreq_policy *policy; >> cpumask_var_t opp_shared_cpus; >> + struct notifier_block limit_notify_nb; >> }; >> >> +const struct scmi_handle *handle; >> +static struct scmi_device *scmi_dev; >> static struct scmi_protocol_handle *ph; >> static const struct scmi_perf_proto_ops *perf_ops; >> >> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power, >> return 0; >> } >> >> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data) >> +{ >> + unsigned long freq_hz; >> + struct scmi_perf_limits_report *limit_notify = data; >> + struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb); >> + struct cpufreq_policy *policy = priv->policy; >> + >> + if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz)) >> + return NOTIFY_OK; >> + >> + policy->max = freq_hz / HZ_PER_KHZ; >> + cpufreq_update_pressure(policy); >> + >> + return NOTIFY_OK; >> +} >> + >> static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> { >> int ret, nr_opp, domain; >> @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> struct device *cpu_dev; >> struct scmi_data *priv; >> struct cpufreq_frequency_table *freq_table; >> + struct scmi_perf_notify_info info = {}; >> >> cpu_dev = get_cpu_device(policy->cpu); >> if (!cpu_dev) { >> @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> policy->fast_switch_possible = >> perf_ops->fast_switch_possible(ph, domain); >> >> + ret = perf_ops->perf_notify_support(ph, domain, &info); >> + if (ret) >> + dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret); >> + >> + if (info.perf_limit_notify) { >> + priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb; >> + ret = handle->notify_ops->devm_event_notifier_register(scmi_dev, SCMI_PROTOCOL_PERF, >> + SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED, >> + &domain, >> + &priv->limit_notify_nb); >> + if (ret) { >> + dev_err(cpu_dev, "Error in registering limit change notifier for domain %d\n", >> + domain); >> + return ret; >> + } > > Is there a reason to fail completely here if it was not possible to register > the notifier ? (even though expected to succeed given perf_limit_notify > was true...) > > Maybe a big fat warn that the system perf could be degraded, but > carrying on ? > > Or maybe you have in mind a good reason to fail like you did, so please > explain in that case in a comment. ack a warn should suffice here -Sibi > > Thanks, > Cristian
On 1/31/24 19:59, Pierre Gondois wrote: > Hello Sibi, > > On 1/17/24 11:41, Sibi Sankar wrote: >> Register for limit change notifications if supported with the help of >> perf_notify_support interface and determine the throttled frequency >> using the perf_freq_xlate to apply HW pressure. >> Hey Pierre, Thanks for taking time to review the series. >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> >> v2: >> * Export cpufreq_update_pressure and use it directly [Lukasz] >> >> drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++- >> 1 file changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/scmi-cpufreq.c >> b/drivers/cpufreq/scmi-cpufreq.c >> index 4ee23f4ebf4a..e0aa85764451 100644 >> --- a/drivers/cpufreq/scmi-cpufreq.c >> +++ b/drivers/cpufreq/scmi-cpufreq.c >> @@ -25,9 +25,13 @@ struct scmi_data { >> int domain_id; >> int nr_opp; >> struct device *cpu_dev; >> + struct cpufreq_policy *policy; >> cpumask_var_t opp_shared_cpus; >> + struct notifier_block limit_notify_nb; >> }; >> +const struct scmi_handle *handle; >> +static struct scmi_device *scmi_dev; >> static struct scmi_protocol_handle *ph; >> static const struct scmi_perf_proto_ops *perf_ops; >> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, >> unsigned long *power, >> return 0; >> } >> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned >> long event, void *data) >> +{ >> + unsigned long freq_hz; >> + struct scmi_perf_limits_report *limit_notify = data; >> + struct scmi_data *priv = container_of(nb, struct scmi_data, >> limit_notify_nb); >> + struct cpufreq_policy *policy = priv->policy; >> + >> + if (perf_ops->perf_freq_xlate(ph, priv->domain_id, >> limit_notify->range_max, &freq_hz)) >> + return NOTIFY_OK; >> + >> + policy->max = freq_hz / HZ_PER_KHZ; > > Maybe 'policy->max' should be checked. The limits received by SCMI is > blindly > trusted. This might be ok, but could also lead to some inconsistency. > > The scmi_cpufreq_driver's verify() callback could be used. ack, will fix this in the next re-spin. > > --- > > I think there might also be corner cases where the SCP might advertise > the maximum boosted frequency as the max limit, but boosting might not > be enabled on the kernel side. > So I think this should be checked when setting 'policy->max', ack -Sibi > > Regards, > Pierre > [...] >> if (!handle)
diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c index 4ee23f4ebf4a..e0aa85764451 100644 --- a/drivers/cpufreq/scmi-cpufreq.c +++ b/drivers/cpufreq/scmi-cpufreq.c @@ -25,9 +25,13 @@ struct scmi_data { int domain_id; int nr_opp; struct device *cpu_dev; + struct cpufreq_policy *policy; cpumask_var_t opp_shared_cpus; + struct notifier_block limit_notify_nb; }; +const struct scmi_handle *handle; +static struct scmi_device *scmi_dev; static struct scmi_protocol_handle *ph; static const struct scmi_perf_proto_ops *perf_ops; @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power, return 0; } +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data) +{ + unsigned long freq_hz; + struct scmi_perf_limits_report *limit_notify = data; + struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb); + struct cpufreq_policy *policy = priv->policy; + + if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz)) + return NOTIFY_OK; + + policy->max = freq_hz / HZ_PER_KHZ; + cpufreq_update_pressure(policy); + + return NOTIFY_OK; +} + static int scmi_cpufreq_init(struct cpufreq_policy *policy) { int ret, nr_opp, domain; @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) struct device *cpu_dev; struct scmi_data *priv; struct cpufreq_frequency_table *freq_table; + struct scmi_perf_notify_info info = {}; cpu_dev = get_cpu_device(policy->cpu); if (!cpu_dev) { @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) policy->fast_switch_possible = perf_ops->fast_switch_possible(ph, domain); + ret = perf_ops->perf_notify_support(ph, domain, &info); + if (ret) + dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret); + + if (info.perf_limit_notify) { + priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb; + ret = handle->notify_ops->devm_event_notifier_register(scmi_dev, SCMI_PROTOCOL_PERF, + SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED, + &domain, + &priv->limit_notify_nb); + if (ret) { + dev_err(cpu_dev, "Error in registering limit change notifier for domain %d\n", + domain); + return ret; + } + } + + priv->policy = policy; + return 0; out_free_opp: @@ -321,8 +361,8 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev) { int ret; struct device *dev = &sdev->dev; - const struct scmi_handle *handle; + scmi_dev = sdev; handle = sdev->handle; if (!handle)
Register for limit change notifications if supported with the help of perf_notify_support interface and determine the throttled frequency using the perf_freq_xlate to apply HW pressure. Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- v2: * Export cpufreq_update_pressure and use it directly [Lukasz] drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)