Re: PM related performance degradation on OMAP3

[Jean Pihet <jean.pihet@newoldbits.com>]

Hi Kevin,

On Mon, May 7, 2012 at 7:31 PM, Kevin Hilman <khilman@ti.com> wrote:
> Jean Pihet <jean.pihet@newoldbits.com> writes:
>
>> On Tue, May 1, 2012 at 7:27 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> Jean Pihet <jean.pihet@newoldbits.com> writes:
>>>
>>>> HI Kevin, Grazvydas,
>>>>
>>>> On Tue, Apr 24, 2012 at 4:29 PM, Kevin Hilman <khilman@ti.com> wrote:
>>>>> Jean Pihet <jean.pihet@newoldbits.com> 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.
I just sent 2 patches which optimize the C1 state latency:
 . [PATCH 1/2] ARM: OMAP3: PM: cpuidle: optimize the PER latency in C1 state
 . [PATCH 2/2] ARM: OMAP3: PM: cpuidle: optimize the clkdm idle
latency in C1 state

Note: those patches apply on top of your pre/post_transition
optimization patches.

The performance results are close to the !PM case (No IDLE, no
omap_sram_idle, all pwrdms to ON), i.e. 3.1MB/s on Beagleboard.
The wiki page update comes asap.

Regards,
Jean

>
> 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 <khilman@ti.com>
>>> 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 majordomo@vger.kernel.org
>>> 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 majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html