Re: [PATCH] drm/i915/vrr: Configure VRR timings after enabling TRANS_DDI_FUNC_CTL

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 3/3/2026 3:24 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Apparently ICL may hang with an MCE if we write TRANS_VRR_VMAX/FLIPLINE
> before enabling TRANS_DDI_FUNC_CTL.
>
> Personally I was only able to reproduce a hang (on an Dell XPS 7390
> 2-in-1) with an external display connected via a dock using a dodgy
> type-C cable that made the link training fail. After the failed
> link training the machine would hang. TGL seemed immune to the
> problem for whatever reason.
>
> BSpec does tell us to configure VRR after enabling TRANS_DDI_FUNC_CTL
> as well. The DMC firmware also does the VRR restore in two stages:
> - first stage seems to be unconditional and includes TRANS_VRR_CTL
>    and a few other VRR registers, among other things
> - second stage is conditional on the DDI being enabled,
>    and includes TRANS_DDI_FUNC_CTL and TRANS_VRR_VMAX/VMIN/FLIPLINE,
>    among other things
>
> So let's reorder the steps to match to avoid the hang, and
> toss in an extra WARN to make sure we don't screw this up later.
>
> BSpec: 22243
> Cc: stable@vger.kernel.org
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reported-by: Benjamin Tissoires <bentiss@kernel.org>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15777
> Tested-by: Benjamin Tissoires <bentiss@kernel.org>
> Fixes: dda7dcd9da73 ("drm/i915/vrr: Use fixed timings for platforms that support VRR")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

You are right. VRR timing registers are indeed supposed to be programmed 
after TRANS_DDI_FUNC_CTL.

Thanks for catching this, Ville, and thanks Benjamin for the bisection.

Change looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> ---
>   drivers/gpu/drm/i915/display/intel_display.c |  1 -
>   drivers/gpu/drm/i915/display/intel_vrr.c     | 14 ++++++++++++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 27354585ba92..138ee7dd1977 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1637,7 +1637,6 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>   	}
>   
>   	intel_set_transcoder_timings(crtc_state);
> -	intel_vrr_set_transcoder_timings(crtc_state);
>   
>   	if (cpu_transcoder != TRANSCODER_EDP)
>   		intel_de_write(display, TRANS_MULT(display, cpu_transcoder),
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 00ca76dbdd6c..8a957804cb97 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -599,6 +599,18 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>   	if (!HAS_VRR(display))
>   		return;
>   
> +	/*
> +	 * Bspec says:
> +	 * "(note: VRR needs to be programmed after
> +	 *  TRANS_DDI_FUNC_CTL and before TRANS_CONF)."
> +	 *
> +	 * In practice it turns out that ICL can hang if
> +	 * TRANS_VRR_VMAX/FLIPLINE are written before
> +	 * enabling TRANS_DDI_FUNC_CTL.
> +	 */
> +	drm_WARN_ON(display->drm,
> +		    !(intel_de_read(display, TRANS_DDI_FUNC_CTL(display, cpu_transcoder)) & TRANS_DDI_FUNC_ENABLE));
> +
>   	/*
>   	 * This bit seems to have two meanings depending on the platform:
>   	 * TGL: generate VRR "safe window" for DSB vblank waits
> @@ -961,6 +973,8 @@ void intel_vrr_transcoder_enable(const struct intel_crtc_state *crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(crtc_state);
>   
> +	intel_vrr_set_transcoder_timings(crtc_state);
> +
>   	if (!intel_vrr_possible(crtc_state))
>   		return;
>