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

  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@arm.com>
---
 include/linux/energy_model.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
  

On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> 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@arm.com>
> ---
>  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.

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

> +        * @freq        : Frequency at the performance state in kHz

What performance state does this refer to?  And the frequency of what?

> +        * @power       : New power value at the performance state
> +        *              (modified)

Similarly unclear as the above.

> +        * @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 update_power() is used by runtime modifiable EM. It aims to

I would drop "The" from the above.

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

> + The power value provided by this callback
> +        * should fit in the [0, EM_MAX_POWER] range.
> +        *
> +        * Return 0 on success, or appropriate error value in case of failure.
> +        */
> +       int (*update_power)(struct device *dev, unsigned long freq,
> +                           unsigned long *power, void *priv);
>  };
>  #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) ((em_cb).active_power = cb)
>  #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb)     \
> @@ -175,6 +195,7 @@ struct em_data_callback {
>           .get_cost = _cost_cb }
>  #define EM_DATA_CB(_active_power_cb)                   \
>                 EM_ADV_DATA_CB(_active_power_cb, NULL)
> +#define EM_UPDATE_CB(_update_power_cb) { .update_power = &_update_power_cb }
>
>  struct em_perf_domain *em_cpu_get(int cpu);
>  struct em_perf_domain *em_pd_get(struct device *dev);
> @@ -331,6 +352,7 @@ struct em_data_callback {};
>  #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb) { }
>  #define EM_DATA_CB(_active_power_cb) { }
>  #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) do { } while (0)
> +#define EM_UPDATE_CB(_update_cb) { }
>
>  static inline
>  int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
> --
  

On 9/26/23 19:59, Rafael J. Wysocki wrote:
> On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> 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@arm.com>
>> ---
>>   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.'
  

On Fri, Sep 29, 2023 at 10:59 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> On 9/26/23 19:59, Rafael J. Wysocki wrote:
> > On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> 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@arm.com>
> >> ---
> >>   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.'