On Thu, 03 Jul 2025, Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahman...@intel.com> wrote: > On 02-07-2025 14:31, Jani Nikula wrote: >> On Tue, 01 Jul 2025, Lucas De Marchi <lucas.demar...@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.subrahman...@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.syrj...@linux.intel.com > > Regards, > > Dibin > >> BR, >> Jani. > >> >> -- Jani Nikula, Intel
