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

[Nautiyal, Ankit K]

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.

>   	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.


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;
>   }