Re: [PATCH 1/5] drm/i915/psr: Add helper to get min psr guardband

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

On Fri, 2025-10-17 at 12:58 +0300, Hogander, Jouni wrote:
> On Fri, 2025-10-17 at 15:07 +0530, Nautiyal, Ankit K wrote:
> > 
> > On 10/17/2025 2:37 PM, Hogander, Jouni wrote:
> > > On Fri, 2025-10-17 at 10:31 +0530, Ankit Nautiyal wrote:
> > > > Introduce a helper to compute the max link wake latency when
> > > > using
> > > > Auxless/Aux wake mechanism for PSR/Panel Replay/LOBF features.
> > > > 
> > > > This will be used to compute the minimum guardband so that the
> > > > link
> > > > wake
> > > > latencies are accounted and these features work smoothly for
> > > > higher
> > > > refresh rate panels.
> > > > 
> > > > Bspec: 70151, 71477
> > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/display/intel_psr.c | 12 ++++++++++++
> > > >   drivers/gpu/drm/i915/display/intel_psr.h |  1 +
> > > >   2 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 703e5f6af04c..a8303b669853 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -4416,3 +4416,15 @@ void
> > > > intel_psr_compute_config_late(struct
> > > > intel_dp *intel_dp,
> > > >   
> > > >   	intel_psr_set_non_psr_pipes(intel_dp, crtc_state);
> > > >   }
> > > > +
> > > > +int intel_psr_min_guardband(struct intel_crtc_state
> > > > *crtc_state)
> > > > +{
> > > > +	struct intel_display *display =
> > > > to_intel_display(crtc_state);
> > > > +	int auxless_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;
> > > > +
> > > > +	return max(auxless_wake_lines, wake_lines);
> > > hmm, now if you add:
> > > 
> > > if (crtc_state->req_psr2_sdp_prior_scanline)
> > > 		psr_min_guardband++;
> > 
> > I did not get this part. Do we need to account for 1 more wakelines
> > if 
> > this flag is set?
> 
> If you look at how that flag affects vblank check in 
> intel_psr_compute_config_late:
> 
> ...
> 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;
> ...
> 
> So to take that into account when calculating minimum guardband
> needed
> by PSR you need to increase by one. Same goes with SCL:
> 
> ...
> 	int scl = _intel_psr_min_set_context_latency(crtc_state,
> 						    
> needs_panel_replay,
> 						    
> needs_sel_update);
> 	vblank -= scl;
> ...
> 
> You are already partially taking into account PSR needs when
> calculating optimized guardband except these two lines which are
> needed
> conditionally.
> 
> Also intel_psr_compute_config is run at this point -> you know which
> one to use: auxless wake time or aux wake time. no need to use max()
> with them. Just choose the one which is relevant.
> 
> With these changes you could drop intel_psr_compute_config_late as
> vblank would be long enough for PSR mode computed by
> intel_psr_compute_config, no?

Ok, noticed now this in the last patch:

...
crtc_state->vrr.guardband = min(guardband, intel_vrr_max_guardband(crtc_state));
...

So if we need to fall back to intel_vrr_max_guardband we need to have
that intel_psr_compute_config_late.

Anyways I think you need to take into account that
req_psr2_sdp_prior_scanline and _intel_psr_min_set_context_latency in
intel_psr_min_guardband.

BR,

Jouni Högander
> 
> BR,
> 
> Jouni Högander
> 
> 
> > 
> > What we want to do is to check for min guardband for 
> > panel_replay/sel_update based on the required wakelines.
> > 
> > Whether we can use the auxless_wake_lines and wake_lines as
> > computed 
> > above to estimate the max wakelines or instead we should use
> > functions 
> > from alpm.c :
> > 
> > io_buffer_wake_time() and _lnl_compute_aux_less_wake_time() to get
> > the 
> > worst case wakelines.
> > 
> > 
> > > 
> > > Whatever is the PSR mode it can be enabled what comes to vblank
> > > restrictions and you can drop psr_compute_config_late?
> > 
> > 
> > I think we cannot drop the psr_compute_config_late as it checks
> > whether 
> > the actual guardband is enough for PSR features.
> > 
> > intel_psr_min_guardband() is used along with other features to have
> > an estimate on the guardband that works for all cases, without a
> > need
> > to change the guardband.
> > It is bounded by the vblank length available
> > 
> > Regards,
> > 
> > Ankit
> > 
> > > 
> > > BR,
> > > 
> > > Jouni Högander
> > > 
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > index b17ce312dc37..620b35928832 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > @@ -85,5 +85,6 @@ 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);
> > > > +int intel_psr_min_guardband(struct intel_crtc_state
> > > > *crtc_state);
> > > >   
> > > >   #endif /* __INTEL_PSR_H__ */
>