"Pallipadi, Venkatesh" <venkatesh.pallipadi@xxxxxxxxx> writes: >>Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx> writes: >> >>> cpuidle accounts the idle time for the C-state it was trying >>to enter and >>> not to the actual state that the driver eventually entered. >>The driver may >>> select a different state than the one chosen by cpuidle due to >>> constraints like bus-mastering, etc. >>> >>> Change the time acounting code to look at the dev->last_state after >>> returning from target_state->enter(). Driver can modify >>dev->last_state >>> internally, inside the enter routine to reflect the actual C-state >>> entered. >>> >>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx> >>> >>> --- >>> drivers/cpuidle/cpuidle.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> Index: tip/drivers/cpuidle/cpuidle.c >>> =================================================================== >>> --- tip.orig/drivers/cpuidle/cpuidle.c 2008-09-23 >>13:52:59.000000000 -0700 >>> +++ tip/drivers/cpuidle/cpuidle.c 2008-09-23 >>14:22:43.000000000 -0700 >>> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void) >>> target_state = &dev->states[next_state]; >>> >>> /* enter the state and update stats */ >>> - dev->last_residency = target_state->enter(dev, target_state); >>> dev->last_state = target_state; >>> + dev->last_residency = target_state->enter(dev, target_state); >>> + if (dev->last_state) >>> + target_state = dev->last_state; >>> + >>> target_state->time += (unsigned long long)dev->last_residency; >>> target_state->usage++; >> >>Under what conditions would the enter hook set dev->last_sate to NULL? >>Having the check seems to indicate it's possilble. > > Sorry about the delayed response. > > Yes. I added the check after finding out that last_state can be > possibly NULL. That happens when the governor changes while one > core is in idle and also during CPU offline/online. OK, thanks for the clarification. >>A minor nit-pick... why not explicitly do the accounting using >>'last_state' like below. While functionally the same as above, this >>makes it makes it more explicit when reading the code that the >>accounting is done using 'last_state' and not 'target_state.' >> > > Yes. It is cleaner. But, we still have to check for last_state being NULL. Agreed. I've switched to your version and tested this on OMAP3 and it's working well. Tested-by: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> >> >> >>diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >>index 5ce07b5..c1294f5 100644 >>--- a/drivers/cpuidle/cpuidle.c >>+++ b/drivers/cpuidle/cpuidle.c >>@@ -67,10 +67,10 @@ static void cpuidle_idle_call(void) >> target_state = &dev->states[next_state]; >> >> /* enter the state and update stats */ >>- dev->last_residency = target_state->enter(dev, target_state); >> dev->last_state = target_state; >>- target_state->time += (unsigned long long)dev->last_residency; >>- target_state->usage++; >>+ dev->last_residency = target_state->enter(dev, target_state); >>+ dev->last_state->time += (unsigned long >>long)dev->last_residency; >>+ dev->last_state->usage++; >> >> /* give the governor an opportunity to reflect on the >>outcome */ >> if (cpuidle_curr_governor->reflect) >> > > Thanks, > Venki _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
