On 01.05.2023 21:30, Jason Andryuk wrote:
> Print HWP-specific parameters. Some are always present, but others
> depend on hardware support.
>
> Signed-off-by: Jason Andryuk <jandr...@gmail.com>
> ---
> v2:
> Style fixes
> Declare i outside loop
> Replace repearted hardware/configured limits with spaces
> Fixup for hw_ removal
> Use XEN_HWP_GOVERNOR
> Use HWP_ACT_WINDOW_EXPONENT_*
> Remove energy_perf hw autonomous - 0 doesn't mean autonomous
> ---
> tools/misc/xenpm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index ce8d7644d0..b2defde0d4 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -708,6 +708,44 @@ void start_gather_func(int argc, char *argv[])
> pause();
> }
>
> +static void calculate_hwp_activity_window(const xc_hwp_para_t *hwp,
> + unsigned int *activity_window,
> + const char **units)
The function's return value would be nice to use for one of the two
values that are being returned.
> +{
> + unsigned int mantissa = hwp->activity_window &
> HWP_ACT_WINDOW_MANTISSA_MASK;
> + unsigned int exponent =
> + (hwp->activity_window >> HWP_ACT_WINDOW_EXPONENT_SHIFT) &
> + HWP_ACT_WINDOW_EXPONENT_MASK;
I wish we had MASK_EXTR() in common-macros.h. While really a comment on
patch 7 - HWP_ACT_WINDOW_EXPONENT_SHIFT is redundant information and
should imo be omitted from the public interface, in favor of just a
(suitably shifted) mask value. Also note how those constants all lack
proper XEN_ prefixes.
> + unsigned int multiplier = 1;
> + unsigned int i;
> +
> + if ( hwp->activity_window == 0 )
> + {
> + *units = "hardware selected";
> + *activity_window = 0;
> +
> + return;
> + }
While in line with documentation, any mantissa of 0 results in a 0us
window, which I assume would then also mean "hardware selected".
> @@ -773,6 +811,33 @@ static void print_cpufreq_para(int cpuid, struct
> xc_get_cpufreq_para *p_cpufreq)
> p_cpufreq->scaling_cur_freq);
> }
>
> + if ( strcmp(p_cpufreq->scaling_governor, XEN_HWP_GOVERNOR) == 0 )
> + {
> + const xc_hwp_para_t *hwp = &p_cpufreq->u.hwp_para;
> +
> + printf("hwp variables :\n");
> + printf(" hardware limits : lowest [%u] most_efficient [%u]\n",
Here and ...
> + hwp->lowest, hwp->most_efficient);
> + printf(" : guaranteed [%u] highest [%u]\n",
> + hwp->guaranteed, hwp->highest);
> + printf(" configured limits : min [%u] max [%u] energy_perf [%u]\n",
... here I wonder what use the underscores are in produced output. I'd
use blanks. If you really want a separator there, then please use
dashes.
Jan
