On Mon, 28 Jul 2025, Dibin Moolakadan Subrahmanian <[email protected]> wrote: > The current wait_panel_status() 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.
It's not about the timeout, it's about the exponentially increasing poll delay. > This patch replaces intel_de_wait() with read_poll_timeout() + > intel_de_read() to actively poll the register at given interval and exit > early when panel is ready, improving resume latency Please do not say "this patch" in commit messages. Just use the imperative "Replace ...". The commit messages is unnecessarily indented with a space. > Changes in v2: > Replaced two-phase intel_de_wait() with read_poll_timeout() > + intel_de_read() > > Changes in v3: > - Add poll_interval_ms argument 'wait_panel_status' function. > - Modify 'wait_panel_status' callers with proper poll interval > > Changes in v4: > - Change 'wait_panel_off' poll interval to 10ms > > Signed-off-by: Dibin Moolakadan Subrahmanian > <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_pps.c | 41 +++++++++++++++++------- > 1 file changed, 30 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c > b/drivers/gpu/drm/i915/display/intel_pps.c > index b64d0b30f5b1..56ef835fc2eb 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.c > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > @@ -22,6 +22,7 @@ > #include "intel_pps.h" > #include "intel_pps_regs.h" > #include "intel_quirks.h" > +#include <linux/iopoll.h> Please look at how includes are ordered in every single file in i915. > static void vlv_steal_power_sequencer(struct intel_display *display, > enum pipe pipe); > @@ -600,14 +601,18 @@ 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_MAXIMUM_ON_TIME_MS (5000) The name of the macro is misleading. For single-use things, maybe better to just keep the value inline as it were. Side note, the parenthesis are superfluous here. > + > static void intel_pps_verify_state(struct intel_dp *intel_dp); > > static void wait_panel_status(struct intel_dp *intel_dp, > - u32 mask, u32 value) > + u32 mask, u32 value, int poll_interval_ms) Can we not add the extra parameter please? Can we have a meaningful default instead? 10 ms? Is the 1 ms poll interval really required? > { > 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; > + int ret; > + u32 reg_val; Nitpick, usually just "val". > lockdep_assert_held(&display->pps.mutex); > > @@ -624,14 +629,27 @@ static void wait_panel_status(struct intel_dp *intel_dp, > intel_de_read(display, pp_stat_reg), > intel_de_read(display, pp_ctrl_reg)); > > - if (intel_de_wait(display, pp_stat_reg, mask, value, 5000)) > - 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)); > + if (poll_interval_ms <= 0) > + poll_interval_ms = 1; //if <0 is passed go with 1ms Without the parameter, we could get rid of checks like this. The comment just duplicates what the code already says. Also, we don't use // comments. > + > + ret = read_poll_timeout(intel_de_read, reg_val, > + ((reg_val & mask) == value), > + (poll_interval_ms * 1000), // poll intervell > + (PANEL_MAXIMUM_ON_TIME_MS * 1000), // total > timeout (us) > + true, > + display, pp_stat_reg); The outer parenthesis in the parameters are superfluous. The comments are useless (and have a typo too). > + > + if (ret == 0) > + goto panel_wait_complete; We do use goto in kernel, but primarily for error handling. Please use if (ret) here, and the whole drm_err() thing remains unchanged, and doesn't become part of the patch... > > + drm_err(display->drm, > + "dibin [ENCODER:%d:%s] %s panel status timeout: PP_STATUS: > 0x%08x PP_CONTROL: 0x%08x\n", ...and it'll be easier to notice you've left your name in the debug logs. Oops. > + 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"); > } > > @@ -644,7 +662,8 @@ 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_status(intel_dp, IDLE_ON_MASK, IDLE_ON_VALUE, 20); > } > > static void wait_panel_off(struct intel_dp *intel_dp) > @@ -656,7 +675,7 @@ static void wait_panel_off(struct intel_dp *intel_dp) > "[ENCODER:%d:%s] %s wait for panel power off time\n", > dig_port->base.base.base.id, dig_port->base.base.name, > pps_name(intel_dp)); > - wait_panel_status(intel_dp, IDLE_OFF_MASK, IDLE_OFF_VALUE); > + wait_panel_status(intel_dp, IDLE_OFF_MASK, IDLE_OFF_VALUE, 10); > } > > static void wait_panel_power_cycle(struct intel_dp *intel_dp) > @@ -683,7 +702,7 @@ static void wait_panel_power_cycle(struct intel_dp > *intel_dp) > if (remaining) > wait_remaining_ms_from_jiffies(jiffies, remaining); > > - wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > + wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE, 1); > } > > void intel_pps_wait_power_cycle(struct intel_dp *intel_dp) -- Jani Nikula, Intel
