On Wed, 16 Nov 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Wed, Nov 16, 2022 at 05:06:57PM +0200, Jani Nikula wrote: >> Panel power off delay is the time the panel power needs to remain off >> after being switched off, before it can be switched on again. >> >> For the purpose of respecting panel power off delay at driver probe, >> assuming the panel was last switched off at driver probe is overly >> pessimistic. If the panel was never on, we'd end up waiting for no >> reason. >> >> We don't know what has happened before kernel boot, but we can make some >> assumptions: >> >> - The panel may have been switched off right before kernel boot by some >> pre-os environment. >> >> - After kernel boot, the panel may only be switched off by i915. >> >> - At i915 driver probe, only a previously loaded and removed i915 may >> have switched the panel power off. >> >> With these assumptions, we can initialize the last power off time to >> kernel boot time, if we also ensure i915 driver remove waits for the >> panel power off delay after switching panel power off. >> >> This shaves off the time it takes from kernel boot to i915 probe from >> the first panel enable, if (and only if) the panel was not already >> enabled at boot. >> >> The encoder destroy hook is pretty much the last place where we can >> wait, right after we've ensured the panel power has been switched off, >> and before the whole encode is destroyed. > > Yeah, the fact that we have the vdd_off_sync() in there at least > theoretically means the vdd might be on at any point before that. > > I was also pondering about doing this for all encoder types. > Though the benefits are slightly less pronounced I guess: > a) we don't need to power the panel for the output probe on those, > so a bit more time will have elapsed anyway before we have to > power the panel on during the first modeset > b) for LVDS we rely 100% on the pps state machine so the initial > boot case is already as optimal as possible. Adding the explicit > wait into the unload path could thus only help the speed of > of the first modeset during module reload > > But maybe we'd stil want to do that, for consistency if nothing else? > > Either way, this patch is > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Thanks for the review and testing, pushed to drm-intel-next. BR, Jani. > >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7417 >> Cc: Lee Shawn C <shawn.c.lee@xxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ >> drivers/gpu/drm/i915/display/intel_pps.c | 8 +++++++- >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 914161d7d122..67089711d9e2 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -4877,6 +4877,12 @@ void intel_dp_encoder_flush_work(struct drm_encoder *encoder) >> >> intel_pps_vdd_off_sync(intel_dp); >> >> + /* >> + * Ensure power off delay is respected on module remove, so that we can >> + * reduce delays at driver probe. See pps_init_timestamps(). >> + */ >> + intel_pps_wait_power_cycle(intel_dp); >> + >> intel_dp_aux_fini(intel_dp); >> } >> >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c >> index 81ee7f3aadf6..9bbf41a076f7 100644 >> --- a/drivers/gpu/drm/i915/display/intel_pps.c >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c >> @@ -1100,7 +1100,13 @@ bool intel_pps_have_panel_power_or_vdd(struct intel_dp *intel_dp) >> >> static void pps_init_timestamps(struct intel_dp *intel_dp) >> { >> - intel_dp->pps.panel_power_off_time = ktime_get_boottime(); >> + /* >> + * Initialize panel power off time to 0, assuming panel power could have >> + * been toggled between kernel boot and now only by a previously loaded >> + * and removed i915, which has already ensured sufficient power off >> + * delay at module remove. >> + */ >> + intel_dp->pps.panel_power_off_time = 0; >> intel_dp->pps.last_power_on = jiffies; >> intel_dp->pps.last_backlight_off = jiffies; >> } >> -- >> 2.34.1 -- Jani Nikula, Intel Open Source Graphics Center
