[PATCH 0/3] firmware: arm_scmi: Register and handle limits change notification

[PATCH 0/3] firmware: arm_scmi: Register and handle limits change notification

[Sibi Sankar]

This series registers for scmi limits change notifications and adds
perf_notify_support/perf_opp_xlate interfaces which are used by the
scmi cpufreq driver to determine the throttled frequency and apply HW
pressure.

Depends on:
HW pressure: https://patchwork.kernel.org/project/linux-arm-msm/cover/20231221152407.436177-1-vincent.guittot@linaro.org/

Sibi Sankar (3):
  firmware: arm_scmi: Add perf_notify_support interface
  firmware: arm_scmi: Add perf_opp_xlate interface
  cpufreq: scmi: Register for limit change notifications

 drivers/cpufreq/scmi-cpufreq.c   | 42 +++++++++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/perf.c | 37 ++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h    | 11 +++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] firmware: arm_scmi: Add perf_notify_support interface

[Sibi Sankar]

Add a new perf_notify_support interface to the existing perf_ops to export
info regarding limit/level change notification support.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/firmware/arm_scmi/perf.c | 16 ++++++++++++++++
 include/linux/scmi_protocol.h    |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 8ea2a7b3d35d..c167bb5e3607 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -949,6 +949,21 @@ scmi_power_scale_get(const struct scmi_protocol_handle *ph)
 	return pi->power_scale;
 }
 
+static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain,
+			       struct scmi_perf_notify_info *info)
+{
+	struct perf_dom_info *dom;
+
+	dom = scmi_perf_domain_lookup(ph, domain);
+	if (IS_ERR(dom))
+		return -EINVAL;
+
+	info->perf_limit_notify = dom->perf_limit_notify;
+	info->perf_level_notify = dom->perf_level_notify;
+
+	return 0;
+}
+
 static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.num_domains_get = scmi_perf_num_domains_get,
 	.info_get = scmi_perf_info_get,
@@ -963,6 +978,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.est_power_get = scmi_dvfs_est_power_get,
 	.fast_switch_possible = scmi_fast_switch_possible,
 	.power_scale_get = scmi_power_scale_get,
