[PATCH 1/4] drm/amd/display: Prevent vblank irq disable while VRR is active. (v2)

[Mario Kleiner]

On Wed, Mar 20, 2019 at 2:11 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas at amd.com> wrote:
>
> On 3/20/19 4:12 AM, Mario Kleiner wrote:
> > During VRR mode we can not allow vblank irq dis-/enable
> > transitions, as an enable after a disable can happen at
> > an arbitrary time during the video refresh cycle, e.g.,
> > with a high likelyhood inside vblank front-porch. An
> > enable during front-porch would cause vblank timestamp
> > updates/calculations which are completely bogus, given
> > the code can't know when the vblank will end as long
> > as we are in front-porch with no page flip completed.
> >
> > Hold a permanent vblank reference on the crtc while
> > in active VRR mode to prevent a vblank disable, and
> > drop the reference again when switching back to fixed
> > refresh rate non-VRR mode.
> >
> > v2: Make sure transition is also handled if vrr is
> >      disabled and stream gets disabled in the same
> >      atomic commit. Suggested by Nicholas.
> >
> > Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 +++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index a718ac2..1c83e80 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -251,6 +251,12 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,
> >       return NULL;
> >   }
> >
> > +static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)
> > +{
> > +     return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||
> > +            dm_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
> > +}
> > +
> >   static void dm_pflip_high_irq(void *interrupt_params)
> >   {
> >       struct amdgpu_crtc *amdgpu_crtc;
> > @@ -4716,6 +4722,31 @@ static void update_freesync_state_on_stream(
> >                             (int)vrr_params.state);
> >   }
> >
> > +static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> > +                                         struct dm_crtc_state *new_state)
> > +{
> > +     bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
> > +     bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
> > +
> > +     if (!old_vrr_active && new_vrr_active) {
> > +             /* Transition VRR inactive -> active:
> > +              * While VRR is active, we must not disable vblank irq, as a
> > +              * reenable after disable would compute bogus vblank/pflip
> > +              * timestamps if it likely happened inside display front-porch.
> > +              */
> > +             drm_crtc_vblank_get(new_state->base.crtc);
> > +             DRM_DEBUG_DRIVER("%s: crtc=%u VRR off->on: Get vblank ref\n",
> > +                              __func__, new_state->base.crtc->base.id);
> > +     } else if (old_vrr_active && !new_vrr_active) {
> > +             /* Transition VRR active -> inactive:
> > +              * Allow vblank irq disable again for fixed refresh rate.
> > +              */
> > +             drm_crtc_vblank_put(new_state->base.crtc);
> > +             DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
> > +                              __func__, new_state->base.crtc->base.id);
> > +     }
> > +}
> > +
> >   static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> >                                   struct dc_state *dc_state,
> >                                   struct drm_device *dev,
> > @@ -5250,6 +5281,11 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> >
> >               dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> >               dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> > +
> > +             /* Handle vrr on->off / off->on transitions */
> > +             amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
> > +                                             dm_new_crtc_state);
> > +
>
> I guess there's actually another problem with this here - we won't have
> the actual dm_state->freesync_config.state until the commit following
> this one since it gets calculated below this transition handler.
>
> We need this handler to trigger before the new framebuffer address is
> written but after these parameters are calculated.
>
> So this needs a v3 or another patch in the series that shifts the
> calculation of config.state before this.
>

It's surprising how well it works with the thing i threw at it, for
something that's still wrong ;). But maybe that's just dumb luck in
the specific way X currently uses it.

Could you maybe try to create a patch on top of this one, implementing
your proposal below?

thanks,
-mario

> While it's probably sufficient to just reuse the block:
>
>         if (new_crtc_state->vrr_supported &&
>             config.min_refresh_in_uhz &&
>             config.max_refresh_in_uhz) {
>                 config.state = new_crtc_state->base.vrr_enabled ?
>                         VRR_STATE_ACTIVE_VARIABLE :
>                         VRR_STATE_INACTIVE;
>         } else {
>                 config.state = VRR_STATE_UNSUPPORTED;
>         }
>
> the problem is config.state could still technically be modified within
> mod_freesync_build_vrr_params I think.
>
> So it's probably best to shift everything up to and including
> mod_freesync_build_vrr_params(...) from within
> update_freesync_state_on_stream(...) earlier, either in atomic check or
> in commit tail in another loop.
>
> While I prefer the first approach, it's a little bit complicated. The
> get_freesync_config_for_crtc looks like the right place to put this, but
> that function would have to be moved. The problem is that
>
> if (!(enable && aconnector && new_crtc_state->enable &&
>               new_crtc_state->active))
>
> Won't be true if we're disabling a CRTC at the same time we're disabling
> VRR. The reset_freesync_config_for_crtc function would cover this but it
> doesn't actually touch the freesync_config - but I think it's safe to do
> so, since the check above will be true if the stream still exists.
>
> Nicholas Kazlauskas
>
> >               modeset_needed = modeset_required(
> >                               new_crtc_state,
> >                               dm_new_crtc_state->stream,
> >
>