[Intel-gfx] [PATCH] drm/i915: Suspend resume timing optimization.

[Kumar, Shobhit]

On 12/09/2015 05:42 PM, Ville Syrjälä wrote:
> On Wed, Dec 09, 2015 at 04:25:41PM +0530, Kumar, Shobhit wrote:
>> On 12/08/2015 02:22 AM, Paulo Zanoni wrote:
>>> 2015-12-07 18:28 GMT-02:00  <abhay.kumar at intel.com>:
>>>> From: Abhay Kumar <abhay.kumar at intel.com>
>>>>
>>>> Moving 250ms from T12 timing to suspend path so that
>>>> resume path will be faster.
>>>
>>> Can you please elaborate more on your motivation for this patch? I'm a
>>> little confused. You're trying to make resume faster by making suspend
>>> slower? What are your main arguments for this?
>>>
>>
>> Actually the display resume time is currently roughly around ~700ms for
>> eDP. The panel power sequence as per spec needs to be minimum 510ms . So
>> the t11_12 time is 600ms in our code. Question is how to optimize this.
>> What Abhay has tried is to move the wait for panel_power_cycle_delay in
>> the suspend path rather then in resume path when we check if panel has
>> power. Since this is jiffied based we end up waiting while going down
>> and by the time we come back jiffies have already expired, giving us
>> optimization in resume path. Will this work. Is it violates the spec in
>> any way ? Any other suggestions ?
>
> I'm thinking this should work. However, it seems unlikely we would
> really need any wait during suspend or resume since the panel will
> likely be off for a longer period anyway while the machine is
> suspended. So I wonder if we could look at the wall clock instead
> during suspend? Eg. sample it during susend and resume,
> and adjust last_power_cycle jiffies based on the difference.

Yes, but infact I found another thing that in edp_panel_on we just 
blindly call this wait_for_panel_cycle. We can check there if any PPS is 
in progress at that time via PP_STAT, and if no PPS (mainly power down 
seq) we should just skip the wait. My understanding of the 
panel_power_cycle_delay is that if we power it off, we should not try to 
turn on within 500ms.

>
>>
>>>>
>>>> Signed-off-by: Abhay Kumar <abhay.kumar at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 7f618cf..2679c9e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>>>
>>> Funcion intel_ddi_post_disable() doesn't only run on suspend
>>> situations, yet your commit message suggests you're optimizing
>>> suspend. Maybe this commit makes non-suspend modesets slower because
>>> now we need to wait the panel power cycle earlier? Have you measured
>>> the possible downsides?
>>
>> Yes this is a problem. I again looked at the spec and found that t7 can
>> be 50, but VBT was default programming 200ms which is overriding our
>> driver initialized value. Correcting VBT gives back 150ms here. But yes
>> this will impact non-suspend modeset paths. Perhaps we should figure out
>> a way to avoid this and do only when relevant. A flag based check would
>> work ? I know it sounds hackish.
>>
>>>
>>>>                   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>                   intel_edp_panel_vdd_on(intel_dp);
>>>>                   intel_edp_panel_off(intel_dp);
>>>> +
>>>> +               /* Give additional delay of 250 ms so that resume time will
>>>> +                  be faster and also meets T12 delay.
>>>> +               */
>>>
>>> The comment says 250ms, but the code doesn't. Also, there's a missing
>>> '*' char in the comment.
>>>
>>>> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
>>>> +                                      (intel_dp->panel_power_cycle_delay/2));
>>>
>>> Why wait half the panel power cycle? Why did you choose exactly this value?
>>>
>>
>> Actually I would want to wait out full panel power cycle delay. Brings
>> our resume time down to ~250ms. But ultimately if this is acceptable
>> solution, will still depend on suspend KPIs and we might have to
>> balance. But as far as I know suspend path has no strict KPI, so we
>> might get away with this.
>>
>> Regards
>> Shobhit
>>
>>> Thanks,
>>> Paulo
>>>
>>>>           }
>>>>
>>>>           if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>