Re: Re: [PATCH v1 3/3] ACPI: CPPC: Read CPC FFH counters in a single IPI

[Vanshidhar Konda]

On Thu, Feb 29, 2024 at 06:32:59PM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 29, 2024 at 5:25 PM Vanshidhar Konda
> <vanshikonda@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > The CPPC driver reads delivered and reference counters using cpc_read
> > one at a time. This leads to inaccurate measurement of CPU frequency
> > discussed in [1]. If the firmware indicates that both the registers are
> > in the FFH interface the kernel can read the registers together in a
> > single IPI. This has two benefits:
> > 1. Reduces the number of IPIs needed to read the two registers
> > 2. The two registers will be read in close proximity resulting in more
> >    accurate CPU frequency measurement
> >
> > [1]: https://lore.kernel.org/all/20230328193846.8757-1-yang@xxxxxxxxxxxxxxxxxxxxxx/
> >
> > Signed-off-by: Vanshidhar Konda <vanshikonda@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  arch/arm64/kernel/topology.c | 37 ++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/cppc_acpi.c     | 32 +++++++++++++++++++++++++++----
> >  include/acpi/cppc_acpi.h     | 13 +++++++++++++
> >  3 files changed, 78 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 8905eb0c681f..8207565f43ee 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -421,6 +421,43 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> >         return ret;
> >  }
> >
> > +static void cpc_update_freq_counters(void *info)
> > +{
> > +       update_freq_counters_refs();
> > +}
> > +
> > +int cpc_read_regs_ffh(int cpu, struct ffh_cpc_reg_values *ffh_regs)
> > +{
> > +       struct amu_counters *ctrs = per_cpu_ptr(&cpu_samples, cpu);
> > +       int idx;
> > +
> > +       if (!cpc_ffh_supported() || !freq_counters_valid(cpu))
> > +               return -EOPNOTSUPP;
> > +
> > +       if (WARN_ON_ONCE(irqs_disabled()))
> > +               return -EPERM;
> > +
> > +       if (!idle_cpu(cpu))
> > +               smp_call_function_single(cpu, cpc_update_freq_counters, NULL, 1);
> > +
> > +       for (idx = 0; idx < MAX_NUM_CPC_REGS_FFH; idx++) {
> > +
> > +               if (!ffh_regs->regs[idx].reg)
> > +                       continue;
> > +
> > +               switch ((u64)(ffh_regs->regs[idx].reg->address)) {
> > +               case 0x0:
> > +                       ffh_regs->regs[idx].value = ctrs->core_cnt;
> > +                       break;
> > +               case 0x1:
> > +                       ffh_regs->regs[idx].value = ctrs->const_cnt;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
> >  {
> >         return -EOPNOTSUPP;
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index d155a86a8614..55ffb1915e4f 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -113,6 +113,10 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
> >                                 (cpc)->cpc_entry.reg.space_id ==        \
> >                                 ACPI_ADR_SPACE_SYSTEM_IO)
> >
> > +#define CPC_IN_FFH(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&            \
> > +                               (cpc)->cpc_entry.reg.space_id ==        \
> > +                               ACPI_ADR_SPACE_FIXED_HARDWARE)
> > +
> >  /* Evaluates to True if reg is a NULL register descriptor */
> >  #define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
> >                                 (reg)->address == 0 &&                  \
> > @@ -974,6 +978,11 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
> >         return -ENOTSUPP;
> >  }
> >
> > +int __weak cpc_read_regs_ffh(int cpu, struct ffh_cpc_reg_values *regs)
> > +{
> > +       return -ENOTSUPP;
> > +}
>
> This might return a bool value.
>
> Is there any case in which the caller would handle different error
> codes differently?
I kept this similar to cpc_read_ffh(). I didn't see any usage of the error
codes. The return value of cpc_read_ffh() is returned only from cpc_read(),
but I didn't see anyone consuming the return value of cpc_read().
Additionally, checkpatch complains about using -ENOTSUPP and suggests
replacing it with -EOPNOTSUPP. Does it make sense to update
cpc_read/write_ffh() as well to switch to return type bool?

> > +
> >  /*
> >   * Since cpc_read and cpc_write are called while holding pcc_lock, it should be
> >   * as fast as possible. We have already mapped the PCC subspace during init, so
> > @@ -1317,7 +1326,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> >         int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> >         struct cppc_pcc_data *pcc_ss_data = NULL;
> >         u64 delivered, reference, ref_perf, ctr_wrap_time;
> > -       int ret = 0, regs_in_pcc = 0;
> > +       int ret = 0, regs_in_pcc = 0, regs_read_in_ffh = 0;
>
> Please use bool as the type for boolean variables.
Thanks for pointing that out. I'll do that for the next version.

Thanks,
Vanshi

> >         if (!cpc_desc) {
> >                 pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> > @@ -1353,8 +1362,23 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> >                 }
> >         }
> >
> > -       cpc_read(cpunum, delivered_reg, &delivered);
> > -       cpc_read(cpunum, reference_reg, &reference);
> > +       if (CPC_IN_FFH(delivered_reg) && CPC_IN_FFH(reference_reg)) {
> > +               struct ffh_cpc_reg_values ffh_regs;
> > +
> > +               ffh_regs.regs[0].reg = &(delivered_reg->cpc_entry.reg);
> > +               ffh_regs.regs[1].reg = &(reference_reg->cpc_entry.reg);
> > +               ret = cpc_read_regs_ffh(cpunum, &ffh_regs);
> > +               if (!ret) {
>
> If cpc_read_regs_ffh() returned 'true' on success, the above could be written as
>
> if (cpc_read_regs_ffh(cpunum, &ffh_regs)) {
>
> > +                       delivered = ffh_regs.regs[0].value;
> > +                       reference = ffh_regs.regs[1].value;
> > +                       regs_read_in_ffh = 1;
> > +               }
> > +       }
> > +
> > +       if (!regs_read_in_ffh) {
> > +               cpc_read(cpunum, delivered_reg, &delivered);
> > +               cpc_read(cpunum, reference_reg, &reference);
> > +       }
> >         cpc_read(cpunum, ref_perf_reg, &ref_perf);
> >
> >         /*
> > @@ -1366,7 +1390,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> >         if (CPC_SUPPORTED(ctr_wrap_reg))
> >                 cpc_read(cpunum, ctr_wrap_reg, &ctr_wrap_time);
> >
> > -       if (!delivered || !reference || !ref_perf) {
> > +       if (!delivered || !reference || !ref_perf) {
> >                 ret = -EFAULT;
> >                 goto out_err;
> >         }
> > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> > index 3a0995f8bce8..0da614a50edd 100644
> > --- a/include/acpi/cppc_acpi.h
> > +++ b/include/acpi/cppc_acpi.h
> > @@ -137,6 +137,18 @@ struct cppc_cpudata {
> >  };
> >
> >  #ifdef CONFIG_ACPI_CPPC_LIB
> > +
> > +#define MAX_NUM_CPC_REGS_FFH 2
> > +
> > +struct ffh_cpc_reg {
> > +       struct cpc_reg *reg;
> > +       u64 value;
> > +};
> > +
> > +struct ffh_cpc_reg_values {
> > +       struct ffh_cpc_reg regs[MAX_NUM_CPC_REGS_FFH];
> > +};
> > +
> >  extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
> >  extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
> >  extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
> > @@ -153,6 +165,7 @@ extern unsigned int cppc_get_transition_latency(int cpu);
> >  extern bool cpc_ffh_supported(void);
> >  extern bool cpc_supported_by_cpu(void);
> >  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> > +extern int cpc_read_regs_ffh(int cpu, struct ffh_cpc_reg_values *regs);
> >  extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> >  extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
> >  extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
> > --