Jean Pihet <[email protected]> writes:
> On Tue, May 1, 2012 at 7:27 PM, Kevin Hilman <[email protected]> wrote:
>> Jean Pihet <[email protected]> writes:
>>
>>> HI Kevin, Grazvydas,
>>>
>>> On Tue, Apr 24, 2012 at 4:29 PM, Kevin Hilman <[email protected]> wrote:
>>>> Jean Pihet <[email protected]> writes:
>>>>
>>>>> Hi Grazvydas, Kevin,
>>>>>
>>>>> I did some gather some performance measurements and statistics using
>>>>> custom tracepoints in __omap3_enter_idle.
>>> I posted the patches for the power domains registers cache, cf.
>>> http://marc.info/?l=linux-omap&m=133587781712039&w=2.
>>>
>>>>> All the details are at
>>>>> http://www.omappedia.org/wiki/Power_Management_Device_Latencies_Measurement#C1_performance_problem:_analysis
>>> I updated the page with the measurements results with Kevin's patches
>>> and the registers cache patches.
>>>
>>> The results are showing that:
>>> - the registers cache optimizes the low power mode transitions, but is
>>> not sufficient to obtain a big gain. A few unused domains are
>>> transitioning, which causes a big penalty in the idle path.
>>
>> PER is the one that seems to be causing the most latency.
>>
>> Can you try do your measurements using hack below which makes sure that
>> PER isn't any deeper than CORE?
>
> Indeed your patch brings significant improvements, cf. wiki page at
> http://www.omappedia.org/wiki/Power_Management_Device_Latencies_Measurement#C1_performance_problem:_analysis
> for detailed information.
> Here below is the reworked patch, more suited for inclusion in mainline [1]
>
> I have another optimisation -in proof of concept state- that brings
> another significant improvement. It is about allowing/disabling idle
> for only 1 clkdm in a pwrdm and not iterate through all the clkdms.
> This still needs some rework though. Cf. patch [2]
That should work since disabling idle for any clkdm will have the same
effect. Can you send this as a separate patch with a descriptive
changelog.
Kevin
> Patches [1] and [2] on top of the registers cache and the
> optimisations in pre/post_transition bring the performance close to
> the performance for the non cpuidle case (3.0MB/s compared to 3.1MB/s
> on Beagleboard).
>
> What do you think?
>
> Regards,
> Jean
>
> ---
> [1]
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index e406d7b..572b605 100644
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -279,32 +279,36 @@ static int omap3_enter_idle_bm(struct cpuidle_device
> *dev,
> int ret;
>
> /*
> - * Prevent idle completely if CAM is active.
> + * Use only C1 if CAM is active.
> * CAM does not have wakeup capability in OMAP3.
> */
> - if (pwrdm_read_func_pwrst(cam_pd) == PWRDM_FUNC_PWRST_ON) {
> + if (pwrdm_read_func_pwrst(cam_pd) == PWRDM_FUNC_PWRST_ON)
> new_state_idx = drv->safe_state_index;
> - goto select_state;
> - }
> -
> - new_state_idx = next_valid_state(dev, drv, index);
> + else
> + new_state_idx = next_valid_state(dev, drv, index);
>
> - /*
> - * Prevent PER off if CORE is not in retention or off as this
> - * would disable PER wakeups completely.
> - */
> + /* Program PER state */
> cx = cpuidle_get_statedata(&dev->states_usage[new_state_idx]);
> core_next_state = cx->core_state;
> - per_next_state = per_saved_state = pwrdm_read_next_func_pwrst(per_pd);
> - if ((per_next_state == PWRDM_FUNC_PWRST_OFF) &&
> - (core_next_state > PWRDM_FUNC_PWRST_CSWR))
> - per_next_state = PWRDM_FUNC_PWRST_CSWR;
> + if (new_state_idx == 0) {
> + /* In C1 do not allow PER state lower than CORE state */
> + per_next_state = core_next_state;
> + } else {
> + /*
> + * Prevent PER off if CORE is not in RETention or OFF as this
> + * would disable PER wakeups completely.
> + */
> + per_next_state = per_saved_state =
> + pwrdm_read_next_func_pwrst(per_pd);
> + if ((per_next_state == PWRDM_FUNC_PWRST_OFF) &&
> + (core_next_state > PWRDM_FUNC_PWRST_CSWR))
> + per_next_state = PWRDM_FUNC_PWRST_CSWR;
> + }
>
> /* Are we changing PER target state? */
> if (per_next_state != per_saved_state)
> omap_set_pwrdm_state(per_pd, per_next_state);
>
> -select_state:
> ret = omap3_enter_idle(dev, drv, new_state_idx);
>
> /* Restore original PER state if it was modified */
> @@ -390,7 +394,6 @@ int __init omap3_idle_init(void)
>
> /* C1 . MPU WFI + Core active */
> _fill_cstate(drv, 0, "MPU ON + CORE ON");
> - (&drv->states[0])->enter = omap3_enter_idle;
> drv->safe_state_index = 0;
> cx = _fill_cstate_usage(dev, 0);
> cx->valid = 1; /* C1 is always valid */
>
> [2]
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index e406d7b..6aa3c75 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -118,8 +118,10 @@ static int __omap3_enter_idle(struct cpuidle_device *dev,
>
> /* Deny idle for C1 */
> if (index == 0) {
> - pwrdm_for_each_clkdm(mpu_pd, _cpuidle_deny_idle);
> - pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle);
> + //pwrdm_for_each_clkdm(mpu_pd, _cpuidle_deny_idle);
> + clkdm_deny_idle(mpu_pd->pwrdm_clkdms[0]);
> + //pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle);
> + clkdm_deny_idle(core_pd->pwrdm_clkdms[0]);
> }
>
> /*
> @@ -141,8 +143,10 @@ static int __omap3_enter_idle(struct cpuidle_device *dev,
>
> /* Re-allow idle for C1 */
> if (index == 0) {
> - pwrdm_for_each_clkdm(mpu_pd, _cpuidle_allow_idle);
> - pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
> + //pwrdm_for_each_clkdm(mpu_pd, _cpuidle_allow_idle);
> + clkdm_allow_idle(mpu_pd->pwrdm_clkdms[0]);
> + //pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
> + clkdm_allow_idle(core_pd->pwrdm_clkdms[0]);
> }
>
> return_sleep_time:
>
>>
>> Kevin
>>
>> From bb2f67ed93dc83c645080e293d315d383c23c0c6 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <[email protected]>
>> Date: Mon, 16 Apr 2012 17:53:14 -0700
>> Subject: [PATCH] cpuidle34xx: per follows core, C1 use _bm
>>
>> ---
>> arch/arm/mach-omap2/cpuidle34xx.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> b/arch/arm/mach-omap2/cpuidle34xx.c
>> index 374708d..00400ad 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -278,9 +278,11 @@ static int omap3_enter_idle_bm(struct cpuidle_device
>> *dev,
>> cx = cpuidle_get_statedata(&dev->states_usage[index]);
>> core_next_state = cx->core_state;
>> per_next_state = per_saved_state = pwrdm_read_next_pwrst(per_pd);
>> - if ((per_next_state == PWRDM_POWER_OFF) &&
>> - (core_next_state > PWRDM_POWER_RET))
>> - per_next_state = PWRDM_POWER_RET;
>> + /* if ((per_next_state == PWRDM_POWER_OFF) && */
>> + /* (core_next_state > PWRDM_POWER_RET)) */
>> + /* per_next_state = PWRDM_POWER_RET; */
>> + if (per_next_state < core_next_state)
>> + per_next_state = core_next_state;
>>
>> /* Are we changing PER target state? */
>> if (per_next_state != per_saved_state)
>> @@ -374,7 +376,6 @@ int __init omap3_idle_init(void)
>>
>> /* C1 . MPU WFI + Core active */
>> _fill_cstate(drv, 0, "MPU ON + CORE ON");
>> - (&drv->states[0])->enter = omap3_enter_idle;
>> drv->safe_state_index = 0;
>> cx = _fill_cstate_usage(dev, 0);
>> cx->valid = 1; /* C1 is always valid */
>> --
>> 1.7.9.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
