On Fri, 2025-10-10 at 19:12 +0530, Nautiyal, Ankit K wrote: > > On 10/10/2025 12:23 PM, Hogander, Jouni wrote: > > On Thu, 2025-10-09 at 14:30 +0530, Ankit Nautiyal wrote: > > > Currently, wake line latency checks rely on the vblank length, > > > which does not account for either the extra vblank delay for > > > icl/tgl > > > or for > > > the optimized guardband which will come into picture later at > > > some > > > point. > > > > > > Introduce intel_dp_compute_config_late() to handle late-stage > > > configuration checks for DP/eDP features. For now, it validates > > > whether the > > > final vblank (with extra vblank delay) or guardband is sufficient > > > to > > > support wake line latencies required by Panel Replay and PSR2 > > > selective > > > update. > > > > > > Check if vblank is sufficient for PSR features, and disable them > > > if > > > their > > > wake requirements cannot be accomodated. > > Now as we are adding this: Can't we just drop checks made earlier > > and > > rely on psr_compute_config_late checking the vblank? > > > You're right to raise this question. The key point is that there are > dependencies between the PSR configuration, the VRR guardband, and > SCL > that influence the sequence of checks. > > Here’s how the flow works: > > > 1. psr_compute_config() > This is called first to determine if PSR is possible. > At this stage: > > -> We check if the vblank is long enough to accommodate wake lines. > -> However, we don’t yet know the actual guardband or whether SCL > lines > need to be accounted for. > -> So, we can only establish whether the vblank length is sufficient > in > a general sense. > -> On platforms like ICL/TGL (with extra vblank delay) or with > optimized > guardband, the actual lines may be fewer than the full vblank length.
Please add a comment into psr_compute_config that it is roughly checking if PSR is possible with current understanding of vblank length. It will be checked later in psr_compute_config_late against optimized vblank length. > > > 2. compute_scl() > > -> This computes the SCL. > -> If PSR was not enabled earlier, SCL will be 0 at this point. > -> The vblank_start is adjusted to accommodate the SCL lines. > > > 3. vrr_compute_guardband() > > -> This sets the guardband. > -> With optimized guardband, we consider max PSR requirements and > other > prefill latencies. > -> On platforms where VRR TG is always active, the guardband cannot > be > changed dynamically and any change in guardband triggers a full > modeset. > -> So, the goal is to set a guardband during modeset that works > across > most scenarios. > > > 4. psr_compute_config_late() > > -> This is where we re-check if the guardband is sufficient for PSR > wake > time latencies. > -> If not, we disable PSR features that can’t be supported with the > current timing. Add comment into psr_compute_config_late about SCL being left untouched and containing intel_psr_set_context_latency if PSR was possible after intel_psr_compute_config. > > > As mentioned in the earlier comment, more details are available in > the > following references: > [1] https://lore.kernel.org/all/[email protected]/ > [2] > https://patchwork.freedesktop.org/patch/678520/?series=151245&rev=13 > > So to answer your question: We can't entirely drop the early checks > in > psr_compute_config(), as it helps to filter PSR early based on vblank > length, and also helps to get the SCL adjustments. By the time we > reach > psr_compute_config_late() we have more accurate picture to take a > call > to disable specific PSR features. > > > That said, do you see any issues if we disable these later? > Also, are there other parts or logic that depend on > crtc_state->has_panel_replay and crtc_state->has_sel_update that you > think could be moved to psr_compute_config_late()? I don't see other need for psr_compute_config_late ATM. BR, Jouni Högander > > Regards, > > Ankit > > > > > BR, > > > > Jouni Högander > > > > > Signed-off-by: Ankit Nautiyal <[email protected]> > > > Cc: Animesh Manna <[email protected]> > > > Cc: Jouni Högander <[email protected]> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 3 ++ > > > drivers/gpu/drm/i915/display/intel_dp.c | 9 +++++ > > > drivers/gpu/drm/i915/display/intel_dp.h | 3 ++ > > > drivers/gpu/drm/i915/display/intel_psr.c | 51 > > > ++++++++++++++++++++-- > > > -- > > > drivers/gpu/drm/i915/display/intel_psr.h | 2 + > > > 5 files changed, 60 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index c09aa759f4d4..94c593bbedf4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -4560,6 +4560,9 @@ static int > > > intel_ddi_compute_config_late(struct > > > intel_encoder *encoder, > > > struct drm_connector *connector = conn_state->connector; > > > u8 port_sync_transcoders = 0; > > > > > > + if (intel_crtc_has_dp_encoder(crtc_state)) > > > + intel_dp_compute_config_late(encoder, > > > crtc_state, > > > conn_state); > > > + > > > drm_dbg_kms(display->drm, "[ENCODER:%d:%s] > > > [CRTC:%d:%s]\n", > > > encoder->base.base.id, encoder->base.name, > > > crtc_state->uapi.crtc->base.id, crtc_state- > > > > uapi.crtc->name); > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index a723e846321f..e481ff4c4959 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -6979,3 +6979,12 @@ void intel_dp_mst_resume(struct > > > intel_display > > > *display) > > > } > > > } > > > } > > > + > > > +void intel_dp_compute_config_late(struct intel_encoder *encoder, > > > + struct intel_crtc_state > > > *crtc_state, > > > + struct drm_connector_state > > > *conn_state) > > > +{ > > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > + > > > + intel_psr_compute_config_late(intel_dp, crtc_state); > > > +} > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h > > > b/drivers/gpu/drm/i915/display/intel_dp.h > > > index b379443e0211..0d9573ca44cb 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.h > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h > > > @@ -218,5 +218,8 @@ int intel_dp_compute_min_hblank(struct > > > intel_crtc_state *crtc_state, > > > int intel_dp_dsc_bpp_step_x16(const struct intel_connector > > > *connector); > > > void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp, bool > > > force_on_external); > > > bool intel_dp_in_hdr_mode(const struct drm_connector_state > > > *conn_state); > > > +void intel_dp_compute_config_late(struct intel_encoder *encoder, > > > + struct intel_crtc_state > > > *crtc_state, > > > + struct drm_connector_state > > > *conn_state); > > > > > > #endif /* __INTEL_DP_H__ */ > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 212bd244beed..dcab4127b399 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1405,6 +1405,20 @@ int > > > _intel_psr_min_set_context_latency(const > > > struct intel_crtc_state *crtc_state > > > return 1; > > > } > > > > > > +static bool _wake_lines_fit_into_vblank(const struct > > > intel_crtc_state *crtc_state, > > > + int vblank, > > > + int wake_lines) > > > +{ > > > + if (crtc_state->req_psr2_sdp_prior_scanline) > > > + vblank -= 1; > > > + > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line > > > count > > > */ > > > + if (vblank < wake_lines) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > static bool wake_lines_fit_into_vblank(struct intel_dp > > > *intel_dp, > > > const struct > > > intel_crtc_state > > > *crtc_state, > > > bool aux_less, > > > @@ -1428,14 +1442,7 @@ static bool > > > wake_lines_fit_into_vblank(struct > > > intel_dp *intel_dp, > > > crtc_state- > > > > alpm_state.fast_wake_lines) : > > > crtc_state->alpm_state.io_wake_lines; > > > > > > - if (crtc_state->req_psr2_sdp_prior_scanline) > > > - vblank -= 1; > > > - > > > - /* Vblank >= PSR2_CTL Block Count Number maximum line > > > count > > > */ > > > - if (vblank < wake_lines) > > > - return false; > > > - > > > - return true; > > > + return _wake_lines_fit_into_vblank(crtc_state, vblank, > > > wake_lines); > > > } > > > > > > static bool alpm_config_valid(struct intel_dp *intel_dp, > > > @@ -4346,3 +4353,31 @@ bool intel_psr_needs_alpm_aux_less(struct > > > intel_dp *intel_dp, > > > { > > > return intel_dp_is_edp(intel_dp) && crtc_state- > > > > has_panel_replay; > > > } > > > + > > > +void intel_psr_compute_config_late(struct intel_dp *intel_dp, > > > + struct intel_crtc_state > > > *crtc_state) > > > +{ > > > + struct intel_display *display = > > > to_intel_display(intel_dp); > > > + int vblank = intel_crtc_vblank_length(crtc_state); > > > + int aux_less_wake_lines = crtc_state- > > > > alpm_state.aux_less_wake_lines; > > > + int wake_lines = DISPLAY_VER(display) < 20 ? > > > + psr2_block_count_lines(crtc_state- > > > > alpm_state.io_wake_lines, > > > + crtc_state- > > > > alpm_state.fast_wake_lines) : > > > + crtc_state->alpm_state.io_wake_lines; > > > + > > > + if (intel_psr_needs_alpm_aux_less(intel_dp, crtc_state) > > > && > > > + !_wake_lines_fit_into_vblank(crtc_state, vblank, > > > aux_less_wake_lines)) { > > > + drm_dbg_kms(display->drm, > > > + "Disabling Panel replay: vblank > > > insufficient for wakelines = %d\n", > > > + aux_less_wake_lines); > > > + crtc_state->has_panel_replay = false; > > > + } > > > + > > > + if (crtc_state->has_sel_update && > > > + !_wake_lines_fit_into_vblank(crtc_state, vblank, > > > wake_lines)) { > > > + drm_dbg_kms(display->drm, > > > + "Disabling Selective Update: vblank > > > insufficient for wakelines = %d\n", > > > + wake_lines); > > > + crtc_state->has_sel_update = false; > > > + } > > > +} > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > > b/drivers/gpu/drm/i915/display/intel_psr.h > > > index 9147996d6c9e..b17ce312dc37 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > > @@ -83,5 +83,7 @@ void intel_psr_debugfs_register(struct > > > intel_display *display); > > > bool intel_psr_needs_alpm(struct intel_dp *intel_dp, const > > > struct > > > intel_crtc_state *crtc_state); > > > bool intel_psr_needs_alpm_aux_less(struct intel_dp *intel_dp, > > > const struct intel_crtc_state > > > *crtc_state); > > > +void intel_psr_compute_config_late(struct intel_dp *intel_dp, > > > + struct intel_crtc_state > > > *crtc_state); > > > > > > #endif /* __INTEL_PSR_H__ */
