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.nauti...@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?
+ 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
