From: Lukasz Luba <lukasz.luba@arm.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>,
Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
LKML <linux-kernel@vger.kernel.org>,
Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v1 0/3] thermal/debugfs: Fix and clean up trip point statistics updates
Date: Tue, 23 Apr 2024 14:38:50 +0100 [thread overview]
Message-ID: <95c30582-4fba-4e2d-8cc1-776b3e5507b7@arm.com> (raw)
In-Reply-To: <CAJZ5v0j5Ja9-vuC9ve9Li=UxATcBtTmdMdhMS1g993XBe1DVqw@mail.gmail.com>
On 4/23/24 13:26, Rafael J. Wysocki wrote:
> On Mon, Apr 22, 2024 at 6:12 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 22/04/2024 17:48, Rafael J. Wysocki wrote:
>>> On Mon, Apr 22, 2024 at 5:34 PM Daniel Lezcano
>>
>> [ ... ]
>>
>>>> or we should expect at least the residency to be showed even if the
>>>> mitigation state is not closed ?
>>>
>>> Well, in fact the device has already been in that state for some time
>>> and the mitigation can continue for a while.
>>
>> Yes, but when to update the residency time ?
>>
>> When we cross a trip point point ?
>>
>> or
>>
>> When we read the information ?
>>
>> The former is what we are currently doing AFAIR
>
> Not really.
>
> Records are added by thermal_debug_cdev_state_update() which only runs
> when __thermal_cdev_update() is called, ie. from governors.
>
> Moreover, it assumes the initial state to be 0 and checks if the new
> state is equal to the current one before doing anything else, so it
> will not make a record for the 0 state until the first transition.
Correct, AFAIKS.
>
>> and the latter must add the delta between the last update and the current time for the current
>> state, right ?
>
> Yes, and it is doing this already AFAICS.
>
> The problem is that it only creates a record for the old state, so if
> the new one is seen for the first time, there will be no record for it
> until it changes to some other state.
Exactly, it's not totally wrong what we have now, just some missing part
that needs to be added in the code, while we are counting/updating
these stats.
>
> The appended patch (modulo GMail-induced white space breakage) should
> help with this, but the initial state handling needs to be addressed
> separately.
>
> ---
> drivers/thermal/thermal_debugfs.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -433,6 +433,14 @@ void thermal_debug_cdev_state_update(con
> }
>
> cdev_dbg->current_state = new_state;
> +
> + /*
> + * Create a record for the new state if it is not there, so its
> + * duration will be printed by cdev_dt_seq_show() as expected if it
> + * runs before the next state transition.
> + */
> + thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg->durations,
> new_state);
> +
> transition = (old_state << 16) | new_state;
>
> /*
Yes, something like this should do the trick. We will get the record
entry in the list, so we at least enter the list loop in the
cdev_dt_seq_show().
prev parent reply other threads:[~2024-04-23 13:38 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 13:07 [PATCH v1 0/3] thermal/debugfs: Fix and clean up trip point statistics updates Rafael J. Wysocki
2024-04-17 13:09 ` [PATCH v1 1/3] thermal/debugfs: Avoid excessive updates of trip point statistics Rafael J. Wysocki
2024-04-22 11:14 ` Lukasz Luba
2024-04-23 15:54 ` Daniel Lezcano
2024-04-17 13:10 ` [PATCH v1 2/3] thermal/debugfs: Clean up thermal_debug_update_temp() Rafael J. Wysocki
2024-04-22 11:15 ` Lukasz Luba
2024-04-23 15:30 ` Daniel Lezcano
2024-04-17 13:11 ` [PATCH v1 3/3] thermal/debugfs: Rename thermal_debug_update_temp() to thermal_debug_update_trip_stats() Rafael J. Wysocki
2024-04-22 11:15 ` Lukasz Luba
2024-04-23 15:30 ` Daniel Lezcano
2024-04-22 11:37 ` [PATCH v1 0/3] thermal/debugfs: Fix and clean up trip point statistics updates Lukasz Luba
2024-04-22 14:20 ` Rafael J. Wysocki
2024-04-22 15:34 ` Daniel Lezcano
2024-04-22 15:48 ` Rafael J. Wysocki
2024-04-22 16:12 ` Daniel Lezcano
2024-04-23 12:26 ` Rafael J. Wysocki
2024-04-23 13:38 ` Lukasz Luba [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=95c30582-4fba-4e2d-8cc1-776b3e5507b7@arm.com \
--to=lukasz.luba@arm.com \
--cc=daniel.lezcano@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).