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

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

[Mario Kleiner]

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@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);
+
 		modeset_needed = modeset_required(
 				new_crtc_state,
 				dm_new_crtc_state->stream,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Kazlauskas, Nicholas]

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@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.

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,
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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@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,
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Mario Kleiner]

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@gmail.com>
---
 .../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 42531ed6ae75..b73313d6450f 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;
@@ -4712,6 +4718,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,
@@ -5246,6 +5277,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);
+
 		modeset_needed = modeset_required(
 				new_crtc_state,
 				dm_new_crtc_state->stream,
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx