Re: [PATCH 9/9] drm/i915/vrr: Clamp guardband as per hardware and timing constraints

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

On Tue, Sep 23, 2025 at 04:02:44PM +0530, Nautiyal, Ankit K wrote:
> 
> On 9/22/2025 4:27 PM, Ville Syrjälä wrote:
> > On Sun, Sep 21, 2025 at 10:05:35AM +0530, Ankit Nautiyal wrote:
> >> The maximum guardband value is constrained by two factors:
> >> - The actual vblank length minus set context latency (SCL)
> >> - The hardware register field width:
> >>    - 8 bits for ICL/TGL (VRR_CTL_PIPELINE_FULL_MASK -> max 255)
> >>    - 16 bits for ADL+ (XELPD_VRR_CTL_VRR_GUARDBAND_MASK -> max 65535)
> >>
> >> Remove the #FIXME and clamp the guardband to the maximum allowed value.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_vrr.c | 36 ++++++++++++++++++++----
> >>   1 file changed, 30 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> index 5fa86356a791..9bed273f96df 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> @@ -409,6 +409,34 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> >>   	}
> >>   }
> >>   
> >> +static int intel_vrr_max_hw_guardband(const struct intel_crtc_state *crtc_state)
> >> +{
> >> +#define VRR_GUARDBAND_MAX 65535   /* based on XELPD_VRR_CTL_VRR_GUARDBAND_MASK */
> >> +#define VRR_PIPELINE_FULL_MAX 255 /* based on VRR_CTL_PIPELINE_FULL_MASK */
> > Magic numbers aren't great.
> >
> > We can get those straight from the register definitions:
> >   REG_FIELD_GET(XELPD_VRR_CTL_VRR_GUARDBAND_MASK, XELPD_VRR_CTL_VRR_GUARDBAND_MASK)
> >   REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, VRR_CTL_PIPELINE_FULL_MASK)
> >
> > or perhaps
> >   REG_FIELD_GET(XELPD_VRR_CTL_VRR_GUARDBAND_MASK, ~0)
> >   REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, ~0)
> > to be a bit less repetitive.
> >
> > Hmm, yeah I like that second form since it seems harder
> > to screw up the masks that way. I suppose we could even
> > formalize this sort of stuff into a REG_FIELD_MAX() macro...
> 
> 
> I was thinking to have a wrapper REG_FIELD_MAX() using FIELD_MAX defined 
> bitfield.h
> 
> Or should we have  REG_FIELD_MAX(mask)    REG_FIELD_GET(max, ~0) as 
> suggested?

If FIELD_MAX() already exists and does what we need then we can
use it. I suppose we do want our own wrapper for it for consistency
with names, and I guess we also want the same (u32) cast all our
other macros have.

> 
> 
> >
> >
> >> +	struct intel_display *display = to_intel_display(crtc_state);
> >> +
> >> +	if (!HAS_VRR(display))
> >> +		return 0;
> > No one should be calling this in that case.
> >
> >> +
> >> +	if (DISPLAY_VER(display) >= 13)
> >> +		return VRR_GUARDBAND_MAX;
> >> +
> >> +	return intel_vrr_pipeline_full_to_guardband(crtc_state, VRR_PIPELINE_FULL_MAX);
> >> +}
> >> +
> >> +static int clamp_guardband(struct intel_crtc_state *crtc_state, int guardband)
> >> +{
> >> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> >> +	int vblank_length = adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay;
> >> +	int set_context_latency = crtc_state->set_context_latency;
> >> +	int max_hw_guardband = intel_vrr_max_hw_guardband(crtc_state);
> >> +	int max_guardband;
> >> +
> >> +	max_guardband = min(max_hw_guardband, vblank_length - set_context_latency);
> >> +
> >> +	return min(guardband, max_guardband);
> > You are missing intel_vrr_extra_vblank_delay() here.
> >
> > To reduce the clutter I'd pull the max guardband (in terms
> > of the vblank length) calculation into a separate function:
> >
> > intel_vrr_max_guardband()
> > {
> > 	return vmin - vdisplay - extra - scl;
> > }
> >
> > Or maybe call it something like intel_vrr_max_vblank_guardband().
> >
> > And then we could have a
> >
> > intel_vrr_max_guardband()
> > {
> > 	return min(intel_vrr_max_vblank_guardband(), intel_vrr_max_hw_guardband());
> > }
> >
> > to give the final number.
> >
> >> +}
> >> +
> >>   void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> >>   {
> >>   	struct intel_display *display = to_intel_display(crtc_state);
> >> @@ -421,16 +449,12 @@ void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> >>   		crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
> >>   		intel_vrr_extra_vblank_delay(display);
> >>   
> > I think the initial guardband value here we could change to be
> > simply 'vmin - crtc_vdisplay' (until we start to optimize it).
> > That way all the hw details and whatnot will be handled by
> > intel_vrr_max_guardband().
> >
> > So in the end this could be just
> > guardband = min(vmin - crtc_vdisplay,
> > 		intel_vrr_max_guardband());
> 
> 
> Sure, will make the changes as suggested.
> 
> 
> Regards,
> 
> Ankit
> 
> >
> >
> >> -	if (DISPLAY_VER(display) < 13) {
> >> -		/* FIXME handle the limit in a proper way */
> >> -		crtc_state->vrr.guardband =
> >> -			min(crtc_state->vrr.guardband,
> >> -			    intel_vrr_pipeline_full_to_guardband(crtc_state, 255));
> >> +	crtc_state->vrr.guardband = clamp_guardband(crtc_state, crtc_state->vrr.guardband);
> >>   
> >> +	if (DISPLAY_VER(display) < 13)
> >>   		crtc_state->vrr.pipeline_full =
> >>   			intel_vrr_guardband_to_pipeline_full(crtc_state,
> >>   							     crtc_state->vrr.guardband);
> >> -	}
> >>   }
> >>   
> >>   static u32 trans_vrr_ctl(const struct intel_crtc_state *crtc_state)
> >> -- 
> >> 2.45.2

-- 
Ville Syrjälä
Intel