[PATCH v4] drm/i915/display: Optimize panel power-on wait time
Dibin Moolakadan Subrahmanian
dibin.moolakadan.subrahmanian at intel.com
Tue Jul 29 14:34:17 UTC 2025
On 29-07-2025 18:57, Jani Nikula wrote:
> On Tue, 29 Jul 2025, Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian at intel.com> wrote:
>> 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
> It's still opening a can of worms. The 5 second timeout is relevant for
> all cases either, so you'd have to pass *both*. And it's not so simple
> anymore.
>
> BR,
> Jani.
I assume any of the above 3 cases should not pass maximum timeout of 5 seconds, so
wanted to keep the same maximum timeout currently present for wait_panel_status().
>
>>
>>>> {
>>>> 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)
More information about the Intel-gfx
mailing list