On Tue, Feb 07, 2023 at 01:38:08PM +0100, Daniel Lezcano wrote: > On 07/02/2023 13:18, Thierry Reding wrote: > > On Mon, Feb 06, 2023 at 03:50:22PM +0100, Daniel Lezcano wrote: > > > > > > Hi Thierry, > > > > > > did you have the time to look at the get_thermal_instance() removal ? > > > > > > > > > On 26/01/2023 13:55, Thierry Reding wrote: > > > > > > > [ 12.354091] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp > > > > [ 12.379009] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC > > > > [ 12.388882] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500 > > > > [ 12.401007] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC > > > > [ 12.471041] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC > > > > [ 12.482852] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000 > > > > [ 12.482860] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC > > > > [ 12.485357] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC > > > > [ 12.501774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC > > > > > > > > and after these changes, it turns into: > > > > > > > > [ 12.447113] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp > > > > [ 12.472300] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC > > > > [ 12.481789] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500 > > > > [ 12.495447] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC > > > > [ 12.496514] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC > > > > [ 12.510353] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000 > > > > [ 12.526856] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC > > > > [ 12.528774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC > > > > [ 12.569352] tegra_soctherm 700e2000.thermal-sensor: programming throttle for pll to 103000 > > > > [ 12.577635] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when pll reaches 103000 mC > > > > [ 12.590952] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC > > > > [ 12.600783] tegra_soctherm 700e2000.thermal-sensor: programming throttle for mem to 103000 > > > > [ 12.609204] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when mem reaches 103000 mC > > > > > > > > The "programming throttle ..." messages are something I've added locally > > > > to trace what gets called. So it looks like for "pll" and "mem" thermal > > > > zones, we now program trip points whereas we previously didn't. > > > > My diagnosis above wasn't entirely correct. We're not actually skipping > > trip point programming for PLL and MEM thermal zones in the current > > code. Instead, we skip throttle programming. As far as I can tell this > > is a mechanism built into ACTMON to allow it to automatically throttle > > when a zone reaches a certain temperature. > > > > This is modelled as a cooling device, but internally it's actually done > > automatically, which is why we have this code that programs the throttle > > at driver probe time, rather than the on-demand programming that typical > > cooling device would do (such as a fan). > > > > The reason why we have get_thermal_instance() here is to check if this > > built-in cooling device has been configured for the "hot" trip point. If > > not, we don't want the throttle programming to happen. This adds the > > added flexibility of explicitly disabling the automatic throttling by > > ACTMON and using another cooling device (or none at all) if that's what > > is needed. > > > > Dropping just the call to get_thermal_instance() and relying on the > > find_throttle_cfg_by_name() function will always return a valid throttle > > configuration. This is slightly obfuscated because of this: > > > > cdev = ts->throt_cfgs[i].cdev; > > if (get_thermal_instance(tz, cdev, trip_id)) > > stc = find_throttle_cfg_by_name(ts, cdev->type); > > > > As far as I can tell this will always return &ts->throt_cfgs[i], so the > > find_throttle_cfg_by_name() call is a bit redundant here. I'll look into > > fixing that. > > > > In any case, the important thing is that it would always find a valid > > throttle configuration and therefore program the throttle, even if we > > may not want to. > > Why not rely on the thermal framework mechanism to set the hwtrpis ? > > thermal_zone_device_register() calls thermal_zone_device_update(). This one > calls thermal_zone_set_trips() which programs the hardware trip point. > > When we suspend/resume, the PM notifiers are calling > thermal_zone_device_update() which in turn sets the hw trip points. > > May be I'm missing something but isn't enough for the sensor ? These aren't actually trip points getting programmed, but rather the built-in throttling mechanism. That said, it might be possible to append that programming to the driver's ->set_trips() implementation. I'll look into that. Thanks for the suggestion, Thierry > > > > Possibly we could work around that by removing this fiddly special case > > and instead add a new callback for the cooling devices that can be run > > when they are bound to a thermal zone. This would allow the throttle > > programming to be initiated from within the thermal core rather than > > "bolted on" like it is now and should allow us to achieve the same > > effect but without calling into get_thermal_instance(). > > > > I'll try and prototype this, but feel free to suggest anything better if > > you can think of something. > > > > Thierry > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
Attachment:
signature.asc
Description: PGP signature
