Re: [PATCH] drm/i915: Suspend resume timing optimization.

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

On 12/08/2015 02:22 AM, Paulo Zanoni wrote:
> 2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com>:
>> From: Abhay Kumar <abhay.kumar@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 ?

>>
>> Signed-off-by: Abhay Kumar <abhay.kumar@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx