[Intel-gfx] [PATCH 4/6] drm/i915: Enable VRR later during fastsets

[Manasi Navare]

On Sun, Aug 27, 2023 at 10:41 PM Ville Syrjala
<ville.syrjala at linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> In order to reconcile seamless M/N updates with VRR we'll
> need to defer the fastset VRR enable to happen after the
> seamless M/N update (which happens during the vblank evade
> critical section). So just push the VRR enable to be the last
> thing during the update.
>
> This will also affect the vblank evasion as the transcoder
> will now still be running with the old VRR state during
> the vblank evasion. So just grab the timings always from the
> old crtc state during any non-modeset commit, and also grab
> the current state of VRR from the active timings (as we disable
> VRR before vblank evasion during fastsets).
>
> This also fixes vblank evasion for seamless M/N updates as
> we now properly account for the fact that the M/N update
> happens after vblank evasion.
>
> Cc: Manasi Navare <navaremanasi at chromium.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c    | 35 ++++++++++++--------
>  drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++----
>  2 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index e46a15d59d79..1992e7060263 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -472,15 +472,31 @@ static void intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state,
>                                               struct intel_crtc *crtc,
>                                               int *min, int *max, int *vblank_start)
>  {
> +       const struct intel_crtc_state *old_crtc_state =
> +               intel_atomic_get_old_crtc_state(state, crtc);
>         const struct intel_crtc_state *new_crtc_state =
>                 intel_atomic_get_new_crtc_state(state, crtc);
> -       const struct drm_display_mode *adjusted_mode = &new_crtc_state->hw.adjusted_mode;
> +       const struct intel_crtc_state *crtc_state;
> +       const struct drm_display_mode *adjusted_mode;
>
> -       if (new_crtc_state->vrr.enable) {
> -               if (intel_vrr_is_push_sent(new_crtc_state))
> -                       *vblank_start = intel_vrr_vmin_vblank_start(new_crtc_state);
> +       /*
> +        * During fastsets/etc. the transcoder is still
> +        * running with the old timings at this point.
> +        *
> +        * TODO: maybe just use the active timings here?
> +        */
> +       if (intel_crtc_needs_modeset(new_crtc_state))
> +               crtc_state = new_crtc_state;
> +       else
> +               crtc_state = old_crtc_state;
> +
> +       adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +       if (crtc->mode_flags & I915_MODE_FLAG_VRR) {
> +               if (intel_vrr_is_push_sent(crtc_state))
> +                       *vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
>                 else
> -                       *vblank_start = intel_vrr_vmax_vblank_start(new_crtc_state);
> +                       *vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
>         } else {
>                 *vblank_start = intel_mode_vblank_start(adjusted_mode);
>         }
> @@ -710,15 +726,6 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
>          */
>         intel_vrr_send_push(new_crtc_state);
>
> -       /*
> -        * Seamless M/N update may need to update frame timings.
> -        *
> -        * FIXME Should be synchronized with the start of vblank somehow...
> -        */
> -       if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))
> -               intel_crtc_update_active_timings(new_crtc_state,
> -                                                new_crtc_state->vrr.enable);
> -
>         local_irq_enable();
>
>         if (intel_vgpu_active(dev_priv))
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index cfad967b5684..632f1f58df9e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6476,6 +6476,8 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state,
>                                     struct intel_crtc *crtc)
>  {
>         struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +       const struct intel_crtc_state *old_crtc_state =
> +               intel_atomic_get_old_crtc_state(state, crtc);
>         const struct intel_crtc_state *new_crtc_state =
>                 intel_atomic_get_new_crtc_state(state, crtc);
>
> @@ -6487,6 +6489,9 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state,
>         if (DISPLAY_VER(dev_priv) >= 9 &&
>             !intel_crtc_needs_modeset(new_crtc_state))
>                 skl_detach_scalers(new_crtc_state);
> +
> +       if (vrr_enabling(old_crtc_state, new_crtc_state))
> +               intel_vrr_enable(new_crtc_state);

Wouldnt we also need the condition here:
new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))

So that we update VRR enable bit in the seamless_m_n fastset case as well.
This will be needed later once we start updating the VRR params regs
in fastset, since
that otherwise ends up resetting VRR enable bit.



>  }
>
>  static void intel_enable_crtc(struct intel_atomic_state *state,
> @@ -6527,12 +6532,6 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>                         intel_dpt_configure(crtc);
>         }
>
> -       if (vrr_enabling(old_crtc_state, new_crtc_state)) {
> -               intel_vrr_enable(new_crtc_state);
> -               intel_crtc_update_active_timings(new_crtc_state,
> -                                                new_crtc_state->vrr.enable);
> -       }
> -
>         if (!modeset) {
>                 if (new_crtc_state->preload_luts &&
>                     intel_crtc_needs_color_update(new_crtc_state))
> @@ -6569,6 +6568,16 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>
>         intel_pipe_update_end(state, crtc);
>
> +       /*
> +        * VRR/Seamless M/N update may need to update frame timings.
> +        *
> +        * FIXME Should be synchronized with the start of vblank somehow...
> +        */
> +       if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> +           (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state)))
> +               intel_crtc_update_active_timings(new_crtc_state,
> +                                                new_crtc_state->vrr.enable);
> +

So would the VRR min/max, flipline values also be updated here
eventually for the fastset in seamless_m_n case?

Manasi


>         /*
>          * We usually enable FIFO underrun interrupts as part of the
>          * CRTC enable sequence during modesets.  But when we inherit a
> --
> 2.41.0
>