On Wed, Sep 20, 2023 at 12:40 PM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 20, 2023 at 11:47:05AM -0700, Manasi Navare wrote:
> > Hi Ville,
> >
> > Quick question here on the use case and the trigger for the LRR case
> > which is within VRR range.
> > Could this perhaps be used if we had a virtual mode say 40Hz that now
> > falls in the VRR range (30 -120Hz) that is
> > exposed through the connector mode list and then if we do a modest to
> > 40Hz that would make update_lrr = true within VRR and
> > hand adjust the vtotal to that exact value?
> > I am looking at adding this virtual mode to DRM soon, wondering if
> > this would be how the kernel would actual set the timings for it.
>
> Userspace can supply any mode it wants. So just take whatever higher
> refresh rate mode you have and increase vtotal until you reach
> the desired lower vrefresh rate and feed that to the kernel.
Okay perfect that makes sense.
So effectively if i create specific modes lower than the highest
refresh rate, then
userspace can request a modeset to any of the lower refresh rate virtual mode
and the kernel would stretch out the vtotal manually corresponding to
the vtotal of
that virtual mode timing and modeset to that seamlessly in a fastset fashion.
Is this correct?
Regards
Manasi
>
> >
> > Regards
> > Manasi
> >
> > On Mon, Sep 18, 2023 at 4:16 PM Manasi Navare <navaremanasi@xxxxxxxxxxxx> wrote:
> > >
> > > Thanks Ville for the respin, the changes look good now.
> > >
> > > Reviewed-by: Manasi Navare <navaremanasi@xxxxxxxxxxxx>
> > >
> > > Manasi
> > >
> > >
> > > On Fri, Sep 15, 2023 at 3:38 AM Ville Syrjala
> > > <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > Implement low refresh rate (LRR) where we change the vblank
> > > > length by hand as requested, but otherwise keep the timing
> > > > generator running in non-VRR mode (ie. fixed refresh rate).
> > > >
> > > > The panel itself must support VRR for this to work, and
> > > > only TGL+ has the double buffred TRANS_VTOTAL.VTOTAL that
> > > > we need to make the switch properly. The double buffer
> > > > latching happens at the start of transcoders undelayed
> > > > vblank. The other thing that we change is
> > > > TRANS_VBLANK.VBLANK_END but the hardware entirely ignores
> > > > that in DP mode. But I decided to keep writing it anyway
> > > > just to avoid more special cases in readout/state check.
> > > >
> > > > v2: Document that TRANS_VBLANK.VBLANK_END is ignored by
> > > > the hardware
> > > > v3: Reconcile with VRR fastset
> > > > Adjust update_lrr flag behaviour
> > > > Make sure timings stay within VRR range
> > > > v4: Fix up update_m_n vs. update_lrr rebase fail (Manasi)
> > > > Drop DOUBLE_BUFFER_VACTIVE define as it's not needed (Manasi)
> > > >
> > > > TODO: Hook LRR into the automatic DRRS downclocking stuff?
> > > >
> > > > Cc: Manasi Navare <navaremanasi@xxxxxxxxxxxx>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_atomic.c | 1 +
> > > > drivers/gpu/drm/i915/display/intel_crtc.c | 9 +--
> > > > drivers/gpu/drm/i915/display/intel_display.c | 60 +++++++++++++++++--
> > > > .../drm/i915/display/intel_display_device.h | 1 +
> > > > .../drm/i915/display/intel_display_types.h | 3 +-
> > > > drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++-
> > > > 6 files changed, 70 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index aaddd8c0cfa0..5d18145da279 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -260,6 +260,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> > > >
> > > > crtc_state->update_pipe = false;
> > > > crtc_state->update_m_n = false;
> > > > + crtc_state->update_lrr = false;
> > > > crtc_state->disable_lp_wm = false;
> > > > crtc_state->disable_cxsr = false;
> > > > crtc_state->update_wm_pre = false;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > index a39e31c1ca85..22e85fe7e8aa 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > @@ -495,7 +495,7 @@ static void intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state,
> > > > if (crtc->mode_flags & I915_MODE_FLAG_VRR) {
> > > > /* timing changes should happen with VRR disabled */
> > > > drm_WARN_ON(state->base.dev, intel_crtc_needs_modeset(new_crtc_state) ||
> > > > - new_crtc_state->update_m_n);
> > > > + new_crtc_state->update_m_n || new_crtc_state->update_lrr);
> > > >
> > > > if (intel_vrr_is_push_sent(crtc_state))
> > > > *vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> > > > @@ -511,10 +511,11 @@ static void intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state,
> > > > *max = *vblank_start - 1;
> > > >
> > > > /*
> > > > - * M/N is double buffered on the transcoder's undelayed vblank,
> > > > - * so with seamless M/N we must evade both vblanks.
> > > > + * M/N and TRANS_VTOTAL are double buffered on the transcoder's
> > > > + * undelayed vblank, so with seamless M/N and LRR we must evade
> > > > + * both vblanks.
> > > > */
> > > > - if (new_crtc_state->update_m_n)
> > > > + if (new_crtc_state->update_m_n || new_crtc_state->update_lrr)
> > > > *min -= adjusted_mode->crtc_vblank_start - adjusted_mode->crtc_vdisplay;
> > > > }
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index f0bb5c70ebfc..988558ccf794 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -928,7 +928,7 @@ static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
> > > > {
> > > > return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > > > (new_crtc_state->vrr.enable &&
> > > > - (new_crtc_state->update_m_n ||
> > > > + (new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
> > > > vrr_params_changed(old_crtc_state, new_crtc_state)));
> > > > }
> > > >
> > > > @@ -937,7 +937,7 @@ static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> > > > {
> > > > return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > > > (old_crtc_state->vrr.enable &&
> > > > - (new_crtc_state->update_m_n ||
> > > > + (new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
> > > > vrr_params_changed(old_crtc_state, new_crtc_state)));
> > > > }
> > > >
> > > > @@ -2586,6 +2586,37 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
> > > > VTOTAL(crtc_vtotal - 1));
> > > > }
> > > >
> > > > +static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > + const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> > > > + u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
> > > > +
> > > > + crtc_vdisplay = adjusted_mode->crtc_vdisplay;
> > > > + crtc_vtotal = adjusted_mode->crtc_vtotal;
> > > > + crtc_vblank_start = adjusted_mode->crtc_vblank_start;
> > > > + crtc_vblank_end = adjusted_mode->crtc_vblank_end;
> > > > +
> > > > + drm_WARN_ON(&dev_priv->drm, adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE);
> > > > +
> > > > + /*
> > > > + * The hardware actually ignores TRANS_VBLANK.VBLANK_END in DP mode.
> > > > + * But let's write it anyway to keep the state checker happy.
> > > > + */
> > > > + intel_de_write(dev_priv, TRANS_VBLANK(cpu_transcoder),
> > > > + VBLANK_START(crtc_vblank_start - 1) |
> > > > + VBLANK_END(crtc_vblank_end - 1));
> > > > + /*
> > > > + * The double buffer latch point for TRANS_VTOTAL
> > > > + * is the transcoder's undelayed vblank.
> > > > + */
> > > > + intel_de_write(dev_priv, TRANS_VTOTAL(cpu_transcoder),
> > > > + VACTIVE(crtc_vdisplay - 1) |
> > > > + VTOTAL(crtc_vtotal - 1));
> > > > +}
> > > > +
> > > > static void intel_set_pipe_src_size(const struct intel_crtc_state *crtc_state)
> > > > {
> > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > @@ -5082,11 +5113,13 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > > PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
> > > > PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
> > > > PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
> > > > - PIPE_CONF_CHECK_I(name.crtc_vtotal); \
> > > > PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
> > > > - PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
> > > > PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
> > > > PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
> > > > + if (!fastset || !pipe_config->update_lrr) { \
> > > > + PIPE_CONF_CHECK_I(name.crtc_vtotal); \
> > > > + PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
> > > > + } \
> > > > } while (0)
> > > >
> > > > #define PIPE_CONF_CHECK_RECT(name) do { \
> > > > @@ -5420,6 +5453,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
> > > > crtc_state->uapi.mode_changed = true;
> > > > crtc_state->update_pipe = false;
> > > > crtc_state->update_m_n = false;
> > > > + crtc_state->update_lrr = false;
> > > >
> > > > ret = drm_atomic_add_affected_connectors(&state->base,
> > > > &crtc->base);
> > > > @@ -5537,6 +5571,10 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > > {
> > > > struct drm_i915_private *i915 = to_i915(old_crtc_state->uapi.crtc->dev);
> > > >
> > > > + /* only allow LRR when the timings stay within the VRR range */
> > > > + if (old_crtc_state->vrr.in_range != new_crtc_state->vrr.in_range)
> > > > + new_crtc_state->update_lrr = false;
> > > > +
> > > > if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true))
> > > > drm_dbg_kms(&i915->drm, "fastset requirement not met, forcing full modeset\n");
> > > > else
> > > > @@ -5547,6 +5585,11 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > > &new_crtc_state->dp_m_n))
> > > > new_crtc_state->update_m_n = false;
> > > >
> > > > + if (intel_crtc_needs_modeset(new_crtc_state) ||
> > > > + (old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> > > > + old_crtc_state->hw.adjusted_mode.crtc_vblank_end == new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
> > > > + new_crtc_state->update_lrr = false;
> > > > +
> > > > if (!intel_crtc_needs_modeset(new_crtc_state))
> > > > new_crtc_state->update_pipe = true;
> > > > }
> > > > @@ -6262,6 +6305,7 @@ int intel_atomic_check(struct drm_device *dev,
> > > > new_crtc_state->uapi.mode_changed = true;
> > > > new_crtc_state->update_pipe = false;
> > > > new_crtc_state->update_m_n = false;
> > > > + new_crtc_state->update_lrr = false;
> > > > }
> > > > }
> > > >
> > > > @@ -6275,6 +6319,7 @@ int intel_atomic_check(struct drm_device *dev,
> > > > new_crtc_state->uapi.mode_changed = true;
> > > > new_crtc_state->update_pipe = false;
> > > > new_crtc_state->update_m_n = false;
> > > > + new_crtc_state->update_lrr = false;
> > > > }
> > > > }
> > > >
> > > > @@ -6283,6 +6328,7 @@ int intel_atomic_check(struct drm_device *dev,
> > > > new_crtc_state->uapi.mode_changed = true;
> > > > new_crtc_state->update_pipe = false;
> > > > new_crtc_state->update_m_n = false;
> > > > + new_crtc_state->update_lrr = false;
> > > > }
> > > > }
> > > > }
> > > > @@ -6464,6 +6510,9 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
> > > > if (new_crtc_state->update_m_n)
> > > > intel_cpu_transcoder_set_m1_n1(crtc, new_crtc_state->cpu_transcoder,
> > > > &new_crtc_state->dp_m_n);
> > > > +
> > > > + if (new_crtc_state->update_lrr)
> > > > + intel_set_transcoder_timings_lrr(new_crtc_state);
> > > > }
> > > >
> > > > static void commit_pipe_pre_planes(struct intel_atomic_state *state,
> > > > @@ -6600,7 +6649,8 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> > > > *
> > > > * FIXME Should be synchronized with the start of vblank somehow...
> > > > */
> > > > - if (vrr_enabling(old_crtc_state, new_crtc_state) || new_crtc_state->update_m_n)
> > > > + if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> > > > + new_crtc_state->update_m_n || new_crtc_state->update_lrr)
> > > > intel_crtc_update_active_timings(new_crtc_state,
> > > > new_crtc_state->vrr.enable);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > > index 8198401aa5be..ee77750af82b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > > @@ -56,6 +56,7 @@ struct drm_printer;
> > > > #define HAS_HW_SAGV_WM(i915) (DISPLAY_VER(i915) >= 13 && !IS_DGFX(i915))
> > > > #define HAS_IPC(i915) (DISPLAY_INFO(i915)->has_ipc)
> > > > #define HAS_IPS(i915) (IS_HASWELL_ULT(i915) || IS_BROADWELL(i915))
> > > > +#define HAS_LRR(i915) (DISPLAY_VER(i915) >= 12)
> > > > #define HAS_LSPCON(i915) (IS_DISPLAY_VER(i915, 9, 10))
> > > > #define HAS_MBUS_JOINING(i915) (IS_ALDERLAKE_P(i915) || DISPLAY_VER(i915) >= 14)
> > > > #define HAS_MSO(i915) (DISPLAY_VER(i915) >= 12)
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 2f35560d7e4e..536c642eb562 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1084,6 +1084,7 @@ struct intel_crtc_state {
> > > > unsigned fb_bits; /* framebuffers to flip */
> > > > bool update_pipe; /* can a fast modeset be performed? */
> > > > bool update_m_n; /* update M/N seamlessly during fastset? */
> > > > + bool update_lrr; /* update TRANS_VTOTAL/etc. during fastset? */
> > > > bool disable_cxsr;
> > > > bool update_wm_pre, update_wm_post; /* watermarks are updated */
> > > > bool fifo_changed; /* FIFO split is changed */
> > > > @@ -1383,7 +1384,7 @@ struct intel_crtc_state {
> > > >
> > > > /* Variable Refresh Rate state */
> > > > struct {
> > > > - bool enable;
> > > > + bool enable, in_range;
> > > > u8 pipeline_full;
> > > > u16 flipline, vmin, vmax, guardband;
> > > > } vrr;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > index 12731ad725a8..5d905f932cb4 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > @@ -120,9 +120,14 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> > > > if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > > return;
> > > >
> > > > - if (!intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode)))
> > > > + crtc_state->vrr.in_range =
> > > > + intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
> > > > + if (!crtc_state->vrr.in_range)
> > > > return;
> > > >
> > > > + if (HAS_LRR(i915))
> > > > + crtc_state->update_lrr = true;
> > > > +
> > > > vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
> > > > adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
> > > > vmax = adjusted_mode->crtc_clock * 1000 /
> > > > --
> > > > 2.41.0
> > > >
>
> --
> Ville Syrjälä
> Intel
