[PATCH v3 1/3] drm/i915/vrr: Update vrr.vsync_{start,end} computation

[Golani, Mitulkumar Ajitkumar]

Thanks Ankit for the review,

> -----Original Message-----
> From: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> Sent: 18 November 2024 13:26
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>;
> intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com
> Subject: Re: [PATCH v3 1/3] drm/i915/vrr: Update vrr.vsync_{start,end}
> computation
> 
> 
> On 10/21/2024 4:20 PM, Mitul Golani wrote:
> > vrr.vsync_{start,end} computation should not depend on
> > crtc_state->vrr.enable. Also add them to state dump.
> >
> > --v1:
> >   - Explain commit message more clearly [Jani]
> >   - Instead of tweaking to fastset use vrr.flipline while computing AS_SDP.
> > --v2:
> >   - Correct computation of vrr.vsync_start/end should not depend on
> >     vrr.enable.[ville]
> >   - vrr enable disable requirement should not obstruct by SDP enable
> >     disable requirements. [Ville]
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> > ---
> >   .../drm/i915/display/intel_crtc_state_dump.c  |  4 +++-
> >   drivers/gpu/drm/i915/display/intel_vrr.c      | 24 +++++++------------
> >   2 files changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > index 705ec5ad385c..296a6c9de1f7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > @@ -303,7 +303,9 @@ void intel_crtc_state_dump(const struct
> intel_crtc_state *pipe_config,
> >   		   pipe_config->vrr.flipline,
> >   		   intel_vrr_vmin_vblank_start(pipe_config),
> >   		   intel_vrr_vmax_vblank_start(pipe_config));
> > -
> > +	drm_printf(&p, "vrr_vsync_start: %d, vrr_vsync_end: %d\n",
> > +		   pipe_config->vrr.vsync_start,
> > +		   pipe_config->vrr.vsync_end);
> 
> I think it would be good to have this in same line as other vrr parameters.
> 
> Also lets have this in a separate patch.

Agreed, I will add to separate patch in single line. 

> 
> >   	drm_printf(&p, "requested mode: " DRM_MODE_FMT "\n",
> >   		   DRM_MODE_ARG(&pipe_config->hw.mode));
> >   	drm_printf(&p, "adjusted mode: " DRM_MODE_FMT "\n", diff --git
> > a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 19a5d0076bb8..89696243f320 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -236,7 +236,7 @@ intel_vrr_compute_config(struct intel_crtc_state
> *crtc_state,
> >   		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> >   	}
> >
> > -	if (intel_dp->as_sdp_supported && crtc_state->vrr.enable) {
> > +	if (HAS_AS_SDP(display)) {
> >   		crtc_state->vrr.vsync_start =
> >   			(crtc_state->hw.adjusted_mode.crtc_vtotal -
> >   			 crtc_state->hw.adjusted_mode.vsync_start);
> 
> This again should be separate patch where we are just using HAS_AS_SDP()
> in compute_config and get_config.
> 
> 
> > @@ -316,6 +316,11 @@ void intel_vrr_set_transcoder_timings(const struct
> intel_crtc_state *crtc_state)
> >   		       trans_vrr_ctl(crtc_state));
> >   	intel_de_write(display, TRANS_VRR_FLIPLINE(display,
> cpu_transcoder),
> >   		       crtc_state->vrr.flipline - 1);
> > +	if (HAS_AS_SDP(display))
> > +		intel_de_write(display,
> > +			       TRANS_VRR_VSYNC(display, cpu_transcoder),
> > +			       VRR_VSYNC_END(crtc_state->vrr.vsync_end) |
> > +			       VRR_VSYNC_START(crtc_state->vrr.vsync_start));
> 
> This should be yet another patch where we are setting VRR_VSYNC along
> with other vrr transcoder timings.
> 
> Along with removal from existing places in vrr_enable/disable.
> 
> Apart from splitting into separate patches, changes look good to me.
> 

Splitting this from transcoder timing update, in either way it can create state mismatch when source/sink supports AS SDP but vrr is not enabled from uapi. 

Is it ok to keep this as a single patch ?

> 
> Regards,
> 
> Ankit
> 
> 
> >   }
> >
> >   void intel_vrr_send_push(const struct intel_crtc_state *crtc_state)
> > @@ -352,12 +357,6 @@ void intel_vrr_enable(const struct intel_crtc_state
> *crtc_state)
> >   	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> >   		       TRANS_PUSH_EN);
> >
> > -	if (HAS_AS_SDP(display))
> > -		intel_de_write(display,
> > -			       TRANS_VRR_VSYNC(display, cpu_transcoder),
> > -			       VRR_VSYNC_END(crtc_state->vrr.vsync_end) |
> > -			       VRR_VSYNC_START(crtc_state->vrr.vsync_start));
> > -
> >   	if (crtc_state->cmrr.enable) {
> >   		intel_de_write(display, TRANS_VRR_CTL(display,
> cpu_transcoder),
> >   			       VRR_CTL_VRR_ENABLE |
> VRR_CTL_CMRR_ENABLE |
> > @@ -382,10 +381,6 @@ void intel_vrr_disable(const struct intel_crtc_state
> *old_crtc_state)
> >   				TRANS_VRR_STATUS(display,
> cpu_transcoder),
> >   				VRR_STATUS_VRR_EN_LIVE, 1000);
> >   	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0);
> > -
> > -	if (HAS_AS_SDP(display))
> > -		intel_de_write(display,
> > -			       TRANS_VRR_VSYNC(display, cpu_transcoder), 0);
> >   }
> >
> >   void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
> > @@ -425,10 +420,6 @@ void intel_vrr_get_config(struct intel_crtc_state
> *crtc_state)
> >
> TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
> >   		crtc_state->vrr.vmin = intel_de_read(display,
> >   						     TRANS_VRR_VMIN(display,
> cpu_transcoder)) + 1;
> > -	}
> > -
> > -	if (crtc_state->vrr.enable) {
> > -		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> >
> >   		if (HAS_AS_SDP(display)) {
> >   			trans_vrr_vsync =
> > @@ -440,4 +431,7 @@ void intel_vrr_get_config(struct intel_crtc_state
> *crtc_state)
> >   				REG_FIELD_GET(VRR_VSYNC_END_MASK,
> trans_vrr_vsync);
> >   		}
> >   	}
> > +
> > +	if (crtc_state->vrr.enable)
> > +		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> >   }