Re: [PATCH v4 08/18] PM: EM: Add update_power() callback for runtime modifications

[Rafael J. Wysocki]

On Fri, Sep 29, 2023 at 10:59 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
> On 9/26/23 19:59, Rafael J. Wysocki wrote:
> > On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> >>
> >> The Energy Model (EM) is going to support runtime modifications. This
> >> new callback would be used in the upcoming EM changes. The drivers
> >> or frameworks which want to modify the EM have to implement the
> >> update_power() callback.
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> >> ---
> >>   include/linux/energy_model.h | 22 ++++++++++++++++++++++
> >>   1 file changed, 22 insertions(+)
> >>
> >> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> >> index d236e08e80dc..546dee90f716 100644
> >> --- a/include/linux/energy_model.h
> >> +++ b/include/linux/energy_model.h
> >> @@ -168,6 +168,26 @@ struct em_data_callback {
> >>           */
> >>          int (*get_cost)(struct device *dev, unsigned long freq,
> >>                          unsigned long *cost);
> >> +
> >> +       /**
> >> +        * update_power() - Provide new power at the given performance state of
> >> +        *              a device
> >
> > The meaning of the above is unclear to me.
>
> I can try to rephrase this a bit:
> ' Provide a new power value for the device at the given frequency. This
> allows to reflect changed power profile in runtime.'

Maybe "Estimate power for a given device frequency"

> >
> >> +        * @dev         : Device for which we do this operation (can be a CPU)
> >
> > It is unclear what "we" means in this context.  Maybe say "Target
> > device (can be a CPU)"?
>
> That sounds better indeed.
>
> >
> >> +        * @freq        : Frequency at the performance state in kHz
> >
> > What performance state does this refer to?  And the frequency of what?
>
> We just call the entry in EM the 'performance state' (so frequency and
> power). I will rephrase this as well:
> 'Frequency of the @dev expressed in kHz'

Or "Device frequency for which to estimate power"?

> >
> >> +        * @power       : New power value at the performance state
> >> +        *              (modified)
> >
> > Similarly unclear as the above.
>
> OK, it can be re-written as:
> 'Power value of the @dev at the given @freq (modified)'

This is not a power value, but a return pointer AFAICS.  So "location
to store the return power value".

> >
> >> +        * @priv        : Pointer to private data useful for tracking context
> >> +        *              during runtime modifications of EM.
> >
> > Who's going to set this pointer and use this data?
>
> The driver or kernel module, which is aware about thermal events. Those
> events might be coming from FW to kernel, but we need to maintain
> the same 'context' for all OPPs when we start the EM update.
>
> This 'priv' field is used for passing the 'context' back to the
> caller, so the caller can consistently the update. The update might
> be with some math formula which multiplies the power by some factor
> value (based on thermal model and current temperature).

I would say "Additional data for the callback function, provided by
the entity setting the callback pointer".

> >
> >> +        *
> >> +        * The update_power() is used by runtime modifiable EM. It aims to
> >
> > I would drop "The" from the above.
>
> OK
>
> >
> >> +        * provide updated power value for a given frequency, which is stored
> >> +        * in the performance state.
> >
> > A given frequency of what and the performance state of what does this refer to?
>
> I will change that and add the '@dev' word to make this more precised.

That would help.  Overall, I would say "is used by ... to obtain a new
power value for a given frequency of @dev".

>
> 'update_power() is used by runtime modifiable EM. It aims to update the
> @dev EM power values for all registered frequencies.'