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

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

On Wed, Mar 04, 2026 at 11:36:12AM +0530, Nautiyal, Ankit K wrote:
> 
> 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>

Pushed.

Thanks for the review, and the bug report.

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

-- 
Ville Syrjälä
Intel