+	.perf_notify_support = scmi_notify_support,
 };
 
 static int scmi_perf_set_notify_enabled(const struct scmi_protocol_handle *ph,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..b0947d004826 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -117,6 +117,11 @@ struct scmi_perf_domain_info {
 	bool set_perf;
 };
 
+struct scmi_perf_notify_info {
+	bool perf_limit_notify;
+	bool perf_level_notify;
+};
+
 /**
  * struct scmi_perf_proto_ops - represents the various operations provided
  *	by SCMI Performance Protocol
@@ -139,6 +144,7 @@ struct scmi_perf_domain_info {
  *	for a given device
  * @power_scale_mw_get: indicates if the power values provided are in milliWatts
  *	or in some other (abstract) scale
+ * @perf_notify_support: indicates if limit and level change notification is supported
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
@@ -165,6 +171,8 @@ struct scmi_perf_proto_ops {
 	bool (*fast_switch_possible)(const struct scmi_protocol_handle *ph,
 				     u32 domain);
 	enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
+	int (*perf_notify_support)(const struct scmi_protocol_handle *ph, u32 domain,
+				   struct scmi_perf_notify_info *info);
 };
 
 /**
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] firmware: arm_scmi: Add perf_opp_xlate interface

[Sibi Sankar]

Add a new perf_opp_xlate interface to the existing perf_ops to translate
a given perf index to frequency.

This can be used by the cpufreq driver and framework to determine the
throttled frequency from a given perf index and apply HW pressure
accordingly.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
 include/linux/scmi_protocol.h    |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index c167bb5e3607..f26390924e1c 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -964,6 +964,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
 	return 0;
 }
 
+static int scmi_perf_opp_xlate(const struct scmi_protocol_handle *ph, u32 domain,
+			       int idx, unsigned long *freq)
+{
+	struct perf_dom_info *dom;
+
+	dom = scmi_perf_domain_lookup(ph, domain);
+	if (IS_ERR(dom))
+		return PTR_ERR(dom);
+
+	if (idx >= dom->opp_count)
+		return -ERANGE;
+
+	if (!dom->level_indexing_mode)
+		*freq = dom->opp[idx].perf * dom->mult_factor;
+	else
+		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
+
+	return 0;
+}
+
 static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.num_domains_get = scmi_perf_num_domains_get,
 	.info_get = scmi_perf_info_get,
@@ -979,6 +999,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.fast_switch_possible = scmi_fast_switch_possible,
 	.power_scale_get = scmi_power_scale_get,
 	.perf_notify_support = scmi_notify_support,
+	.perf_opp_xlate = scmi_perf_opp_xlate,
 };
 
 static int scmi_perf_set_notify_enabled(const struct scmi_protocol_handle *ph,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b0947d004826..9c285ae89848 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -145,6 +145,7 @@ struct scmi_perf_notify_info {
  * @power_scale_mw_get: indicates if the power values provided are in milliWatts
  *	or in some other (abstract) scale
  * @perf_notify_support: indicates if limit and level change notification is supported
+ * @perf_opp_xlate: translates the given perf index to frequency in Hz
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
@@ -173,6 +174,8 @@ struct scmi_perf_proto_ops {
 	enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
 	int (*perf_notify_support)(const struct scmi_protocol_handle *ph, u32 domain,
 				   struct scmi_perf_notify_info *info);
+	int (*perf_opp_xlate)(const struct scmi_protocol_handle *ph, u32 domain,
+			      int idx, unsigned long *freq);
 };
 
 /**
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] firmware: arm_scmi: Add perf_opp_xlate interface

[Viresh Kumar]

On 08-01-24, 19:31, Sibi Sankar wrote:
> Add a new perf_opp_xlate interface to the existing perf_ops to translate
> a given perf index to frequency.
> 
> This can be used by the cpufreq driver and framework to determine the
> throttled frequency from a given perf index and apply HW pressure
> accordingly.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
>  include/linux/scmi_protocol.h    |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index c167bb5e3607..f26390924e1c 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -964,6 +964,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
>  	return 0;
>  }
>  
> +static int scmi_perf_opp_xlate(const struct scmi_protocol_handle *ph, u32 domain,
> +			       int idx, unsigned long *freq)
> +{
> +	struct perf_dom_info *dom;
> +
> +	dom = scmi_perf_domain_lookup(ph, domain);
> +	if (IS_ERR(dom))
> +		return PTR_ERR(dom);
> +
> +	if (idx >= dom->opp_count)
> +		return -ERANGE;
> +
> +	if (!dom->level_indexing_mode)
> +		*freq = dom->opp[idx].perf * dom->mult_factor;
> +	else
> +		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
> +
> +	return 0;
> +}
> +
>  static const struct scmi_perf_proto_ops perf_proto_ops = {
>  	.num_domains_get = scmi_perf_num_domains_get,
>  	.info_get = scmi_perf_info_get,
> @@ -979,6 +999,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
>  	.fast_switch_possible = scmi_fast_switch_possible,
>  	.power_scale_get = scmi_power_scale_get,
>  	.perf_notify_support = scmi_notify_support,
> +	.perf_opp_xlate = scmi_perf_opp_xlate,

The use of "opp" here is a bit confusing as this doesn't have anything to do
with the OPP framework and you are only getting the frequency out of it after
all.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] firmware: arm_scmi: Add perf_opp_xlate interface

[Sibi Sankar]

On 1/10/24 12:59, Viresh Kumar wrote:
> On 08-01-24, 19:31, Sibi Sankar wrote:
>> Add a new perf_opp_xlate interface to the existing perf_ops to translate
>> a given perf index to frequency.
>>

Hey Viresh,
Thanks for taking time to review the series!

>> This can be used by the cpufreq driver and framework to determine the
>> throttled frequency from a given perf index and apply HW pressure
>> accordingly.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
>>   include/linux/scmi_protocol.h    |  3 +++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
>> index c167bb5e3607..f26390924e1c 100644
>> --- a/drivers/firmware/arm_scmi/perf.c
>> +++ b/drivers/firmware/arm_scmi/perf.c
>> @@ -964,6 +964,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
>>   	return 0;
>>   }
>>   
>> +static int scmi_perf_opp_xlate(const struct scmi_protocol_handle *ph, u32 domain,
>> +			       int idx, unsigned long *freq)
>> +{
>> +	struct perf_dom_info *dom;
>> +
>> +	dom = scmi_perf_domain_lookup(ph, domain);
>> +	if (IS_ERR(dom))
>> +		return PTR_ERR(dom);
>> +
>> +	if (idx >= dom->opp_count)
>> +		return -ERANGE;
>> +
>> +	if (!dom->level_indexing_mode)
>> +		*freq = dom->opp[idx].perf * dom->mult_factor;
>> +	else
>> +		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct scmi_perf_proto_ops perf_proto_ops = {
>>   	.num_domains_get = scmi_perf_num_domains_get,
>>   	.info_get = scmi_perf_info_get,
>> @@ -979,6 +999,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
>>   	.fast_switch_possible = scmi_fast_switch_possible,
>>   	.power_scale_get = scmi_power_scale_get,
>>   	.perf_notify_support = scmi_notify_support,
>> +	.perf_opp_xlate = scmi_perf_opp_xlate,
> 
> The use of "opp" here is a bit confusing as this doesn't have anything to do
> with the OPP framework and you are only getting the frequency out of it after
> all.

Sure will re-name it.

-Sibi

> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] cpufreq: scmi: Register for limit change notifications

[Sibi Sankar]

Register for limit change notifications if supported with the help of
perf_notify_support interface and determine the throttled frequency
using the perf_opp_xlate to apply HW pressure.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 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..53bc8868455d 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_opp_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
+		return NOTIFY_OK;
+
+	/* Update HW pressure (the boost frequencies are accepted) */
+	arch_update_hw_pressure(policy->related_cpus, (freq_hz / HZ_PER_KHZ));
+
+	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)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] cpufreq: scmi: Register for limit change notifications

