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

[Jani Nikula <jani.nikula@linux.intel.com>]

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].

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.nikula@intel.com
[2] https://lore.kernel.org/r/20250626192632.2330349-1-jani.nikula@intel.com
[3] https://lore.kernel.org/r/cover.1751023767.git.jani.nikula@intel.com
[4] https://lore.kernel.org/r/aF67cxjlfWiWMx-4@intel.com

> Signed-off-by: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@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