On 18/05/2022 10:05, Burakov, Anatoly wrote:
On 19-Apr-22 12:25 PM, Kevin Laatz wrote:
Add new get/set API to allow the user or application to set the minimum
and maximum frequencies to use when scaling.
Previously, the frequency range was determined by the HW capabilities of
the CPU. With this new API, the user or application can constrain this
if required.

Signed-off-by: Kevin Laatz <kevin.la...@intel.com>
---

<snip>

  +int
+rte_power_pmd_mgmt_set_scaling_freq_min(unsigned int lcore, unsigned int min)
+{
+    if (lcore >= RTE_MAX_LCORE) {
+        RTE_LOG(ERR, POWER, "Invalid lcore ID: %u\n", lcore);
+        rte_errno = EINVAL;
+        return -1;
+    }
+    scale_freq_min[lcore] = min;

Are there any constraints on the value ranges, or are we just going to accept any and all values? If the idea was to allow valid values plus some special "default" value, you can still restrict the range, but allow 0 as a special case?

When writing min/max values to HW the values are clamped. Since the API takes unsigned integer for the frequency value (in this case 'min'), any value can be considered as 'valid'.

That being said, this should at least check that min <= max for the same lcore. I'll add this for v3.



+
+    return 0;
+}
+
+int
+rte_power_pmd_mgmt_set_scaling_freq_max(unsigned int lcore, unsigned int max)
+{
+    if (lcore >= RTE_MAX_LCORE) {
+        RTE_LOG(ERR, POWER, "Invalid lcore ID: %u\n", lcore);
+        rte_errno = EINVAL;
+        return -1;
+    }
+    scale_freq_max[lcore] = max;

Same as above. Also, do we want UINT32_MAX be the "special" value for the "max" case? What do you think of having "0" as "not set", but maybe set it internally to UINT32_MAX if you still want to keep using the RTE_MIN/MAX macros?

Similar to  'set_scaling_freq_min', the value will be clamped by HW so any value can be considered 'valid'. I don't see the benefit of having "0" for not set, since UINT32_MAX will achieve the same result, i.e. the value won't be used (it will fall back the max value in sysfs). Do you have a use-case for it if we don't need a 'special case'?

Will add a check to make sure max >= min for v3.



+
+    return 0;
+}
+
+int
+rte_power_pmd_mgmt_get_scaling_freq_min(unsigned int lcore)
+{

<snip>

diff --git a/lib/power/rte_power_pmd_mgmt.h b/lib/power/rte_power_pmd_mgmt.h
index 18a9c3abb5..74e3fa710b 100644
--- a/lib/power/rte_power_pmd_mgmt.h
+++ b/lib/power/rte_power_pmd_mgmt.h
@@ -148,6 +148,86 @@ __rte_experimental
  unsigned int
  rte_power_pmd_mgmt_get_pause_duration(void);
  +/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
+ *
+ * Set the min frequency to be used for frequency scaling.
+ *
+ * @note Supported by: Pstate mode.
+ *
+ * @param lcore
+ *   The ID of the lcore to set the min frequency for.
+ * @param min
+ *   The value, in Hertz, to set the minimum frequency to.

Is it really in Hertz? As far as I can tell, it's in steps of 100MHz (BUS_FREQ).

Correct, the frequency changes in steps of 100MHz, but the value passed to 'min' is in kHz - will ammend the comments.


Reply via email to