Re: [PATCH 17/20] drm/i915/display: Use fixed_rr timings in modeset sequence

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

On Wed, Feb 26, 2025 at 05:06:34PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 24, 2025 at 11:47:14AM +0530, Ankit Nautiyal wrote:
> > During modeset enable sequence, program the fixed timings, and turn on the
> > VRR Timing Generator (VRR TG) for platforms that always use VRR TG.
> > 
> > For this intel_vrr_set_transcoder now always programs fixed timings.
> > Later if vrr timings are required, vrr_enable() will switch
> > to the real VRR timings.
> > 
> > For platforms that will always use VRR TG, the VRR_CTL Enable bit is set
> > and reset in the transcoder enable/disable path.
> > 
> > v2: Update intel_vrr_set_transcoder_timings for fixed_rr.
> > v3: Update intel_set_transcoder_timings_lrr for fixed_rr. (Ville)
> > v4: Have separate functions to enable/disable VRR CTL
> > 
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c    |  5 +++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c |  4 +++
> >  drivers/gpu/drm/i915/display/intel_vrr.c    | 40 +++++++++++++--------
> >  drivers/gpu/drm/i915/display/intel_vrr.h    |  3 ++
> >  4 files changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 5082f38b0a02..8863d1526aea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -77,6 +77,7 @@
> >  #include "intel_tc.h"
> >  #include "intel_vdsc.h"
> >  #include "intel_vdsc_regs.h"
> > +#include "intel_vrr.h"
> >  #include "skl_scaler.h"
> >  #include "skl_universal_plane.h"
> >  
> > @@ -3276,6 +3277,8 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
> >  				   const struct intel_crtc_state *old_crtc_state,
> >  				   const struct drm_connector_state *old_conn_state)
> >  {
> > +	intel_vrr_transcoder_disable(old_crtc_state);
> > +
> >  	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST))
> >  		intel_ddi_post_disable_hdmi_or_sst(state, encoder, old_crtc_state,
> >  						   old_conn_state);
> > @@ -3524,6 +3527,8 @@ static void intel_ddi_enable(struct intel_atomic_state *state,
> >  
> >  	intel_ddi_enable_transcoder_func(encoder, crtc_state);
> >  
> > +	intel_vrr_transcoder_enable(crtc_state);
> > +
> >  	/* Enable/Disable DP2.0 SDP split config before transcoder */
> >  	intel_audio_sdp_split_update(crtc_state);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 2c4a9ac6f61e..5ec353eceab4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -1048,6 +1048,8 @@ static void mst_stream_post_disable(struct intel_atomic_state *state,
> >  		intel_crtc_vblank_off(old_pipe_crtc_state);
> >  	}
> >  
> > +	intel_vrr_transcoder_disable(old_crtc_state);
> > +
> >  	intel_disable_transcoder(old_crtc_state);
> >  
> >  	drm_dp_remove_payload_part1(&intel_dp->mst_mgr, new_mst_state, new_payload);
> > @@ -1325,6 +1327,8 @@ static void mst_stream_enable(struct intel_atomic_state *state,
> >  
> >  	intel_ddi_enable_transcoder_func(encoder, pipe_config);
> >  
> > +	intel_vrr_transcoder_enable(pipe_config);
> > +
> >  	intel_ddi_clear_act_sent(encoder, pipe_config);
> >  
> >  	intel_de_rmw(display, TRANS_DDI_FUNC_CTL(display, trans), 0,
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index e77f5b483b09..551dcc16f0d4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -434,6 +434,7 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_display *display = to_intel_display(crtc_state);
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +	bool needs_modeset = intel_crtc_needs_modeset(crtc_state);
> >  
> >  	if (intel_crtc_is_joiner_secondary(crtc_state))
> >  		return;
> > @@ -447,12 +448,6 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
> >  		intel_de_rmw(display, CHICKEN_TRANS(display, cpu_transcoder),
> >  			     0, PIPE_VBLANK_WITH_DELAY);
> >  
> > -	if (!intel_vrr_possible(crtc_state)) {
> > -		intel_de_write(display,
> > -			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
> > -		return;
> > -	}
> > -
> >  	if (crtc_state->cmrr.enable) {
> >  		intel_de_write(display, TRANS_CMRR_M_HI(display, cpu_transcoder),
> >  			       upper_32_bits(crtc_state->cmrr.cmrr_m));
> > @@ -464,14 +459,8 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
> >  			       lower_32_bits(crtc_state->cmrr.cmrr_n));
> >  	}
> >  
> > -	intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> > -		       crtc_state->vrr.vmin - 1);
> > -	intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> > -		       crtc_state->vrr.vmax - 1);
> > -	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > -		       trans_vrr_ctl(crtc_state));
> > -	intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
> > -		       crtc_state->vrr.flipline - 1);
> > +	intel_vrr_set_fixed_rr_timings(crtc_state);
> > +	intel_vrr_update_guardband(crtc_state, needs_modeset);
> >  
> >  	if (HAS_AS_SDP(display))
> >  		intel_de_write(display,
> > @@ -614,6 +603,29 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> >  	intel_vrr_set_fixed_rr_timings(old_crtc_state);
> >  }
> >  
> > +void intel_vrr_transcoder_enable(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_display *display = to_intel_display(crtc_state);
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +
> > +	if (!intel_vrr_always_use_vrr_tg(display))
> > +		return;
> > +
> > +	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > +		       VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> 
> Hmm. Maybe we should actually program this even for the
> !intel_vrr_always_use_vrr_tg() case, but just leave the enable
> bit unset. That way we shouldn't need the intel_vrr_update_guardband()
> stuff in vrr_set_transcoder_timings().
> 
> We'd still need something for the _lrr() case, but I think that
> could simply call intel_vrr_transcoder_enable() as well.

Oh and for the intel_vrr_always_use_vrr_tg()==true case we
probably want to also enable push.

When I was testing this stuff at least tgl and adl seemed to
require require push to be enabled whenever we use the VRR TG.
If we always enable it then we can easily test this on those
platforms witout having to touch any code outside of
intel_vrr_always_use_vrr_tg().

And even if some more modern hardware doesn't strictly require
push to be enabled, I don't think there should be any harm from
enabling it anyway.

-- 
Ville Syrjälä
Intel