Re: [PATCH 5/8] drm/i915/display: Check if final vblank is sufficient for PSR features

["Hogander, Jouni" <jouni.hogander@intel.com>]

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/aOVOJp2zeN1eCp7O@intel.com/
> [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 <ankit.k.nautiyal@intel.com>
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Cc: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >   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__ */