Re: [PATCH] drm/i915/display: Optimize panel power-on wait time

[Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>]

On 04-07-2025 18:17, Jani Nikula wrote:
> On Thu, 03 Jul 2025, Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com> wrote:
>> On 02-07-2025 14:31, Jani Nikula wrote:
>>> On Tue, 01 Jul 2025, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>> On Tue, Jul 01, 2025 at 12:28:41PM +0300, Jani Nikula wrote:
>>>>> On Mon, 30 Jun 2025, Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com> wrote:
>>>>>>    The current wait_panel_on() uses intel_de_wait() with a long timeout
>>>>>>    (5000ms), which is suboptimal on Xe platforms where the underlying
>>>>>>    xe_mmio_wait32() employs an exponential backoff strategy. This leads
>>>>>>    to unnecessary delays during resume or power-on  when the panel becomes
>>>>>>    ready earlier than the full timeout.
>>>>>>
>>>>>>    This patch splits the total wait time into two pases
>>>>>>       - First wait for the typical panel-on time(180ms)
>>>>>>       - If panel is not ready , continue polling in short 20ms intervals
>>>>>>         until the maximum timeout (5000ms) is reached
>>>>> I'm *very* reluctant to merge any new custom wait hacks. I'm in the
>>>>> process of *removing* a plethora of them [1][2][3].
>>>> good riddance
>>> Yay!
>>>
>>>>> I think the question is, should xe_mmio_wait32() (and the i915
>>>>> counterpart) and the intel_de_wait*() functions be migrated to an
>>>>> interface similar to read_poll_timeout(), i.e. provide sleep and timeout
>>>>> separately, no exponential backoff. And implement the waits using
>>>>> read_poll_timeout(), or whatever Ville ends up with [4].
>>>> I saw your patch series and I'm eagerly waiting it to either propagate
>>>> it in xe or have someone merge such a patch.  I'm not sure about
>>>> removing the exponential backoff is a good thing overall, but if it's
>>>> needed then it needs to be justified to add a new function to pair with
>>>> read_poll_timeout(), not a custom driver function.
>>> While I'm negative about the patch at hand, the underlying problem is
>>> very real.
>>>
>>> I think at the very least the exponential sleep backoff needs an upper
>>> bound that's relative to the timeout. Maybe 10-25% of timeout?
>>>
>>> With the example case of 5 second timeout, the exponential backoff
>>> starting from 10 us leads to a dozen polls before reaching 100 ms
>>> elapsed time, but then polls at approximately 1 s, 2 s, 4 s, and 8 s
>>> elapsed time. Longer timeouts are of course rare, but they exhibit
>>> increasingly worse behaviour.
>>>
>>> So if what we're waiting takes 2.1 seconds, the next check will be about
>>> 2 seconds later. Similarly, if it takes 4.1 seconds, the next check will
>>> be about 4 seconds later, in this case exceeding the timeout by 3
>>> seconds.
>>>
>>> Anyway, if xe_mmio_wait32() remains as it is, with read_poll_timeout()
>>> it's trivial to do the wait in the intel_de_*() macros, in display side,
>>> with sleeps and timeouts defined in display. Because for most things the
>>> really quick fast polls are useless in display.
>>>
>> This exponential sleep back-off is causing around 120ms additional
>> delay in resume compared to  i915.
>>
>> how about polling as below , use intel_de_read and read_poll_timeout
>>
>>       ret = read_poll_timeout(intel_de_read, reg_val,
>>                       ((reg_val & mask) == value),
>>                       (20 * 1000),                        // poll every 20ms
>>                       (PANEL_MAXIMUM_ON_TIME_MS * 1000),  // total
>> timeout (us)
>>                       true,
>>                       display, pp_stat_reg);
> This would be a temporary measure pending Ville's work [1], but I'm not
> against it.
>
> Also, needs to happen in wait_panel_status() instead of adding a
> separate wait_panel_on_status() function.
>
> BR,
> Jani.
>
>
> [1] https://lore.kernel.org/r/20250702223439.19752-1-ville.syrjala@linux.intel.com

Adding this logic in wait_panel_status() is causing ~20ms delay when function called from
wait_panel_power_cycle() which expects minimal time.
Adding new function avoids this scenario.

Regards,
Dibin

>
>> Regards,
>>
>> Dibin
>>
>>> BR,
>>> Jani.
>>>