[PATCH v4] drm/i915/display: Optimize panel power-on wait time
Dibin Moolakadan Subrahmanian
dibin.moolakadan.subrahmanian at intel.com
Tue Jul 29 13:21:09 UTC 2025
On 29-07-2025 13:47, Jani Nikula wrote:
> On Mon, 28 Jul 2025, Dibin Moolakadan Subrahmanian<dibin.moolakadan.subrahmanian at intel.com> 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.
will correct this.
>
>> 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<dibin.moolakadan.subrahmanian at intel.com>
>> ---
>> 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.
will correct this.
>
>> 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.
will correct this.
>> +
>> 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?
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
>
>> {
>> 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".
will correct this.
>
>> 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.
will correct this.
>
>> +
>> + 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...
>
will correct this.
>>
>> + 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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20250729/b762d3ec/attachment-0001.htm>
More information about the Intel-gfx
mailing list