Hi Arvid, On 10/7/22 14:51, Arvid Norlander wrote: > On 2022-10-07 13:22, Hans de Goede wrote: >> Hi Arvid, >> >> On 10/6/22 23:12, Arvid Norlander wrote: >>> Hi Hans, >>> >>> Two things: >>> * I have discovered that reading the fan RPM in toshiba_acpi is slow, >>> around 50 ms. I didn't notice it at first, but after adding some more >>> sensors I found (current and voltage for AC and battery) it started to >>> make running "sensors" visibly slow. >>> >>> I don't know what proper fix to this would be. Feel free to revert the >>> fan RPM for now if it is not acceptable for reading sensors to block for >>> ~50 ms (as opposed to the 100s of micro-seconds that other sensors such >>> as coretemp and acpitz take to read on that laptop). >> >> Hmm, so apparently the single ACPI call this ends up making takes along time. >> >> I wonder what happens with the CPU load if you cat the file from >> a "while true; do cat /sys/class/hwmon/...; done" loop in the shell. >> >> With some luck most of that 50 ms is sleeping, so we won't see say >> 25% load on a quad core CPU (so 100% load on 1 core) in "top". > > After testing, thankfully it seems to be the case that it is mostly > sleeping! That is good, still unfortunate that it takes so long though. > >> >> Regardless we also want any desktop-environment sensor applets which >> poll more then once/second to block on this all the time. What most hwmon >> drivers with slow HW access do is update the readings once per second >> and then return cached values for a second see e.g. : >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/f71882fg.c >> >> and then specifically the "bool valid;" and "unsigned long last_updated; /* In jiffies */" >> members of "struct f71882fg_data" and how those are used. >> >> I believe that keeping the fan rpm reading, which IMHO is the most >> useful one is fine when adding the caching; or alternatively you >> can submit a revert. > > I will try to look into this, hopefully this weekend, though I'm currently > down with a nasty cold, so I can't promise anything. > >> As for the AC + bat voltage/current can those be / are those retreived >> with the same tci_raw() call or do those require separate calls ? >> And if they require separate calls do those calls also all take 50 ms? > > These are separate: A shared one for AC current and AC voltage, and two > separate ones for battery current and battery voltage. There is also > more data in the AC reply one that I can't quite decode, but it seems to > be thermally related (but I have not managed to decode it as a temperature > that matches anything else). Annoyingly the windows software displays > almost everything in percentages on dial gauges without any units or even > scales, making actually figuring out the interpretations rather difficult > at times. > > For reading the power a call is can be issued to set the time resolution, > in powers of two between 1 and 16 (i.e. 1, 2, 4, 8 and 16 are valid values), > where 1 is slightly more than one new reading per second, while 16 is about > one reading every 20ish seconds. > > This suggests that the EC would be updating some periodic registers > internally and we are just querying them, however, these are still just as > slow to read as the fan. Hmm, I'm not sure what is the best thing to do here, if we end up making 4 calls of each 50 ms here, then that is going to block the caller for 200 ms which I guess might be just acceptable if we do it only once per x seconds, for some value of x... I guess you could use power_supply_is_system_supplied() to skip the AC readings when not charging, but that ends up calling _PSR on the ACPI AC device, which might also be slow. > >> >> The battery values should already be available in some form under >> /sys/class/power_supply/BAT* although you may only have the >> multiplied value of the 2 there in the form of energy_now. > > I do seem to have voltage_now and power_now. So current could be computed > by measuring the delta of energy_now over time and then using the voltage > to compute the current. > > However, the voltage as reported in /sys/class/power_supply/BAT1 does not > quite match the voltage I get from the HCI calls. The HCI call consistently > reads higher, but how much higher depends on if the AC is connected or not. > Also voltage_now seems to update rather more slowly than the HCI calls. > >> >> And the AC values are nice to have but not super interesting, >> so if they require another slow tci_raw() call then I'm not sure >> if they are worth adding. > > Yes they are slow, but they might be interesting in order to compute > system power usage while on AC (AC power - battery power) as it reports > the battery charging current while charging and the discharging current > while discharging. > > At the very least I will be documenting them when I get time. Yes that sounds like a good idea. I guess you could also add support for them in the driver, but then maybe activated with some "extended_hwmon" kernel-module option. Which then should default to off I guess? Regards, Hans
