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]. 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]. [1] https://lore.kernel.org/r/cover.1750959689.git.jani.nik...@intel.com [2] https://lore.kernel.org/r/20250626192632.2330349-1-jani.nik...@intel.com [3] https://lore.kernel.org/r/cover.1751023767.git.jani.nik...@intel.com [4] https://lore.kernel.org/r/af67cxjlfwiwm...@intel.com > Signed-off-by: Dibin Moolakadan Subrahmanian > <dibin.moolakadan.subrahman...@intel.com> > --- > drivers/gpu/drm/i915/display/intel_pps.c | 61 +++++++++++++++++++++++- > 1 file changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c > b/drivers/gpu/drm/i915/display/intel_pps.c > index bff81fb5c316..57a062c8038d 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.c > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > @@ -598,8 +598,67 @@ void intel_pps_check_power_unlocked(struct intel_dp > *intel_dp) > #define IDLE_CYCLE_MASK (PP_ON | PP_SEQUENCE_MASK | > PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK) > #define IDLE_CYCLE_VALUE (0 | PP_SEQUENCE_NONE | 0 > | PP_SEQUENCE_STATE_OFF_IDLE) > > +#define PANEL_TYPICAL_ON_TIME_MS (180) > +#define PANEL_MAXIMUM_ON_TIME_MS (5000) > + > static void intel_pps_verify_state(struct intel_dp *intel_dp); > > +/* > + * Panel power-on typically completes within ~200ms. Using a large timeout > + * (5000ms) with intel_de_wait() results in unnecessary delays, > + * especially under Xe, where xe_mmio_wait32() uses an exponential backoff. > + * > + * To optimize resume and power-on latency, we first wait for the typical > + * completion window, then perform short polling loops thereafter. > + * This reduces worst-case latency while still ensuring correctness. > + */ > +static void wait_panel_on_status(struct intel_dp *intel_dp) > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + i915_reg_t pp_stat_reg, pp_ctrl_reg; > + u32 mask = IDLE_ON_MASK; > + u32 value = IDLE_ON_VALUE; > + int elapsed = PANEL_TYPICAL_ON_TIME_MS; > + > + lockdep_assert_held(&display->pps.mutex); > + > + intel_pps_verify_state(intel_dp); > + > + pp_stat_reg = _pp_stat_reg(intel_dp); > + pp_ctrl_reg = _pp_ctrl_reg(intel_dp); > + > + drm_dbg_kms(display->drm, > + "[ENCODER:%d:%s] %s mask: 0x%08x value: 0x%08x PP_STATUS: > 0x%08x PP_CONTROL: 0x%08x\n", > + dig_port->base.base.base.id, dig_port->base.base.name, > + pps_name(intel_dp), > + mask, value, > + intel_de_read(display, pp_stat_reg), > + intel_de_read(display, pp_ctrl_reg)); > + > + /* Wait for typical panel on time first */ > + if (intel_de_wait(display, pp_stat_reg, mask, value, > PANEL_TYPICAL_ON_TIME_MS) == 0) > + goto panel_wait_complete; > + > + /* Wait for maxtime in 20ms intervals */ > + while (elapsed < PANEL_MAXIMUM_ON_TIME_MS) { > + if (intel_de_wait(display, pp_stat_reg, mask, value, 20) == 0) > + goto panel_wait_complete; > + > + elapsed += 20; > + } That's just not how time works in the kernel. Please let's not reinvent wheels. BR, Jani. > + > + drm_err(display->drm, > + "[ENCODER:%d:%s] %s panel status timeout: PP_STATUS: 0x%08x > PP_CONTROL: 0x%08x\n", > + dig_port->base.base.base.id, dig_port->base.base.name, > + pps_name(intel_dp), > + intel_de_read(display, pp_stat_reg), > + intel_de_read(display, pp_ctrl_reg)); > + > +panel_wait_complete: > + drm_dbg_kms(display->drm, "Wait complete\n"); > +} > + > static void wait_panel_status(struct intel_dp *intel_dp, > u32 mask, u32 value) > { > @@ -642,7 +701,7 @@ static void wait_panel_on(struct intel_dp *intel_dp) > "[ENCODER:%d:%s] %s wait for panel power on\n", > dig_port->base.base.base.id, dig_port->base.base.name, > pps_name(intel_dp)); > - wait_panel_status(intel_dp, IDLE_ON_MASK, IDLE_ON_VALUE); > + wait_panel_on_status(intel_dp); > } > > static void wait_panel_off(struct intel_dp *intel_dp) -- Jani Nikula, Intel