[Lukasz Luba]

Hi Sibi,

+ Morten and Dietmar on CC

On 1/8/24 14:01, 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_opp_xlate to apply HW pressure.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>   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..53bc8868455d 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_opp_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
> +		return NOTIFY_OK;
> +
> +	/* Update HW pressure (the boost frequencies are accepted) */
> +	arch_update_hw_pressure(policy->related_cpus, (freq_hz / HZ_PER_KHZ));

This is wrong. The whole idea of the new HW pressure was that I wanted
to get rid of the 'signal smoothing' mechanism in order to get
instantaneous value from FW to task scheduler. Vincent created
2 interfaces in that new HW pressure:
1. cpufreq_update_pressure(policy) - raw variable
2. arch_update_hw_pressure(policy->related_cpus, (freq_hz / HZ_PER_KHZ))
    - smoothing PELT mechanism, good for raw IRQ in drivers

In our SCMI cpufreq driver we need the 1st one:
cpufreq_update_pressure(policy)

The FW will do the 'signal smoothing or filtering' and won't
flood the kernel with hundreds of notifications.

So, please change that bit and add me, Morten and Dietmar on CC.
I would like to review it.

Regards,
Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] cpufreq: scmi: Register for limit change notifications

[Sibi Sankar]

On 1/10/24 13:56, Lukasz Luba wrote:
> Hi Sibi,
> 

Hey Lukasz,
Thanks for taking time to review the series!

> + Morten and Dietmar on CC
> 
> On 1/8/24 14:01, 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_opp_xlate to apply HW pressure.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   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..53bc8868455d 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_opp_xlate(ph, priv->domain_id, 
>> limit_notify->range_max, &freq_hz))
>> +        return NOTIFY_OK;
>> +
>> +    /* Update HW pressure (the boost frequencies are accepted) */
>> +    arch_update_hw_pressure(policy->related_cpus, (freq_hz / 
>> HZ_PER_KHZ));
> 
> This is wrong. The whole idea of the new HW pressure was that I wanted
> to get rid of the 'signal smoothing' mechanism in order to get
> instantaneous value from FW to task scheduler. Vincent created
> 2 interfaces in that new HW pressure:
> 1. cpufreq_update_pressure(policy) - raw variable
> 2. arch_update_hw_pressure(policy->related_cpus, (freq_hz / HZ_PER_KHZ))
>     - smoothing PELT mechanism, good for raw IRQ in drivers
> 
> In our SCMI cpufreq driver we need the 1st one:
> cpufreq_update_pressure(policy)
> 
> The FW will do the 'signal smoothing or filtering' and won't
> flood the kernel with hundreds of notifications.

Ack, even though I see no mention of filtering being mandated in the 
SCMI specification, the scmi notification by itself will serve as a
rate limiter I guess.

> 
> So, please change that bit and add me, Morten and Dietmar on CC.
> I would like to review it.

ack

-Sibi

> 
> Regards,
> Lukasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] cpufreq: scmi: Register for limit change notifications

[Lukasz Luba]

