On Tue, Sep 09, 2025 at 08:32:11PM +0530, Nautiyal, Ankit K wrote:
>
> On 9/8/2025 10:13 PM, Ville Syrjälä wrote:
> > On Sun, Sep 07, 2025 at 01:02:39PM +0530, Ankit Nautiyal wrote:
> >> In the current VRR implementation, vrr.vmin and vrr.guardband are set such
> >> that they do not need to change when switching from fixed refresh rate to
> >> variable refresh rate. Specifically, vrr.guardband is always set to match
> >> the vblank length. This approach works for most cases, but not for LRR,
> >> where the guardband would need to change while the VRR timing generator is
> >> still active.
> >>
> >> With the VRR TG always active, live updates to guardband are unsafe and not
> >> recommended. To ensure hardware safety, guardband was moved out of the
> >> !fastset block, meaning any change now requires a full modeset.
> >> This breaks seamless LRR switching, which was previously supported.
> >>
> >> Since the problem arises from guardband being matched to the vblank length,
> >> solution is to use a minimal, sufficient static value, instead. So we use a
> >> static guardband defined during mode-set that fits within the smallest
> >> expected vblank and remains unchanged in case of features like LRR where
> >> vtotal changes. To compute this minimum guardband we take into account
> >> latencies/delays due to different features as mentioned in the Bspec.
> >>
> >> v2:
> >> -Use helpers for dsc/scaler prefill latencies. (Mitul)
> >> -Account for pkgc latency and take max of pkgc and sagv latencies.
> >> v3: Use new helper for PSR2/Panel Replay latency.
> >> v4: Avoid re-setting the Vmin/Flipline for optimized guardband.
> >>
> >> Bspec: 70151
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nauti...@intel.com>
> >> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.gol...@intel.com> (#v3)
> >> ---
> >> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> >> drivers/gpu/drm/i915/display/intel_vrr.c | 127 ++++++++++++++++++-
> >> drivers/gpu/drm/i915/display/intel_vrr.h | 3 +-
> >> 3 files changed, 128 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index fb072275b1c7..3fa94675d5e1 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -4902,7 +4902,6 @@ intel_modeset_pipe_config_late(struct
> >> intel_atomic_state *state,
> >> struct drm_connector *connector;
> >> int i;
> >>
> >> - intel_vrr_compute_config_late(crtc_state);
> >>
> >> for_each_new_connector_in_state(&state->base, connector,
> >> conn_state, i) {
> >> @@ -4914,6 +4913,7 @@ intel_modeset_pipe_config_late(struct
> >> intel_atomic_state *state,
> >> !encoder->compute_config_late)
> >> continue;
> >>
> >> + intel_vrr_compute_config_late(crtc_state, conn_state);
> >> ret = encoder->compute_config_late(encoder, crtc_state,
> >> conn_state);
> >> if (ret)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> index 855974174afd..fff684eb2514 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> @@ -6,12 +6,15 @@
> >>
> >> #include <drm/drm_print.h>
> >>
> >> +#include "intel_alpm.h"
> >> #include "intel_de.h"
> >> #include "intel_display_regs.h"
> >> #include "intel_display_types.h"
> >> #include "intel_dp.h"
> >> #include "intel_vrr.h"
> >> #include "intel_vrr_regs.h"
> >> +#include "skl_scaler.h"
> >> +#include "skl_watermark.h"
> >>
> >> #define FIXED_POINT_PRECISION 100
> >> #define CMRR_PRECISION_TOLERANCE 10
> >> @@ -413,15 +416,135 @@ intel_vrr_compute_config(struct intel_crtc_state
> >> *crtc_state,
> >> }
> >> }
> >>
> >> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> >> +static
> >> +int scaler_prefill_latency(struct intel_crtc_state *crtc_state, int
> >> linetime_us)
> >> +{
> >> + int chroma_downscaling_factor =
> >> skl_scaler_chroma_downscale_factor(crtc_state);
> >> + u64 hscale_k, vscale_k;
> >> + int cdclk_adjustment;
> >> + int num_scaler_users;
> >> +
> >> + /*
> >> + * Assuming:
> >> + * Both scaler enabled.
> >> + * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
> >> + * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
> >> + * Cdclk Adjustment : 1
> >> + */
> >> + num_scaler_users = 2;
> >> + hscale_k = 2 * 1000;
> >> + vscale_k = 2 * 1000;
> >> + cdclk_adjustment = 1;
> >> +
> >> + return intel_vrr_guardband_scaler_latency(num_scaler_users, hscale_k,
> >> vscale_k,
> >> + chroma_downscaling_factor,
> >> + cdclk_adjustment,
> >> + linetime_us);
> >> +}
> >> +
> >> +static
> >> +int dsc_prefill_latency(struct intel_crtc_state *crtc_state, int
> >> linetime_us)
> >> +{
> >> +#define MAX_SCALERS 2
> >> + int chroma_downscaling_factor =
> >> skl_scaler_chroma_downscale_factor(crtc_state);
> >> + u64 hscale_k[MAX_SCALERS], vscale_k[MAX_SCALERS];
> >> + int cdclk_adjustment;
> >> + int num_scaler_users;
> >> +
> >> + /*
> >> + * Assuming:
> >> + * Both scaler enabled.
> >> + * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
> >> + * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
> >> + * Cdclk Adjustment : 1
> >> + */
> >> + num_scaler_users = MAX_SCALERS;
> >> + hscale_k[0] = 2 * 1000;
> >> + vscale_k[0] = 2 * 1000;
> >> + hscale_k[1] = 2 * 1000;
> >> + vscale_k[1] = 1 * 1000;
> >> +
> >> + cdclk_adjustment = 1;
> >> +
> >> + return intel_vrr_guardband_dsc_latency(num_scaler_users, hscale_k,
> >> vscale_k,
> >> + chroma_downscaling_factor,
> >> + cdclk_adjustment,
> >> + linetime_us);
> >> +}
> >> +
> >> +static
> >> +int intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state,
> >> + struct intel_connector *connector)
> >> +{
> >> + const struct drm_display_mode *adjusted_mode =
> >> &crtc_state->hw.adjusted_mode;
> >> + struct intel_display *display = to_intel_display(crtc_state);
> >> + int dsc_prefill_time = 0;
> >> + int psr2_pr_latency = 0;
> >> + int scaler_prefill_time;
> >> + int wm0_prefill_time;
> >> + int pkgc_max_latency;
> >> + int sagv_latency;
> >> + int sdp_latency = 0;
> >> + int guardband_us;
> >> + int linetime_us;
> >> + int guardband;
> >> + int pm_delay;
> >> +
> >> + linetime_us = DIV_ROUND_UP(adjusted_mode->crtc_htotal * 1000,
> >> + adjusted_mode->crtc_clock);
> >> +
> >> + pkgc_max_latency = skl_watermark_max_latency(display, 1);
> >> + sagv_latency = display->sagv.block_time_us;
> >> +
> >> + /* Assuming max wm0 lines = 4 */
> >> + wm0_prefill_time = 4 * linetime_us + 20;
> >> +
> >> + scaler_prefill_time = scaler_prefill_latency(crtc_state, linetime_us);
> >> +
> >> + if (crtc_state->dsc.compression_enable)
> >> + dsc_prefill_time = dsc_prefill_latency(crtc_state, linetime_us);
> >> +
> >> + pm_delay = crtc_state->framestart_delay +
> >> + max(sagv_latency, pkgc_max_latency) +
> >> + wm0_prefill_time +
> >> + scaler_prefill_time +
> >> + dsc_prefill_time;
> >> +
> >> + switch (connector->base.connector_type) {
> >> + case DRM_MODE_CONNECTOR_eDP:
> >> + case DRM_MODE_CONNECTOR_DisplayPort:
> >> + psr2_pr_latency =
> >> intel_alpm_compute_max_link_wake_latency(crtc_state, true);
> >> + sdp_latency = intel_dp_compute_sdp_latency(crtc_state, true);
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> +
> >> + guardband_us = max(sdp_latency, psr2_pr_latency);
> >> + guardband_us = max(guardband_us, pm_delay);
> >> +
> >> + guardband = DIV_ROUND_UP(guardband_us, linetime_us);
> >> +
> >> + /* guardband cannot be more than the Vmax vblank */
> >> + guardband = min(guardband, crtc_state->vrr.vmax -
> >> adjusted_mode->crtc_vblank_start);
> >> +
> >> + return guardband;
> >> +}
> >> +
> >> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state,
> >> + struct drm_connector_state *conn_state)
> >> {
> >> struct intel_display *display = to_intel_display(crtc_state);
> >> const struct drm_display_mode *adjusted_mode =
> >> &crtc_state->hw.adjusted_mode;
> >> + struct intel_connector *connector =
> >> + to_intel_connector(conn_state->connector);
> >>
> >> if (!intel_vrr_possible(crtc_state))
> >> return;
> >>
> >> - if (DISPLAY_VER(display) >= 13) {
> >> + if (intel_vrr_always_use_vrr_tg(display)) {
> >> + crtc_state->vrr.guardband =
> >> intel_vrr_compute_guardband(crtc_state, connector);
> > This all looks to be in the wrong place. It needs to be done as part of
> > intel_crtc_compute_vblank_delay() which then updates crtc_vblank_start
> > to reflect reality instead of leaving some incorrect junk in there.
> > Presumably that is why you also had to do that
> > intel_dsb_wait_for_delayed_vblank() hack.
>
> Hi Ville,
>
> Thanks for for the comments.
>
> Looks like intel_crtc_compute_vblank_delay() indeed is the place to add
> the vblank_delay and get the crtc_vblank_start point to the delayed vblank.
>
> But I think, there are couple of things need to take care with this change:
> 1) The SCL currently is derived from crtc_vblank_start - vdisplay, which
> is ~1 with guradband = vblank length.
> With modified crtc_vblank_start, this will become huge so perhaps we
> need to set it to 1?
When using the legacy timing generator SCL is what defines the
position of the delayed vblank. So it should be exactly what it is.
I don't quite recall how this stuff works with the VRR timing
generator. I think it might have been that guardband more or less
defines the location of the delayed vblank, and SCL then effectively
defines the start of the safe window. So I think it should stay
exactly the way it is. IIRC there is some way on new platforms to
even move the SCL position into the vertical active, but I haven't
though through what that would actually do for us.
>
> 2) The intel_dsb_wait_vblank_delay() (which inturn uses
> intel_vrr_real_vblank_delay() for vrr case) also computes a fixed delay
> based on (crtc_vblank_start - vdisplay) lines
> Instead of this should we not use intel_dsb_wait_scanline_in() to wait
> only till scanline reaches in range [flipline decision boundary, vmax
> decision boundary]?
You can't use absolute line numbers with VRR since you don't know
when the vblank will be terminated. It has to be all relative.
>
> Let me know what you think, I can send a follow-up patch with the
> suggested changes.
>
> Regards,
> Ankit
>
>
> >
> >> + } else if (DISPLAY_VER(display) >= 13) {
> >> crtc_state->vrr.guardband =
> >> crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
> >> } else {
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h
> >> b/drivers/gpu/drm/i915/display/intel_vrr.h
> >> index 950041647e47..362638fd0d66 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> >> @@ -21,7 +21,8 @@ bool intel_vrr_possible(const struct intel_crtc_state
> >> *crtc_state);
> >> void intel_vrr_check_modeset(struct intel_atomic_state *state);
> >> void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> >> struct drm_connector_state *conn_state);
> >> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
> >> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state,
> >> + struct drm_connector_state *conn_state);
> >> void intel_vrr_set_transcoder_timings(const struct intel_crtc_state
> >> *crtc_state);
> >> void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
> >> void intel_vrr_send_push(struct intel_dsb *dsb,
> >> --
> >> 2.45.2
--
Ville Syrjälä
Intel
