Re: [PATCH v2] thermal: intel_powerclamp: Fix cur_state for multi package system

[Rafael J. Wysocki]

On Wed, Feb 1, 2023 at 9:39 PM Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> The powerclamp cooling device cur_state shows actual idle observed by
> package C-state idle counters. But the implementation is not sufficient
> for multi package or multi die system. The cur_state value is incorrect.
> On these systems, these counters must be read from each package/die and
> somehow aggregate them. But there is no good method for aggregation.
>
> It was not a problem when explicit CPU model addition was required to
> enable intel powerclamp. In this way certain CPU models could have
> been avoided. But with the removal of CPU model check with the
> availability of Package C-state counters, the driver is loaded on most
> of the recent systems.
>
> For multi package/die systems, just show the actual target idle state,
> the system is trying to achieve. In powerclamp this is the user set
> state minus one.
>
> Also there is no use of starting a worker thread for polling package
> C-state counters and applying any compensation for multiple package
> or multiple die systems.
>
> Fixes: b721ca0d1927 ("thermal/powerclamp: remove cpu whitelist")
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # 4.14+
> ---
> v2:
> Changed: (true == clamping) to (clamping)
> Updated commit description for the last paragraph
>
>  drivers/thermal/intel/intel_powerclamp.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
> index b80e25ec1261..2f4cbfdf26a0 100644
> --- a/drivers/thermal/intel/intel_powerclamp.c
> +++ b/drivers/thermal/intel/intel_powerclamp.c
> @@ -57,6 +57,7 @@
>
>  static unsigned int target_mwait;
>  static struct dentry *debug_dir;
> +static bool poll_pkg_cstate_enable;
>
>  /* user selected target */
>  static unsigned int set_target_ratio;
> @@ -261,6 +262,9 @@ static unsigned int get_compensation(int ratio)
>  {
>         unsigned int comp = 0;
>
> +       if (!poll_pkg_cstate_enable)
> +               return 0;
> +
>         /* we only use compensation if all adjacent ones are good */
>         if (ratio == 1 &&
>                 cal_data[ratio].confidence >= CONFIDENCE_OK &&
> @@ -519,7 +523,8 @@ static int start_power_clamp(void)
>         control_cpu = cpumask_first(cpu_online_mask);
>
>         clamping = true;
> -       schedule_delayed_work(&poll_pkg_cstate_work, 0);
> +       if (poll_pkg_cstate_enable)
> +               schedule_delayed_work(&poll_pkg_cstate_work, 0);
>
>         /* start one kthread worker per online cpu */
>         for_each_online_cpu(cpu) {
> @@ -585,11 +590,15 @@ static int powerclamp_get_max_state(struct thermal_cooling_device *cdev,
>  static int powerclamp_get_cur_state(struct thermal_cooling_device *cdev,
>                                  unsigned long *state)
>  {
> -       if (true == clamping)
> -               *state = pkg_cstate_ratio_cur;
> -       else
> +       if (clamping) {
> +               if (poll_pkg_cstate_enable)
> +                       *state = pkg_cstate_ratio_cur;
> +               else
> +                       *state = set_target_ratio;
> +       } else {
>                 /* to save power, do not poll idle ratio while not clamping */
>                 *state = -1; /* indicates invalid state */
> +       }
>
>         return 0;
>  }
> @@ -712,6 +721,9 @@ static int __init powerclamp_init(void)
>                 goto exit_unregister;
>         }
>
> +       if (topology_max_packages() == 1 && topology_max_die_per_package() == 1)
> +               poll_pkg_cstate_enable = true;
> +
>         cooling_dev = thermal_cooling_device_register("intel_powerclamp", NULL,
>                                                 &powerclamp_cooling_ops);
>         if (IS_ERR(cooling_dev)) {
> --

Applied as 6.3 material, thanks!