Re: [PATCH 2/8] drm/i915/vrr: s/intel_vrr_compute_config_late/intel_vrr_compute_guardband

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 09, 2025 at 02:30:56PM +0530, Ankit Nautiyal wrote:
> The helper intel_vrr_compute_config_late() practically just computes the
> guardband. Rename intel_vrr_compute_config_late() to
> intel_vrr_compute_guardband().
> 
> Since we are going to compute the guardband and then move the
> vblank_start for optmizing guardband move it to
> intel_crtc_compute_config() which handles such changes.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 4 ++--
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 2 +-
>  drivers/gpu/drm/i915/display/intel_vrr.h     | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b57efd870774..cd499e58bed3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2414,6 +2414,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>  	if (ret)
>  		return ret;
>  
> +	intel_vrr_compute_guardband(crtc_state);
> +
>  	ret = intel_dpll_crtc_compute_clock(state, crtc);

Hmm. The intel_dpll_crtc_compute_clock() probably needs to move to the
very start of the function, so that we'll have an accurate clock for the 
eventual guardband calculations. In fact my plan has been to move it
into .compute_config() entirely, but I haven't had time to revisit
this topic in a while :/

For easier bisectability I'd do that move first as a separate patch.

>  	if (ret)
>  		return ret;

The other thing we have here is intel_crtc_compute_pipe_mode(). I have
a feeling I didn't consider the joiner aspect at all with the prefill
helpers. We might need the pipe_mode for the guardband calculations.
I'll have to have a look at what I did there and think a bit more about
how the joiner affects that stuff.


And the other thing I haven't considered at all is MSO. Right now
adjusted_mode will contain the per-segment timings with MSO which,
now that I think about it again, migth be a bad idea (my idea IIRC).
Eg. adjusted_mode based linetime calculations will be skewed by the
overlap included in the segement timings.

We may have to rethink the MSO apporoach to keep the full timings in
adjusted_mode and either introduce yet another mode for the per-segment
timings, or perhaps just do the full<->segment conversions as needed
(set_transcoder_timings()+its readout, compute_m_n(), maybe some other
places as well?).

> @@ -4722,8 +4724,6 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>  	struct drm_connector *connector;
>  	int i;
>  
> -	intel_vrr_compute_config_late(crtc_state);
> -
>  	for_each_new_connector_in_state(&state->base, connector,
>  					conn_state, i) {
>  		struct intel_encoder *encoder =
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 4bc14b5e685f..8d71d7dc9d12 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -433,7 +433,7 @@ intel_vrr_max_guardband(struct intel_crtc_state *crtc_state)
>  		   intel_vrr_max_vblank_guardband(crtc_state));
>  }
>  
> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> +void intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> index 7317f8730089..bc9044621635 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -21,7 +21,7 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_check_modeset(struct intel_atomic_state *state);
>  void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
> +void intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state);
>  void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_send_push(struct intel_dsb *dsb,
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel