<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<pre>
</pre>
<div class="moz-cite-prefix">On 29-07-2025 13:47, Jani Nikula wrote:<br>
</div>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">On Mon, 28 Jul 2025, Dibin Moolakadan Subrahmanian <a class="moz-txt-link-rfc2396E" href="mailto:dibin.moolakadan.subrahmanian@intel.com"><dibin.moolakadan.subrahmanian@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
It's not about the timeout, it's about the exponentially increasing poll
delay.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please do not say "this patch" in commit messages. Just use the
imperative "Replace ...".
The commit messages is unnecessarily indented with a space.</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">will correct this.</pre>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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 <a class="moz-txt-link-rfc2396E" href="mailto:dibin.moolakadan.subrahmanian@intel.com"><dibin.moolakadan.subrahmanian@intel.com></a>
---
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>
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please look at how includes are ordered in every single file in i915.</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">will correct this.</pre>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">will correct this.</pre>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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?</pre>
</blockquote>
<pre>Motive behind adding new parameter is to adjust the poll time based on case.
Currently each call is taking different time to complete as below
for panel power off time - 82 ms
for panel power cycle - 0.074 ms
for panel power on - 327 ms
Making default poll interval 10ms will increase panel power cycle time to 10ms
</pre>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> {
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;
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Nitpick, usually just "val".</pre>
</blockquote>
will correct this.
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
will correct this.
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ 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);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
The outer parenthesis in the parameters are superfluous.
The comments are useless (and have a typo too).
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+ if (ret == 0)
+ goto panel_wait_complete;
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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...
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">will correct this.</pre>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
+ drm_err(display->drm,
+ "dibin [ENCODER:%d:%s] %s panel status timeout: PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
...and it'll be easier to notice you've left your name in the debug
logs. Oops.
</pre>
</blockquote>
<blockquote type="cite" cite="mid:018afd66168bd9a55abe4e14c24e9b11a89ab15c@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
</html>