[v4,0/4] thermal: Add support of multiple sensors

Message

Alexandre Bailon June 13, 2024, 1:24 p.m. UTC

Following this comment [1], this updates thermal_of to support multiple
sensors.

This series intends to add support of thermal aggregation.
One use case for it is using the IPA in the case we have
multiple sensors for one performance domain.

This has been tested on the mt8195 using s-tui.
To test and validate, we heat up the CPU and the heat sink.
At some point, we run benchmark tests with different configurations:
- Mediatek kernel (IPA + their own thermal aggregation)
- Mainline kernel
- Mainline kernel with IPA and aggregation enabled
With the IPA and the aggregation enabled, we get the best performances
with the most stable CPU temperature.

The aggregation is configured and enabled using device tree.
One thermal zone has to be created with a list of sensors.
It will take care of registering a thermal zone for each sensors.
The cooling device will only be registered with the aggregating thermal
zone.

There are still something important missing: a way to check that all
aggregated sensors are part of the same performance domain.
So far, I don't see how this should be done. Some recommendations would be
appreciated.

Changes in v2:
- Rebased on 6.7
- Separated generic multi sensor and dt specific code
- Simplified the code
- Drop min / max and only do weighted average (seems more adequate for IPA)

Changes in v3:
- Rebased on 6.9
- Reworked the way to register a multi sensor thermal zone
  - Only one thermal zone to define in device tree
- Max has been re-added
- Enabled it on mt8195

Changes in v4:
- Rebased on lastest master (fixed the build issue)
- Dropped the average since I don't have any usecase for it

[1]: https://patchwork.kernel.org/comment/24723927/

Alexandre Bailon (4):
  dt-bindings: thermal: Restore the thermal-sensors property
  thermal: Add support of multi sensors to thermal_core
  thermal: Add support of multi sensors to thermal_of
  ARM64: mt8195: Use thermal aggregation for big and little cpu

 .../bindings/thermal/thermal-zones.yaml       |   5 +-
 arch/arm64/boot/dts/mediatek/mt8195.dtsi      | 212 ++-----------
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/thermal_core.h                |  15 +
 drivers/thermal/thermal_multi.c               | 288 ++++++++++++++++++
 drivers/thermal/thermal_of.c                  | 250 ++++++++++++++-
 include/uapi/linux/thermal.h                  |   5 +
 7 files changed, 579 insertions(+), 197 deletions(-)
 create mode 100644 drivers/thermal/thermal_multi.c

Comments

Daniel Lezcano July 30, 2024, 12:58 p.m. UTC | #1

Hi Alexandre,

thanks for your series and my apologizes for taking a so long time to 
review.

I went through the series and at the first glance I'm not sure we want 
to add all the multi specific code in a separate file.

IMO, there is a preparatory work by changing the functions:

thermal_zone_device_register_with_trips() and 
thermal_zone_device_register_tripless()

where we group and move the functions parameters to the 
thermal_zone_device_param.

Then we can add a num_ops field which is will default to zero.

With that we should have put a foundation for multiple ops, so multiple 
sensors.

On 13/06/2024 15:24, Alexandre Bailon wrote:
> Following this comment [1], this updates thermal_of to support multiple
> sensors.
> 
> This series intends to add support of thermal aggregation.
> One use case for it is using the IPA in the case we have
> multiple sensors for one performance domain.
> 
> This has been tested on the mt8195 using s-tui.
> To test and validate, we heat up the CPU and the heat sink.
> At some point, we run benchmark tests with different configurations:
> - Mediatek kernel (IPA + their own thermal aggregation)
> - Mainline kernel
> - Mainline kernel with IPA and aggregation enabled
> With the IPA and the aggregation enabled, we get the best performances
> with the most stable CPU temperature.
> 
> The aggregation is configured and enabled using device tree.
> One thermal zone has to be created with a list of sensors.
> It will take care of registering a thermal zone for each sensors.
> The cooling device will only be registered with the aggregating thermal
> zone.
> 
> There are still something important missing: a way to check that all
> aggregated sensors are part of the same performance domain.
> So far, I don't see how this should be done. Some recommendations would be
> appreciated.
> 
> Changes in v2:
> - Rebased on 6.7
> - Separated generic multi sensor and dt specific code
> - Simplified the code
> - Drop min / max and only do weighted average (seems more adequate for IPA)
> 
> Changes in v3:
> - Rebased on 6.9
> - Reworked the way to register a multi sensor thermal zone
>    - Only one thermal zone to define in device tree
> - Max has been re-added
> - Enabled it on mt8195
> 
> Changes in v4:
> - Rebased on lastest master (fixed the build issue)
> - Dropped the average since I don't have any usecase for it
> 
> [1]: https://patchwork.kernel.org/comment/24723927/
> 
> Alexandre Bailon (4):
>    dt-bindings: thermal: Restore the thermal-sensors property
>    thermal: Add support of multi sensors to thermal_core
>    thermal: Add support of multi sensors to thermal_of
>    ARM64: mt8195: Use thermal aggregation for big and little cpu
> 
>   .../bindings/thermal/thermal-zones.yaml       |   5 +-
>   arch/arm64/boot/dts/mediatek/mt8195.dtsi      | 212 ++-----------
>   drivers/thermal/Makefile                      |   1 +
>   drivers/thermal/thermal_core.h                |  15 +
>   drivers/thermal/thermal_multi.c               | 288 ++++++++++++++++++
>   drivers/thermal/thermal_of.c                  | 250 ++++++++++++++-
>   include/uapi/linux/thermal.h                  |   5 +
>   7 files changed, 579 insertions(+), 197 deletions(-)
>   create mode 100644 drivers/thermal/thermal_multi.c
>