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

[Lukasz Luba]

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.'

> > +        * @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'

> > +        * @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)'

> > +        * @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).

> > +        *
> > +        * 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.


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