On Thu, Sep 18, 2025 at 03:07:20PM +0530, Nautiyal, Ankit K wrote:
>
> On 9/18/2025 2:04 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä <[email protected]>
> >
> > While ICL/TGL VRR hardware doesn't have a register for the guardband
> > value, our lives will be simpler if we pretend that it does. Start
> > by computing the guardband the same as on ADL+ and storing it in
> > the state, and only then we convert it into the corresponding
> > pipeline_full value that the hardware can consume. During readout we
> > do the opposite.
> >
> > I was debating whether to completely remove pipeline_full from the
> > crtc state, but decided to keep it for now. Mainly because we check
> > it in vrr_params_changed() and simply checking the guardband instead
> > isn't 100% equivalent; Theoretically, framestart_delay may have
> > changed in the opposite direction to pipeline_full, keeping the
> > derived guardband value unchaged. One solution would be to also check
> > framestart_delay, but that feels a bit leaky abstraction wise.
> >
> > Also note that we don't currently handle the maximum limit of 255
> > scanlines for the pipeline_full in a very nice way. The actual
> > position of the delayed vblank will move because of that clamping,
> > and so some of our code may get confused. But fixing this shall
> > wait a for now.
> >
> > Signed-off-by: Ville Syrjälä <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 1 +
> > drivers/gpu/drm/i915/display/intel_vrr.c | 36 +++++++++++---------
> > 2 files changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index c7d85fd38890..f4124c79bc83 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -3891,6 +3891,7 @@ static bool hsw_get_pipe_config(struct intel_crtc
> > *crtc,
> > intel_joiner_get_config(pipe_config);
> > intel_dsc_get_config(pipe_config);
> >
> > + /* intel_vrr_get_config() depends on .framestart_delay */
> > if (!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> > tmp = intel_de_read(display, CHICKEN_TRANS(display,
> > pipe_config->cpu_transcoder));
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 5fee85b0bc99..9cdcc2558ead 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -151,13 +151,7 @@ static int intel_vrr_pipeline_full_to_guardband(const
> > struct intel_crtc_state *c
> > */
> > static int intel_vrr_vblank_exit_length(const struct intel_crtc_state
> > *crtc_state)
> > {
> > - struct intel_display *display = to_intel_display(crtc_state);
> > -
> > - if (DISPLAY_VER(display) >= 13)
> > - return crtc_state->vrr.guardband;
> > - else
> > - return intel_vrr_pipeline_full_to_guardband(crtc_state,
> > -
> > crtc_state->vrr.pipeline_full);
> > + return crtc_state->vrr.guardband;
> > }
> >
> > int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state)
> > @@ -431,18 +425,22 @@ void intel_vrr_compute_config_late(struct
> > intel_crtc_state *crtc_state)
> > {
> > struct intel_display *display = to_intel_display(crtc_state);
> > const struct drm_display_mode *adjusted_mode =
> > &crtc_state->hw.adjusted_mode;
> > - int guardband;
> >
> > if (!intel_vrr_possible(crtc_state))
> > return;
> >
> > - guardband = crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
> > + crtc_state->vrr.guardband =
> > + crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
> > +
> > + 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));
> >
> > - if (DISPLAY_VER(display) >= 13) {
> > - crtc_state->vrr.guardband = guardband;
> > - } else {
> > crtc_state->vrr.pipeline_full =
> > - min(255,
> > intel_vrr_guardband_to_pipeline_full(crtc_state, guardband));
> > + intel_vrr_guardband_to_pipeline_full(crtc_state,
> > +
> > crtc_state->vrr.guardband);
>
>
> For removing the #FIXME to handle the limit what can be required:
>
> Do we need to abstract it with intel_vrr_clamp_pipeline_full() or else
> we need:
I haven't though in detail how we should do this, but basically we
have two constraints that limit the max guardband:
- actual vblank_length-SCL
- hardware register limit (~255 for icl/tgl, (apparently)
65535 for for adl+)
So when calculating the guardband we just have to clamp it
the minimum of those.
>
> crtc_state->vrr.pipeline_full = min(255,
> intel_vrr_guardband_to_pipeline_full(crtc_state,
> crtc_state->vrr.guardband));
>
> crtc_state->vrr.guardband =
> intel_vrr_guardband_to_pipeline_full(crtc_state,
> crtc_state->vrr.pipeline_full);
>
> (Though this might be bit confusing since we use guardband to get
> pipline and again change guardband.)
>
>
> Regards,
>
> Ankit
>
>
> >
> > /*
> > * vmin/vmax/flipline also need to be adjusted by
> > @@ -734,14 +732,20 @@ void intel_vrr_get_config(struct intel_crtc_state
> > *crtc_state)
> > TRANS_CMRR_M_HI(display,
> > cpu_transcoder));
> > }
> >
> > - if (DISPLAY_VER(display) >= 13)
> > + if (DISPLAY_VER(display) >= 13) {
> > crtc_state->vrr.guardband =
> > REG_FIELD_GET(XELPD_VRR_CTL_VRR_GUARDBAND_MASK,
> > trans_vrr_ctl);
> > - else
> > - if (trans_vrr_ctl & VRR_CTL_PIPELINE_FULL_OVERRIDE)
> > + } else {
> > + if (trans_vrr_ctl & VRR_CTL_PIPELINE_FULL_OVERRIDE) {
> > crtc_state->vrr.pipeline_full =
> > REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK,
> > trans_vrr_ctl);
> >
> > + crtc_state->vrr.guardband =
> > + intel_vrr_pipeline_full_to_guardband(crtc_state,
> > +
> > crtc_state->vrr.pipeline_full);
> > + }
> > + }
> > +
> > if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) {
> > crtc_state->vrr.flipline = intel_de_read(display,
> >
> > TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;
--
Ville Syrjälä
Intel