On 1/17/24 02:58, Sibi Sankar wrote:
> 
> 
> On 1/10/24 13:56, Lukasz Luba wrote:
>> Hi Sibi,
>>
> 
> Hey Lukasz,
> Thanks for taking time to review the series!
> 
>> + Morten and Dietmar on CC
>>
>> On 1/8/24 14:01, 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_opp_xlate to apply HW pressure.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>>   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..53bc8868455d 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_opp_xlate(ph, priv->domain_id, 
>>> limit_notify->range_max, &freq_hz))
>>> +        return NOTIFY_OK;
>>> +
>>> +    /* Update HW pressure (the boost frequencies are accepted) */
>>> +    arch_update_hw_pressure(policy->related_cpus, (freq_hz / 
>>> HZ_PER_KHZ));
>>
>> This is wrong. The whole idea of the new HW pressure was that I wanted
>> to get rid of the 'signal smoothing' mechanism in order to get
>> instantaneous value from FW to task scheduler. Vincent created
>> 2 interfaces in that new HW pressure:
>> 1. cpufreq_update_pressure(policy) - raw variable
>> 2. arch_update_hw_pressure(policy->related_cpus, (freq_hz / HZ_PER_KHZ))
>>     - smoothing PELT mechanism, good for raw IRQ in drivers
>>
>> In our SCMI cpufreq driver we need the 1st one:
>> cpufreq_update_pressure(policy)
>>
>> The FW will do the 'signal smoothing or filtering' and won't
>> flood the kernel with hundreds of notifications.
> 
> Ack, even though I see no mention of filtering being mandated in the 
> SCMI specification, the scmi notification by itself will serve as a
> rate limiter I guess.
> 
>>
>> So, please change that bit and add me, Morten and Dietmar on CC.
>> I would like to review it.
> 

True, the SCMI protocol doesn't describe the rate or limits of
often these performance limit notifications can be sent.
It's too HW specific and some balance has to made to not
flood the kernel with hundreds or thousands of notifications
per second. That could overload the SCMI channel.

The FW implementation has to combine the perf. limit
restrictions from different areas: thermal, power
conditions, MPMM, etc. Some smarter approach in FW
to the processing and filtering of perf limit notification
would be needed. The kernel is not able to do the job at the same
quality as FW in those areas.

Therefore, in the kernel HW pressure signal we don't need
another 'filtering or smoothing' on already processed
SCMI notification information. That could even harm us
if we don't get that FW information in kernel at right time
due to convergence delays of the HW pressure w/ PELT smoothing.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] firmware: arm_scmi: Register and handle limits change notification

[Cristian Marussi]

On Mon, Jan 08, 2024 at 07:31:15PM +0530, Sibi Sankar wrote:
> This series registers for scmi limits change notifications and adds
> perf_notify_support/perf_opp_xlate interfaces which are used by the
> scmi cpufreq driver to determine the throttled frequency and apply HW
> pressure.
> 

Hi,

a few initial remarks from the mere SCMI standpoint.

Unlinke most SCMI protocols that expose domains info bits via an
*info_get protocol operation, PERF does no do this since (till now) there
wasn't a compelling reason (i.e. users)

Ulf recently in his GenPD/SCMI series recently started exposing something
and now you need to expose even more, adding also a new xlate ops.

For the sake of simplicity, I think that we could now expose straight
away the whole perf_domain_info and embedded structs via the usual *info_get.

After having done that, you can just drop your patch 1 and 2 since you
can access the needed info from the cpufreq_driver right away.

Having said, I have already such patch ready (for my internal testing), I
wll post it by the end of week after a minor cleanup, if you can bear with me.

Thoughts ?

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] firmware: arm_scmi: Register and handle limits change notification

[Sibi Sankar]

On 1/17/24 15:11, Cristian Marussi wrote:
> On Mon, Jan 08, 2024 at 07:31:15PM +0530, Sibi Sankar wrote:
>> This series registers for scmi limits change notifications and adds
>> perf_notify_support/perf_opp_xlate interfaces which are used by the
>> scmi cpufreq driver to determine the throttled frequency and apply HW
>> pressure.
>>
> 
> Hi,
> 
> a few initial remarks from the mere SCMI standpoint.
> 
> Unlinke most SCMI protocols that expose domains info bits via an
> *info_get protocol operation, PERF does no do this since (till now) there
> wasn't a compelling reason (i.e. users)
> 
> Ulf recently in his GenPD/SCMI series recently started exposing something
> and now you need to expose even more, adding also a new xlate ops.
> 
> For the sake of simplicity, I think that we could now expose straight
> away the whole perf_domain_info and embedded structs via the usual *info_get.
> 
> After having done that, you can just drop your patch 1 and 2 since you
> can access the needed info from the cpufreq_driver right away.
> 
> Having said, I have already such patch ready (for my internal testing), I
> wll post it by the end of week after a minor cleanup, if you can bear with me.
> 
> Thoughts ?

Ack, just from the naming I initially thought info_get would include
everything but it just exposed minimal info. We certainly don't want to
keep adding very similar ops just to expose more such info. I'll re-send
the remainder of the series after you are done with your patches.
Thanks.

-Sibi

> 
> Thanks,
> Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel