AMD Freesync patches for proper vblank and pageflip timestamping in VRR mode.

AMD Freesync patches for proper vblank and pageflip timestamping in VRR mode.

[Mario Kleiner]

Hi

This series implements properly working vblank and pageflip completion
timestamping for amdgpu in VRR / FreeSync mode.

Now pageflip timestamps for pageflip events always carry the
vblank timestamp of the vblank in which the flip completed,
and the vblank timestamp is as accurate as in fixed refresh
rate mode.

It was successfully tested on a DCE-8 gpu by myself, and also
lightly tested on a DCN-1 gpu by Nicholas Kazlauskas.

I want to thank Nicholas for helpful discussions, testing and
feedback on this patch series.

The series applies cleanly to current agd5f/drm-next-5.2-wip, against
which it was tested. It also applies against current amd-staging-drm-next
with a trivial fix to patch 2/4 like shown in the diff below:

Thanks,
-mario

@@ -80,2 +80,3 @@
 -      uint32_t target, target_vblank;
+       bool pflip_present = false;
 -      uint64_t last_flip_vblank;
@@ -84,3 +85,2 @@
 +      bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
-       bool pflip_present = false;


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

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

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

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 +++++++++++++++++++++++
 1 file changed, 35 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..c1c3815 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,32 @@ 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,
@@ -4757,6 +4789,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		goto cleanup;
 	}
 
+	/* Take care to hold extra vblank ref for a crtc in VRR mode */
+	amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, acrtc_state);
+
 	/* update planes when needed */
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		struct drm_crtc *crtc = new_plane_state->crtc;
-- 
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.

[Kazlauskas, Nicholas]

On 3/18/19 1:19 PM, 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.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 +++++++++++++++++++++++
>   1 file changed, 35 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..c1c3815 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,32 @@ 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,
> @@ -4757,6 +4789,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   		goto cleanup;
>   	}
>   
> +	/* Take care to hold extra vblank ref for a crtc in VRR mode */
> +	amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, acrtc_state);

I think this forgets to drop the extra reference in the case where the 
stream is being disabled at the same time VRR is - 
amdgpu_dm_commit_planes is only called when when the stream is non-NULL.

I think the logic will work if simply moved outside of the function into 
the loop that calls this.

Nicholas Kazlauskas

> +
>   	/* update planes when needed */
>   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>   		struct drm_crtc *crtc = new_plane_state->crtc;
> 

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

[Mario Kleiner]

On Mon, Mar 18, 2019 at 6:29 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 3/18/19 1:19 PM, 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.
> >
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 +++++++++++++++++++++++
> >   1 file changed, 35 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..c1c3815 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,32 @@ 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,
> > @@ -4757,6 +4789,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> >               goto cleanup;
> >       }
> >
> > +     /* Take care to hold extra vblank ref for a crtc in VRR mode */
> > +     amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, acrtc_state);
>
> I think this forgets to drop the extra reference in the case where the
> stream is being disabled at the same time VRR is -
> amdgpu_dm_commit_planes is only called when when the stream is non-NULL.
>
> I think the logic will work if simply moved outside of the function into
> the loop that calls this.
>
> Nicholas Kazlauskas
>

Yep! v2 patch out, thanks.
-mario
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/amd/display: Rework vrr flip throttling for late vblank irq.

[Mario Kleiner]

For throttling to work correctly, we always need a baseline vblank
count last_flip_vblank that increments at start of front-porch.

This is the case for drm_crtc_vblank_count() in non-VRR mode, where
the vblank irq fires at start of front-porch and triggers DRM core
vblank handling, but it is no longer the case in VRR mode, where
core vblank handling is done later, after end of front-porch.

Therefore drm_crtc_vblank_count() is no longer useful for this.
We also can't use drm_crtc_accurate_vblank_count(), as that would
screw up vblank timestamps in VRR mode when called in front-porch.

To solve this, use the cooked hardware vblank counter returned by
amdgpu_get_vblank_counter_kms() instead, as that one is cooked to
always increment at start of front-porch, independent of when
vblank related irq's fire.

This patch allows vblank irq handling to happen anywhere within
vblank of even after it, without a negative impact on flip
throttling, so followup patches can shift the vblank core
handling trigger point wherever they need it.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h          |  2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 889e443..add238f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -406,7 +406,7 @@ struct amdgpu_crtc {
 	struct amdgpu_flip_work *pflip_works;
 	enum amdgpu_flip_status pflip_status;
 	int deferred_flip_completion;
-	u64 last_flip_vblank;
+	u32 last_flip_vblank;
 	/* pll sharing */
 	struct amdgpu_atom_ss ss;
 	bool ss_enabled;
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 c1c3815..85e4f87 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -286,7 +286,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	}
 
 	/* Update to correct count(s) if racing with vblank irq */
-	amdgpu_crtc->last_flip_vblank = drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
+	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
 
 	/* wake up userspace */
 	if (amdgpu_crtc->event) {
@@ -298,6 +298,14 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	} else
 		WARN_ON(1);
 
+	/* Keep track of vblank of this flip for flip throttling. We use the
+	 * cooked hw counter, as that one incremented at start of this vblank
+	 * of pageflip completion, so last_flip_vblank is the forbidden count
+	 * for queueing new pageflips if vsync + VRR is enabled.
+	 */
+	amdgpu_crtc->last_flip_vblank = amdgpu_get_vblank_counter_kms(adev->ddev,
+							amdgpu_crtc->crtc_id);
+
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
@@ -4769,9 +4777,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 	struct amdgpu_bo *abo;
 	uint64_t tiling_flags;
-	uint32_t target, target_vblank;
-	uint64_t last_flip_vblank;
-	bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE;
+	uint32_t target_vblank, last_flip_vblank;
+	bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
 	bool pflip_present = false;
 
 	struct {
@@ -4918,7 +4925,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			 * clients using the GLX_OML_sync_control extension or
 			 * DRI3/Present extension with defined target_msc.
 			 */
-			last_flip_vblank = drm_crtc_vblank_count(pcrtc);
+			last_flip_vblank = amdgpu_get_vblank_counter_kms(dm->ddev, acrtc_attach->crtc_id);
 		}
 		else {
 			/* For variable refresh rate mode only:
@@ -4934,11 +4941,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
 
-		target = (uint32_t)last_flip_vblank + wait_for_vblank;
-
-		/* Prepare wait for target vblank early - before the fence-waits */
-		target_vblank = target - (uint32_t)drm_crtc_vblank_count(pcrtc) +
-				amdgpu_get_vblank_counter_kms(pcrtc->dev, acrtc_attach->crtc_id);
+		target_vblank = last_flip_vblank + wait_for_vblank;
 
 		/*
 		 * Wait until we're out of the vertical blank period before the one
-- 
2.7.4

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

Re: [PATCH 2/4] drm/amd/display: Rework vrr flip throttling for late vblank irq.

[Kazlauskas, Nicholas]

On 3/18/19 1:19 PM, Mario Kleiner wrote:
> For throttling to work correctly, we always need a baseline vblank
> count last_flip_vblank that increments at start of front-porch.
> 
> This is the case for drm_crtc_vblank_count() in non-VRR mode, where
> the vblank irq fires at start of front-porch and triggers DRM core
> vblank handling, but it is no longer the case in VRR mode, where
> core vblank handling is done later, after end of front-porch.
> 
> Therefore drm_crtc_vblank_count() is no longer useful for this.
> We also can't use drm_crtc_accurate_vblank_count(), as that would
> screw up vblank timestamps in VRR mode when called in front-porch.
> 
> To solve this, use the cooked hardware vblank counter returned by
> amdgpu_get_vblank_counter_kms() instead, as that one is cooked to
> always increment at start of front-porch, independent of when
> vblank related irq's fire.
> 
> This patch allows vblank irq handling to happen anywhere within
> vblank of even after it, without a negative impact on flip
> throttling, so followup patches can shift the vblank core
> handling trigger point wherever they need it.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Functionally drm_crtc_accurate_vblank_count is the same as 
amdgpu_get_vblank_counter_kms for querying purposes, so this works.

It's a nice cleanup for commit planes too since we no longer grab the 
DRM vblank count only to immediately subtract it off again.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h          |  2 +-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++++----------
>   2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 889e443..add238f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -406,7 +406,7 @@ struct amdgpu_crtc {
>   	struct amdgpu_flip_work *pflip_works;
>   	enum amdgpu_flip_status pflip_status;
>   	int deferred_flip_completion;
> -	u64 last_flip_vblank;
> +	u32 last_flip_vblank;
>   	/* pll sharing */
>   	struct amdgpu_atom_ss ss;
>   	bool ss_enabled;
> 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 c1c3815..85e4f87 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -286,7 +286,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	}
>   
>   	/* Update to correct count(s) if racing with vblank irq */
> -	amdgpu_crtc->last_flip_vblank = drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
> +	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
>   
>   	/* wake up userspace */
>   	if (amdgpu_crtc->event) {
> @@ -298,6 +298,14 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	} else
>   		WARN_ON(1);
>   
> +	/* Keep track of vblank of this flip for flip throttling. We use the
> +	 * cooked hw counter, as that one incremented at start of this vblank
> +	 * of pageflip completion, so last_flip_vblank is the forbidden count
> +	 * for queueing new pageflips if vsync + VRR is enabled.
> +	 */
> +	amdgpu_crtc->last_flip_vblank = amdgpu_get_vblank_counter_kms(adev->ddev,
> +							amdgpu_crtc->crtc_id);
> +
>   	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
>   	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>   
> @@ -4769,9 +4777,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   	unsigned long flags;
>   	struct amdgpu_bo *abo;
>   	uint64_t tiling_flags;
> -	uint32_t target, target_vblank;
> -	uint64_t last_flip_vblank;
> -	bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE;
> +	uint32_t target_vblank, last_flip_vblank;
> +	bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
>   	bool pflip_present = false;
>   
>   	struct {
> @@ -4918,7 +4925,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   			 * clients using the GLX_OML_sync_control extension or
>   			 * DRI3/Present extension with defined target_msc.
>   			 */
> -			last_flip_vblank = drm_crtc_vblank_count(pcrtc);
> +			last_flip_vblank = amdgpu_get_vblank_counter_kms(dm->ddev, acrtc_attach->crtc_id);
>   		}
>   		else {
>   			/* For variable refresh rate mode only:
> @@ -4934,11 +4941,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>   		}
>   
> -		target = (uint32_t)last_flip_vblank + wait_for_vblank;
> -
> -		/* Prepare wait for target vblank early - before the fence-waits */
> -		target_vblank = target - (uint32_t)drm_crtc_vblank_count(pcrtc) +
> -				amdgpu_get_vblank_counter_kms(pcrtc->dev, acrtc_attach->crtc_id);
> +		target_vblank = last_flip_vblank + wait_for_vblank;
>   
>   		/*
>   		 * Wait until we're out of the vertical blank period before the one
> 

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

[PATCH 4/4] drm/amd/display: Make pageflip event delivery compatible with VRR.

[Mario Kleiner]

We want vblank counts and timestamps of flip completion as sent
in pageflip completion events to be consistent with the vblank
count and timestamp of the vblank of flip completion, like in non
VRR mode.

In VRR mode, drm_update_vblank_count() - and thereby vblank
count and timestamp updates - must be delayed until after the
end of front-porch of each vblank, as it is only safe to
calculate vblank timestamps outside of the front-porch, when
we actually know when the vblank will end or has ended.

The function drm_update_vblank_count() which updates timestamps
and counts gets called by drm_crtc_accurate_vblank_count() or by
drm_crtc_handle_vblank().

Therefore we must make sure that pageflip events for a completed
flip are only sent out after drm_crtc_accurate_vblank_count() or
drm_crtc_handle_vblank() is executed, after end of front-porch
for the vblank of flip completion.

Two cases:

a) Pageflip irq handler executes inside front-porch:
   In this case we must defer sending pageflip events until
   drm_crtc_handle_vblank() executes after end of front-porch,
   and thereby calculates proper vblank count and timestamp.
   Iow. the pflip irq handler must just arm a pageflip event
   to be sent out by drm_crtc_handle_vblank() later on.

b) Pageflip irq handler executes after end of front-porch, e.g.,
   after flip completion in back-porch or due to a massively
   delayed handler invocation into the active scanout of the new
   frame. In this case we can call drm_crtc_accurate_vblank_count()
   to safely force calculation of a proper vblank count and
   timestamp, and must send the pageflip completion event
   ourselves from the pageflip irq handler.

   This is the same behaviour as needed for standard fixed refresh
   rate mode.

To decide from within pageflip handler if we are in case a) or b),
we check the current scanout position against the boundary of
front-porch. In non-VRR mode we just do what we did in the past.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68 ++++++++++++++++++-----
 1 file changed, 55 insertions(+), 13 deletions(-)

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 555d9e9f..499284d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -263,6 +263,10 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	struct common_irq_params *irq_params = interrupt_params;
 	struct amdgpu_device *adev = irq_params->adev;
 	unsigned long flags;
+	struct drm_pending_vblank_event *e;
+	struct dm_crtc_state *acrtc_state;
+	uint32_t vpos, hpos, v_blank_start, v_blank_end;
+	bool vrr_active;
 
 	amdgpu_crtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_PFLIP);
 
@@ -285,18 +289,57 @@ static void dm_pflip_high_irq(void *interrupt_params)
 		return;
 	}
 
-	/* Update to correct count(s) if racing with vblank irq */
-	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
+	/* page flip completed. */
+	e = amdgpu_crtc->event;
+	amdgpu_crtc->event = NULL;
 
-	/* wake up userspace */
-	if (amdgpu_crtc->event) {
-		drm_crtc_send_vblank_event(&amdgpu_crtc->base, amdgpu_crtc->event);
+	if (!e)
+		WARN_ON(1);
 
-		/* page flip completed. clean up */
-		amdgpu_crtc->event = NULL;
+	acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);
+	vrr_active = amdgpu_dm_vrr_active(acrtc_state);
+
+	/* Fixed refresh rate, or VRR scanout position outside front-porch? */
+	if (!vrr_active ||
+	    !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,
+				      &v_blank_end, &hpos, &vpos) ||
+	    (vpos < v_blank_start)) {
+		/* Update to correct count and vblank timestamp if racing with
+		 * vblank irq. This also updates to the correct vblank timestamp
+		 * even in VRR mode, as scanout is past the front-porch atm.
+		 */
+		drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
 
-	} else
-		WARN_ON(1);
+		/* Wake up userspace by sending the pageflip event with proper
+		 * count and timestamp of vblank of flip completion.
+		 */
+		if (e) {
+			drm_crtc_send_vblank_event(&amdgpu_crtc->base, e);
+
+			/* Event sent, so done with vblank for this flip */
+			drm_crtc_vblank_put(&amdgpu_crtc->base);
+		}
+	} else if (e) {
+		/* VRR active and inside front-porch: vblank count and
+		 * timestamp for pageflip event will only be up to date after
+		 * drm_crtc_handle_vblank() has been executed from late vblank
+		 * irq handler after start of back-porch (vline 0). We queue the
+		 * pageflip event for send-out by drm_crtc_handle_vblank() with
+		 * updated timestamp and count, once it runs after us.
+		 *
+		 * We need to open-code this instead of using the helper
+		 * drm_crtc_arm_vblank_event(), as that helper would
+		 * call drm_crtc_accurate_vblank_count(), which we must
+		 * not call in VRR mode while we are in front-porch!
+		 */
+
+		/* sequence will be replaced by real count during send-out. */
+		e->sequence = drm_crtc_vblank_count(&amdgpu_crtc->base);
+		e->pipe = amdgpu_crtc->crtc_id;
+
+		list_add_tail(&e->base.link, &adev->ddev->vblank_event_list);
+		e = NULL;
+	}
 
 	/* Keep track of vblank of this flip for flip throttling. We use the
 	 * cooked hw counter, as that one incremented at start of this vblank
@@ -309,10 +352,9 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
-	DRM_DEBUG_DRIVER("%s - crtc :%d[%p], pflip_stat:AMDGPU_FLIP_NONE\n",
-					__func__, amdgpu_crtc->crtc_id, amdgpu_crtc);
-
-	drm_crtc_vblank_put(&amdgpu_crtc->base);
+	DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_NONE, vrr[%d]-fp %d\n",
+			 amdgpu_crtc->crtc_id, amdgpu_crtc,
+			 vrr_active, (int) !e);
 }
 
 static void dm_vupdate_high_irq(void *interrupt_params)
-- 
2.7.4

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

Re: [PATCH 4/4] drm/amd/display: Make pageflip event delivery compatible with VRR.

[Kazlauskas, Nicholas]

On 3/18/19 1:19 PM, Mario Kleiner wrote:
> We want vblank counts and timestamps of flip completion as sent
> in pageflip completion events to be consistent with the vblank
> count and timestamp of the vblank of flip completion, like in non
> VRR mode.
> 
> In VRR mode, drm_update_vblank_count() - and thereby vblank
> count and timestamp updates - must be delayed until after the
> end of front-porch of each vblank, as it is only safe to
> calculate vblank timestamps outside of the front-porch, when
> we actually know when the vblank will end or has ended.
> 
> The function drm_update_vblank_count() which updates timestamps
> and counts gets called by drm_crtc_accurate_vblank_count() or by
> drm_crtc_handle_vblank().
> 
> Therefore we must make sure that pageflip events for a completed
> flip are only sent out after drm_crtc_accurate_vblank_count() or
> drm_crtc_handle_vblank() is executed, after end of front-porch
> for the vblank of flip completion.
> 
> Two cases:
> 
> a) Pageflip irq handler executes inside front-porch:
>     In this case we must defer sending pageflip events until
>     drm_crtc_handle_vblank() executes after end of front-porch,
>     and thereby calculates proper vblank count and timestamp.
>     Iow. the pflip irq handler must just arm a pageflip event
>     to be sent out by drm_crtc_handle_vblank() later on.
> 
> b) Pageflip irq handler executes after end of front-porch, e.g.,
>     after flip completion in back-porch or due to a massively
>     delayed handler invocation into the active scanout of the new
>     frame. In this case we can call drm_crtc_accurate_vblank_count()
>     to safely force calculation of a proper vblank count and
>     timestamp, and must send the pageflip completion event
>     ourselves from the pageflip irq handler.
> 
>     This is the same behaviour as needed for standard fixed refresh
>     rate mode.
> 
> To decide from within pageflip handler if we are in case a) or b),
> we check the current scanout position against the boundary of
> front-porch. In non-VRR mode we just do what we did in the past.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

This patch looks fine to me for the most part, but it's still pending on 
the other patches in the series.

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68 ++++++++++++++++++-----
>   1 file changed, 55 insertions(+), 13 deletions(-)
> 
> 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 555d9e9f..499284d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -263,6 +263,10 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	struct common_irq_params *irq_params = interrupt_params;
>   	struct amdgpu_device *adev = irq_params->adev;
>   	unsigned long flags;
> +	struct drm_pending_vblank_event *e;
> +	struct dm_crtc_state *acrtc_state;
> +	uint32_t vpos, hpos, v_blank_start, v_blank_end;
> +	bool vrr_active;
>   
>   	amdgpu_crtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_PFLIP);
>   
> @@ -285,18 +289,57 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   		return;
>   	}
>   
> -	/* Update to correct count(s) if racing with vblank irq */
> -	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
> +	/* page flip completed. */
> +	e = amdgpu_crtc->event;
> +	amdgpu_crtc->event = NULL;
>   
> -	/* wake up userspace */
> -	if (amdgpu_crtc->event) {
> -		drm_crtc_send_vblank_event(&amdgpu_crtc->base, amdgpu_crtc->event);
> +	if (!e)
> +		WARN_ON(1);
>   
> -		/* page flip completed. clean up */
> -		amdgpu_crtc->event = NULL;
> +	acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);
> +	vrr_active = amdgpu_dm_vrr_active(acrtc_state);
> +
> +	/* Fixed refresh rate, or VRR scanout position outside front-porch? */
> +	if (!vrr_active ||
> +	    !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,
> +				      &v_blank_end, &hpos, &vpos) ||
> +	    (vpos < v_blank_start)) {
> +		/* Update to correct count and vblank timestamp if racing with
> +		 * vblank irq. This also updates to the correct vblank timestamp
> +		 * even in VRR mode, as scanout is past the front-porch atm.
> +		 */
> +		drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
>   
> -	} else
> -		WARN_ON(1);
> +		/* Wake up userspace by sending the pageflip event with proper
> +		 * count and timestamp of vblank of flip completion.
> +		 */
> +		if (e) {
> +			drm_crtc_send_vblank_event(&amdgpu_crtc->base, e);
> +
> +			/* Event sent, so done with vblank for this flip */
> +			drm_crtc_vblank_put(&amdgpu_crtc->base);
> +		}
> +	} else if (e) {
> +		/* VRR active and inside front-porch: vblank count and
> +		 * timestamp for pageflip event will only be up to date after
> +		 * drm_crtc_handle_vblank() has been executed from late vblank
> +		 * irq handler after start of back-porch (vline 0). We queue the
> +		 * pageflip event for send-out by drm_crtc_handle_vblank() with
> +		 * updated timestamp and count, once it runs after us.
> +		 *
> +		 * We need to open-code this instead of using the helper
> +		 * drm_crtc_arm_vblank_event(), as that helper would
> +		 * call drm_crtc_accurate_vblank_count(), which we must
> +		 * not call in VRR mode while we are in front-porch!
> +		 */
> +
> +		/* sequence will be replaced by real count during send-out. */
> +		e->sequence = drm_crtc_vblank_count(&amdgpu_crtc->base);
> +		e->pipe = amdgpu_crtc->crtc_id;
> +
> +		list_add_tail(&e->base.link, &adev->ddev->vblank_event_list);
> +		e = NULL;

I guess drm_crtc_vblank_off handles sending any leftover events here if 
there are any if the IRQ gets disabled incorrectly.

I wonder why we never just used this list in the first place for 
pageflip vblank event handling instead of the locking and NULL set. 
Though I suppose it's more useful to use this now for VRR vblank handling.

> +	}
>   
>   	/* Keep track of vblank of this flip for flip throttling. We use the
>   	 * cooked hw counter, as that one incremented at start of this vblank
> @@ -309,10 +352,9 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
>   	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>   
> -	DRM_DEBUG_DRIVER("%s - crtc :%d[%p], pflip_stat:AMDGPU_FLIP_NONE\n",
> -					__func__, amdgpu_crtc->crtc_id, amdgpu_crtc);
> -
> -	drm_crtc_vblank_put(&amdgpu_crtc->base);
> +	DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_NONE, vrr[%d]-fp %d\n",
> +			 amdgpu_crtc->crtc_id, amdgpu_crtc,
> +			 vrr_active, (int) !e);

The debug output for this is a little strange, but I guess it makes it 
makes as much sense as the original did. I'm glad the __func__ is gone 
at least.

>   }
>   
>   static void dm_vupdate_high_irq(void *interrupt_params)
> 

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

[PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Mario Kleiner]

In VRR mode, proper vblank/pageflip timestamps can only be computed
after the display scanout position has left front-porch. Therefore
delay calls to drm_crtc_handle_vblank(), and thereby calls to
drm_update_vblank_count() and pageflip event delivery, to after the
end of front-porch when in VRR mode.

We add a new vupdate irq, which triggers at the end of the vupdate
interval, ie. at the end of vblank, and calls the core vblank handler
function. The new irq handler is not executed in standard non-VRR
mode, so vblank handling for fixed refresh rate mode is identical
to the past implementation.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
 .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
 .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
 .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
 .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
 8 files changed, 205 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f88761a..64167dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -827,6 +827,7 @@ struct amdgpu_device {
 	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
 	struct work_struct		hotplug_work;
 	struct amdgpu_irq_src		crtc_irq;
+	struct amdgpu_irq_src		vupdate_irq;
 	struct amdgpu_irq_src		pageflip_irq;
 	struct amdgpu_irq_src		hpd_irq;
 
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 85e4f87..555d9e9f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	drm_crtc_vblank_put(&amdgpu_crtc->base);
 }
 
+static void dm_vupdate_high_irq(void *interrupt_params)
+{
+	struct common_irq_params *irq_params = interrupt_params;
+	struct amdgpu_device *adev = irq_params->adev;
+	struct amdgpu_crtc *acrtc;
+	struct dm_crtc_state *acrtc_state;
+
+	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
+
+	if (acrtc) {
+		acrtc_state = to_dm_crtc_state(acrtc->base.state);
+
+		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				 amdgpu_dm_vrr_active(acrtc_state));
+
+		/* Core vblank handling is done here after end of front-porch in
+		 * vrr mode, as vblank timestamping will give valid results
+		 * while now done after front-porch. This will also deliver
+		 * page-flip completion events that have been queued to us
+		 * if a pageflip happened inside front-porch.
+		 */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			drm_crtc_handle_vblank(&acrtc->base);
+	}
+}
+
 static void dm_crtc_high_irq(void *interrupt_params)
 {
 	struct common_irq_params *irq_params = interrupt_params;
@@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
 	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
 
 	if (acrtc) {
-		drm_crtc_handle_vblank(&acrtc->base);
-		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
-
 		acrtc_state = to_dm_crtc_state(acrtc->base.state);
 
+		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
+				 amdgpu_dm_vrr_active(acrtc_state));
+
+		/* Core vblank handling at start of front-porch is only possible
+		 * in non-vrr mode, as only there vblank timestamping will give
+		 * valid results while done in front-porch. Otherwise defer it
+		 * to dm_vupdate_high_irq after end of front-porch.
+		 */
+		if (!amdgpu_dm_vrr_active(acrtc_state))
+			drm_crtc_handle_vblank(&acrtc->base);
+
+		/* Following stuff must happen at start of vblank, for crc
+		 * computation and below-the-range btr support in vrr mode.
+		 */
+		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
+
 		if (acrtc_state->stream &&
 		    acrtc_state->vrr_params.supported &&
 		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
@@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
 				dm_crtc_high_irq, c_irq_params);
 	}
 
+	/* Use VUPDATE interrupt */
+	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
+		r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
+		if (r) {
+			DRM_ERROR("Failed to add vupdate irq id!\n");
+			return r;
+		}
+
+		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
+		int_params.irq_source =
+			dc_interrupt_to_irq_source(dc, i, 0);
+
+		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
+
+		c_irq_params->adev = adev;
+		c_irq_params->irq_src = int_params.irq_source;
+
+		amdgpu_dm_irq_register_interrupt(adev, &int_params,
+				dm_vupdate_high_irq, c_irq_params);
+	}
+
 	/* Use GRPH_PFLIP interrupt */
 	for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
 			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
@@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_crtc_high_irq, c_irq_params);
 	}
 
+	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
+	 * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
+	 * to trigger at end of each vblank, regardless of state of the lock,
+	 * matching DCE behaviour.
+	 */
+	for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
+	     i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
+	     i++) {
+		r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
+
+		if (r) {
+			DRM_ERROR("Failed to add vupdate irq id!\n");
+			return r;
+		}
+
+		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
+		int_params.irq_source =
+			dc_interrupt_to_irq_source(dc, i, 0);
+
+		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
+
+		c_irq_params->adev = adev;
+		c_irq_params->irq_src = int_params.irq_source;
+
+		amdgpu_dm_irq_register_interrupt(adev, &int_params,
+				dm_vupdate_high_irq, c_irq_params);
+	}
+
 	/* Use GRPH_PFLIP interrupt */
 	for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
 			i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
@@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &state->base;
 }
 
+static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
+{
+	enum dc_irq_source irq_source;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct amdgpu_device *adev = crtc->dev->dev_private;
+	int rc;
+
+	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
+
+	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+
+	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
+			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
+	return rc;
+}
 
 static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 {
 	enum dc_irq_source irq_source;
 	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 	struct amdgpu_device *adev = crtc->dev->dev_private;
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	int rc = 0;
+
+	if (enable) {
+		/* vblank irq on -> Only need vupdate irq in vrr mode */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			rc = dm_set_vupdate_irq(crtc, true);
+	}
+	else {
+		/* vblank irq off -> vupdate irq off */
+		rc = dm_set_vupdate_irq(crtc, false);
+	}
+
+	if (rc)
+		return rc;
 
 	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
 	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
@@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * 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.
+		 *
+		 * We also need vupdate irq for the actual core vblank handling
+		 * at end of vblank.
 		 */
+		dm_set_vupdate_irq(new_state->base.crtc, true);
 		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);
@@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		/* Transition VRR active -> inactive:
 		 * Allow vblank irq disable again for fixed refresh rate.
 		 */
+		dm_set_vupdate_irq(new_state->base.crtc, false);
 		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);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f741ea3..0b35f84 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -182,6 +182,15 @@ struct amdgpu_display_manager {
 	struct common_irq_params
 	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
 
+	/**
+	 * @vupdate_params:
+	 *
+	 * Vertical update IRQ parameters, passed to registered handlers when
+	 * triggered.
+	 */
+	struct common_irq_params
+	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
+
 	spinlock_t irq_handler_list_table_lock;
 
 	struct backlight_device *backlight_dev;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index cd10f77..fee7837 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
 		__func__);
 }
 
+static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
+					   struct amdgpu_irq_src *source,
+					   unsigned crtc_id,
+					   enum amdgpu_interrupt_state state)
+{
+	return dm_irq_state(
+		adev,
+		source,
+		crtc_id,
+		state,
+		IRQ_TYPE_VUPDATE,
+		__func__);
+}
+
 static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
 	.set = amdgpu_dm_set_crtc_irq_state,
 	.process = amdgpu_dm_irq_handler,
 };
 
+static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
+	.set = amdgpu_dm_set_vupdate_irq_state,
+	.process = amdgpu_dm_irq_handler,
+};
+
 static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
 	.set = amdgpu_dm_set_pflip_irq_state,
 	.process = amdgpu_dm_irq_handler,
@@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
 	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
 	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
 
+	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
+	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
+
 	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
 	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
 
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
index afe0876..86987f5 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
@@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
 		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
@@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
index 1ea7256..750ba0a 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
@@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		IRQ_REG_ENTRY(CRTC, reg_num,\
 			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
 			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
index 8a2066c..de218fe 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
@@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
 
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
@@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
index e04ae49..10ac6de 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
@@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
 		return DC_IRQ_SOURCE_VBLANK5;
 	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
 		return DC_IRQ_SOURCE_VBLANK6;
+	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE1;
+	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE2;
+	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE3;
+	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE4;
+	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE5;
+	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE6;
 	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
 		return DC_IRQ_SOURCE_PFLIP1;
 	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
@@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		.funcs = &pflip_irq_info_funcs\
 	}
 
-#define vupdate_int_entry(reg_num)\
+/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
+ * of DCE's DC_IRQ_SOURCE_VUPDATEx.
+ */
+#define vupdate_no_lock_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
 		IRQ_REG_ENTRY(OTG, reg_num,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
+		.funcs = &vupdate_no_lock_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
@@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
 	dc_underflow_int_entry(6),
 	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
 	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
-	vupdate_int_entry(0),
-	vupdate_int_entry(1),
-	vupdate_int_entry(2),
-	vupdate_int_entry(3),
-	vupdate_int_entry(4),
-	vupdate_int_entry(5),
+	vupdate_no_lock_int_entry(0),
+	vupdate_no_lock_int_entry(1),
+	vupdate_no_lock_int_entry(2),
+	vupdate_no_lock_int_entry(3),
+	vupdate_no_lock_int_entry(4),
+	vupdate_no_lock_int_entry(5),
 	vblank_int_entry(0),
 	vblank_int_entry(1),
 	vblank_int_entry(2),
-- 
2.7.4

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

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Paul Menzel]

Dear Mario,


On 18.03.19 18:19, Mario Kleiner wrote:
> In VRR mode, proper vblank/pageflip timestamps can only be computed
> after the display scanout position has left front-porch. Therefore
> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> drm_update_vblank_count() and pageflip event delivery, to after the
> end of front-porch when in VRR mode.
> 
> We add a new vupdate irq, which triggers at the end of the vupdate
> interval, ie. at the end of vblank, and calls the core vblank handler
> function. The new irq handler is not executed in standard non-VRR
> mode, so vblank handling for fixed refresh rate mode is identical
> to the past implementation.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>   .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>   .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>   .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>   .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>   8 files changed, 205 insertions(+), 16 deletions(-)

[…]

`scripts/checkpatch.pl` shows two errors on your commit.

> +	/* Use VUPDATE interrupt */
> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {

ERROR: spaces required around that '+=' (ctx:VxV)
#107: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1490:
+	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= 
VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
  	 
                    ^

[…]

>   static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   {
>   	enum dc_irq_source irq_source;
>   	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> +	int rc = 0;
> +
> +	if (enable) {
> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			rc = dm_set_vupdate_irq(crtc, true);
> +	}
> +	else {
> +		/* vblank irq off -> vupdate irq off */
> +		rc = dm_set_vupdate_irq(crtc, false);
> +	}

ERROR: else should follow close brace '}'
#198: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:3346:
+	}
+	else {

Also:

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#258: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c:679:
+					   unsigned crtc_id,

[…]


Kind regards,

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

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Kazlauskas, Nicholas]

On 3/18/19 1:19 PM, Mario Kleiner wrote:
> In VRR mode, proper vblank/pageflip timestamps can only be computed
> after the display scanout position has left front-porch. Therefore
> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> drm_update_vblank_count() and pageflip event delivery, to after the
> end of front-porch when in VRR mode.
> 
> We add a new vupdate irq, which triggers at the end of the vupdate
> interval, ie. at the end of vblank, and calls the core vblank handler
> function. The new irq handler is not executed in standard non-VRR
> mode, so vblank handling for fixed refresh rate mode is identical
> to the past implementation.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>   .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>   .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>   .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>   .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>   8 files changed, 205 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f88761a..64167dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -827,6 +827,7 @@ struct amdgpu_device {
>   	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>   	struct work_struct		hotplug_work;
>   	struct amdgpu_irq_src		crtc_irq;
> +	struct amdgpu_irq_src		vupdate_irq;
>   	struct amdgpu_irq_src		pageflip_irq;
>   	struct amdgpu_irq_src		hpd_irq;
>   
> 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 85e4f87..555d9e9f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	drm_crtc_vblank_put(&amdgpu_crtc->base);
>   }
>   
> +static void dm_vupdate_high_irq(void *interrupt_params)
> +{
> +	struct common_irq_params *irq_params = interrupt_params;
> +	struct amdgpu_device *adev = irq_params->adev;
> +	struct amdgpu_crtc *acrtc;
> +	struct dm_crtc_state *acrtc_state;
> +
> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> +
> +	if (acrtc) {
> +		acrtc_state = to_dm_crtc_state(acrtc->base.state);
> +
> +		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> +				 amdgpu_dm_vrr_active(acrtc_state));
> +
> +		/* Core vblank handling is done here after end of front-porch in
> +		 * vrr mode, as vblank timestamping will give valid results
> +		 * while now done after front-porch. This will also deliver
> +		 * page-flip completion events that have been queued to us
> +		 * if a pageflip happened inside front-porch.
> +		 */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			drm_crtc_handle_vblank(&acrtc->base)
I was thinking that 3 and 4 might have to be merged, but VRR pflip 
timestamping seems to be the same as it was before (off by a line or 
two) since it's not handled here yet. This seems to fix vblank events 
and timestamping at least.

> +	}
> +}
> +
>   static void dm_crtc_high_irq(void *interrupt_params)
>   {
>   	struct common_irq_params *irq_params = interrupt_params;
> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>   
>   	if (acrtc) {
> -		drm_crtc_handle_vblank(&acrtc->base);
> -		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> -
>   		acrtc_state = to_dm_crtc_state(acrtc->base.state);
>   
> +		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> +				 amdgpu_dm_vrr_active(acrtc_state));
> +
> +		/* Core vblank handling at start of front-porch is only possible
> +		 * in non-vrr mode, as only there vblank timestamping will give
> +		 * valid results while done in front-porch. Otherwise defer it
> +		 * to dm_vupdate_high_irq after end of front-porch.
> +		 */
> +		if (!amdgpu_dm_vrr_active(acrtc_state))
> +			drm_crtc_handle_vblank(&acrtc->base);
> +
> +		/* Following stuff must happen at start of vblank, for crc
> +		 * computation and below-the-range btr support in vrr mode.
> +		 */
> +		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> +
>   		if (acrtc_state->stream &&
>   		    acrtc_state->vrr_params.supported &&
>   		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_crtc_high_irq, c_irq_params);
>   	}
>   
> +	/* Use VUPDATE interrupt */
> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> +		r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
> +		if (r) {
> +			DRM_ERROR("Failed to add vupdate irq id!\n");
> +			return r;
> +		}
> +
> +		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> +		int_params.irq_source =
> +			dc_interrupt_to_irq_source(dc, i, 0);
> +
> +		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> +
> +		c_irq_params->adev = adev;
> +		c_irq_params->irq_src = int_params.irq_source;
> +
> +		amdgpu_dm_irq_register_interrupt(adev, &int_params,
> +				dm_vupdate_high_irq, c_irq_params);
> +	}
> +
>   	/* Use GRPH_PFLIP interrupt */
>   	for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>   			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_crtc_high_irq, c_irq_params);
>   	}
>   
> +	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
> +	 * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
> +	 * to trigger at end of each vblank, regardless of state of the lock,
> +	 * matching DCE behaviour.
> +	 */
> +	for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
> +	     i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
> +	     i++) {
> +		r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
> +
> +		if (r) {
> +			DRM_ERROR("Failed to add vupdate irq id!\n");
> +			return r;
> +		}
> +
> +		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> +		int_params.irq_source =
> +			dc_interrupt_to_irq_source(dc, i, 0);
> +
> +		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> +
> +		c_irq_params->adev = adev;
> +		c_irq_params->irq_src = int_params.irq_source;
> +
> +		amdgpu_dm_irq_register_interrupt(adev, &int_params,
> +				dm_vupdate_high_irq, c_irq_params);
> +	}
> +
>   	/* Use GRPH_PFLIP interrupt */
>   	for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
>   			i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>   	return &state->base;
>   }
>   
> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> +{
> +	enum dc_irq_source irq_source;
> +	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	int rc;
> +
> +	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> +
> +	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> +
> +	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> +			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
> +	return rc;
> +}
>   
>   static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   {
>   	enum dc_irq_source irq_source;
>   	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> +	int rc = 0;
> +
> +	if (enable) {
> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			rc = dm_set_vupdate_irq(crtc, true);
> +	}
> +	else {

should be } else {

> +		/* vblank irq off -> vupdate irq off */
> +		rc = dm_set_vupdate_irq(crtc, false);
> +	}
> +
> +	if (rc)
> +		return rc;
>   
>   	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>   	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		 * 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.
> +		 *
> +		 * We also need vupdate irq for the actual core vblank handling
> +		 * at end of vblank.
>   		 */
> +		dm_set_vupdate_irq(new_state->base.crtc, true);
>   		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);
> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		/* Transition VRR active -> inactive:
>   		 * Allow vblank irq disable again for fixed refresh rate.
>   		 */
> +		dm_set_vupdate_irq(new_state->base.crtc, false);
>   		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);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index f741ea3..0b35f84 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>   	struct common_irq_params
>   	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>   
> +	/**
> +	 * @vupdate_params:
> +	 *
> +	 * Vertical update IRQ parameters, passed to registered handlers when
> +	 * triggered.
> +	 */
> +	struct common_irq_params
> +	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> +
>   	spinlock_t irq_handler_list_table_lock;
>   
>   	struct backlight_device *backlight_dev;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> index cd10f77..fee7837 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>   		__func__);
>   }
>   
> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> +					   struct amdgpu_irq_src *source,
> +					   unsigned crtc_id,

should be unsigned int crtc_id

> +					   enum amdgpu_interrupt_state state)
> +{
> +	return dm_irq_state(
> +		adev,
> +		source,
> +		crtc_id,
> +		state,
> +		IRQ_TYPE_VUPDATE,
> +		__func__);
> +}
> +
>   static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>   	.set = amdgpu_dm_set_crtc_irq_state,
>   	.process = amdgpu_dm_irq_handler,
>   };
>   
> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> +	.set = amdgpu_dm_set_vupdate_irq_state,
> +	.process = amdgpu_dm_irq_handler,
> +};
> +
>   static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>   	.set = amdgpu_dm_set_pflip_irq_state,
>   	.process = amdgpu_dm_irq_handler,
> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>   	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>   	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>   
> +	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> +	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> +
>   	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>   	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>   
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> index afe0876..86987f5 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define hpd_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>   		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>   		.ack_value =\
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> index 1ea7256..750ba0a 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define BASE_INNER(seg) \
>   	DCE_BASE__INST0_SEG ## seg
>   
> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		IRQ_REG_ENTRY(CRTC, reg_num,\
>   			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>   			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> index 8a2066c..de218fe 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
>   
>   #define hpd_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>   		.ack_value =\
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> index e04ae49..10ac6de 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>   		return DC_IRQ_SOURCE_VBLANK5;
>   	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>   		return DC_IRQ_SOURCE_VBLANK6;
> +	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE1;
> +	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE2;
> +	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE3;
> +	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE4;
> +	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE5;
> +	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE6;
>   	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>   		return DC_IRQ_SOURCE_PFLIP1;
>   	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define BASE_INNER(seg) \
>   	DCE_BASE__INST0_SEG ## seg
>   
> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		.funcs = &pflip_irq_info_funcs\
>   	}
>   
> -#define vupdate_int_entry(reg_num)\
> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> + */
> +#define vupdate_no_lock_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>   		IRQ_REG_ENTRY(OTG, reg_num,\
> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> -		.funcs = &vblank_irq_info_funcs\
> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> +		.funcs = &vupdate_no_lock_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>   	dc_underflow_int_entry(6),
>   	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>   	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> -	vupdate_int_entry(0),
> -	vupdate_int_entry(1),
> -	vupdate_int_entry(2),
> -	vupdate_int_entry(3),
> -	vupdate_int_entry(4),
> -	vupdate_int_entry(5),
> +	vupdate_no_lock_int_entry(0),
> +	vupdate_no_lock_int_entry(1),
> +	vupdate_no_lock_int_entry(2),
> +	vupdate_no_lock_int_entry(3),
> +	vupdate_no_lock_int_entry(4),
> +	vupdate_no_lock_int_entry(5),
>   	vblank_int_entry(0),
>   	vblank_int_entry(1),
>   	vblank_int_entry(2),
> 

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

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Kazlauskas, Nicholas]

On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> On 3/18/19 1:19 PM, Mario Kleiner wrote:
>> In VRR mode, proper vblank/pageflip timestamps can only be computed
>> after the display scanout position has left front-porch. Therefore
>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
>> drm_update_vblank_count() and pageflip event delivery, to after the
>> end of front-porch when in VRR mode.
>>
>> We add a new vupdate irq, which triggers at the end of the vupdate
>> interval, ie. at the end of vblank, and calls the core vblank handler
>> function. The new irq handler is not executed in standard non-VRR
>> mode, so vblank handling for fixed refresh rate mode is identical
>> to the past implementation.
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Looks I lost some of my comments I wanted to send in my last email. Just 
a few nitpicks (including some things Paul mentioned).

Also meant to CC Harry on this as well.

>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>>    .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>>    .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>>    .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>>    .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>>    8 files changed, 205 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index f88761a..64167dd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -827,6 +827,7 @@ struct amdgpu_device {
>>    	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>>    	struct work_struct		hotplug_work;
>>    	struct amdgpu_irq_src		crtc_irq;
>> +	struct amdgpu_irq_src		vupdate_irq;
>>    	struct amdgpu_irq_src		pageflip_irq;
>>    	struct amdgpu_irq_src		hpd_irq;
>>    
>> 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 85e4f87..555d9e9f 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>>    	drm_crtc_vblank_put(&amdgpu_crtc->base);
>>    }
>>    
>> +static void dm_vupdate_high_irq(void *interrupt_params)
>> +{
>> +	struct common_irq_params *irq_params = interrupt_params;
>> +	struct amdgpu_device *adev = irq_params->adev;
>> +	struct amdgpu_crtc *acrtc;
>> +	struct dm_crtc_state *acrtc_state;
>> +
>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>> +
>> +	if (acrtc) {
>> +		acrtc_state = to_dm_crtc_state(acrtc->base.state);
>> +
>> +		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>> +				 amdgpu_dm_vrr_active(acrtc_state));
>> +
>> +		/* Core vblank handling is done here after end of front-porch in
>> +		 * vrr mode, as vblank timestamping will give valid results
>> +		 * while now done after front-porch. This will also deliver
>> +		 * page-flip completion events that have been queued to us
>> +		 * if a pageflip happened inside front-porch.
>> +		 */
>> +		if (amdgpu_dm_vrr_active(acrtc_state))
>> +			drm_crtc_handle_vblank(&acrtc->base)
> I was thinking that 3 and 4 might have to be merged, but VRR pflip
> timestamping seems to be the same as it was before (off by a line or
> two) since it's not handled here yet. This seems to fix vblank events
> and timestamping at least.
> 
>> +	}
>> +}
>> +
>>    static void dm_crtc_high_irq(void *interrupt_params)
>>    {
>>    	struct common_irq_params *irq_params = interrupt_params;
>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>    	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>    
>>    	if (acrtc) {
>> -		drm_crtc_handle_vblank(&acrtc->base);
>> -		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> -
>>    		acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>    
>> +		DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>> +				 amdgpu_dm_vrr_active(acrtc_state));
>> +
>> +		/* Core vblank handling at start of front-porch is only possible
>> +		 * in non-vrr mode, as only there vblank timestamping will give
>> +		 * valid results while done in front-porch. Otherwise defer it
>> +		 * to dm_vupdate_high_irq after end of front-porch.
>> +		 */
>> +		if (!amdgpu_dm_vrr_active(acrtc_state))
>> +			drm_crtc_handle_vblank(&acrtc->base);
>> +
>> +		/* Following stuff must happen at start of vblank, for crc
>> +		 * computation and below-the-range btr support in vrr mode.
>> +		 */
>> +		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> +
>>    		if (acrtc_state->stream &&
>>    		    acrtc_state->vrr_params.supported &&
>>    		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>>    				dm_crtc_high_irq, c_irq_params);
>>    	}
>>    
>> +	/* Use VUPDATE interrupt */
>> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
>> +		r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
>> +		if (r) {
>> +			DRM_ERROR("Failed to add vupdate irq id!\n");
>> +			return r;
>> +		}
>> +
>> +		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
>> +		int_params.irq_source =
>> +			dc_interrupt_to_irq_source(dc, i, 0);
>> +
>> +		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
>> +
>> +		c_irq_params->adev = adev;
>> +		c_irq_params->irq_src = int_params.irq_source;
>> +
>> +		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>> +				dm_vupdate_high_irq, c_irq_params);
>> +	}
>> +
>>    	/* Use GRPH_PFLIP interrupt */
>>    	for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>>    			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>    				dm_crtc_high_irq, c_irq_params);
>>    	}
>>    
>> +	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>> +	 * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
>> +	 * to trigger at end of each vblank, regardless of state of the lock,
>> +	 * matching DCE behaviour.
>> +	 */
>> +	for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
>> +	     i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
>> +	     i++) {
>> +		r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
>> +
>> +		if (r) {
>> +			DRM_ERROR("Failed to add vupdate irq id!\n");
>> +			return r;
>> +		}
>> +
>> +		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
>> +		int_params.irq_source =
>> +			dc_interrupt_to_irq_source(dc, i, 0);
>> +
>> +		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
>> +
>> +		c_irq_params->adev = adev;
>> +		c_irq_params->irq_src = int_params.irq_source;
>> +
>> +		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>> +				dm_vupdate_high_irq, c_irq_params);
>> +	}
>> +
>>    	/* Use GRPH_PFLIP interrupt */
>>    	for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
>>    			i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>    	return &state->base;
>>    }
>>    
>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>> +{
>> +	enum dc_irq_source irq_source;
>> +	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> +	struct amdgpu_device *adev = crtc->dev->dev_private;
>> +	int rc;
>> +
>> +	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>> +
>> +	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>> +
>> +	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
>> +			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);

This extra __func__ is redundant here.

>> +	return rc;
>> +}
>>    
>>    static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>    {
>>    	enum dc_irq_source irq_source;
>>    	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>    	struct amdgpu_device *adev = crtc->dev->dev_private;
>> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>> +	int rc = 0;
>> +
>> +	if (enable) {
>> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
>> +		if (amdgpu_dm_vrr_active(acrtc_state))
>> +			rc = dm_set_vupdate_irq(crtc, true);
>> +	}
>> +	else {
> 
> should be } else {
> 
>> +		/* vblank irq off -> vupdate irq off */
>> +		rc = dm_set_vupdate_irq(crtc, false);
>> +	}
>> +
>> +	if (rc)
>> +		return rc;
>>    
>>    	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>>    	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>    		 * 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.
>> +		 *
>> +		 * We also need vupdate irq for the actual core vblank handling
>> +		 * at end of vblank.
>>    		 */
>> +		dm_set_vupdate_irq(new_state->base.crtc, true);
>>    		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);
>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>    		/* Transition VRR active -> inactive:
>>    		 * Allow vblank irq disable again for fixed refresh rate.
>>    		 */
>> +		dm_set_vupdate_irq(new_state->base.crtc, false);
>>    		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);
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index f741ea3..0b35f84 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>>    	struct common_irq_params
>>    	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>>    
>> +	/**
>> +	 * @vupdate_params:
>> +	 *
>> +	 * Vertical update IRQ parameters, passed to registered handlers when
>> +	 * triggered.
>> +	 */
>> +	struct common_irq_params
>> +	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
>> +
>>    	spinlock_t irq_handler_list_table_lock;
>>    
>>    	struct backlight_device *backlight_dev;
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> index cd10f77..fee7837 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>>    		__func__);
>>    }
>>    
>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
>> +					   struct amdgpu_irq_src *source,
>> +					   unsigned crtc_id,
> 
> should be unsigned int crtc_id
> 
>> +					   enum amdgpu_interrupt_state state)
>> +{
>> +	return dm_irq_state(
>> +		adev,
>> +		source,
>> +		crtc_id,
>> +		state,
>> +		IRQ_TYPE_VUPDATE,
>> +		__func__);
>> +}
>> +
>>    static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>>    	.set = amdgpu_dm_set_crtc_irq_state,
>>    	.process = amdgpu_dm_irq_handler,
>>    };
>>    
>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
>> +	.set = amdgpu_dm_set_vupdate_irq_state,
>> +	.process = amdgpu_dm_irq_handler,
>> +};
>> +
>>    static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>>    	.set = amdgpu_dm_set_pflip_irq_state,
>>    	.process = amdgpu_dm_irq_handler,
>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>>    	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>>    	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>>    
>> +	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
>> +	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
>> +
>>    	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>>    	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>>    
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> index afe0876..86987f5 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define hpd_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>>    		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>    		.ack_value =\
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> index 1ea7256..750ba0a 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define BASE_INNER(seg) \
>>    	DCE_BASE__INST0_SEG ## seg
>>    
>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		IRQ_REG_ENTRY(CRTC, reg_num,\
>>    			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>>    			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> index 8a2066c..de218fe 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>>    
>>    #define hpd_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>    		.ack_value =\
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> index e04ae49..10ac6de 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>    		return DC_IRQ_SOURCE_VBLANK5;
>>    	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>    		return DC_IRQ_SOURCE_VBLANK6;
>> +	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE1;
>> +	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE2;
>> +	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE3;
>> +	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE4;
>> +	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE5;
>> +	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE6;

I'm not sure we necessarily want to reuse the same 
DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really 
V_UPDATE_NO_LOCK.

>>    	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>    		return DC_IRQ_SOURCE_PFLIP1;
>>    	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define BASE_INNER(seg) \
>>    	DCE_BASE__INST0_SEG ## seg
>>    
>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		.funcs = &pflip_irq_info_funcs\
>>    	}
>>    
>> -#define vupdate_int_entry(reg_num)\
>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>> + */
>> +#define vupdate_no_lock_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>    		IRQ_REG_ENTRY(OTG, reg_num,\
>> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>> -		.funcs = &vblank_irq_info_funcs\
>> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>> +		.funcs = &vupdate_no_lock_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>    	dc_underflow_int_entry(6),
>>    	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>    	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>> -	vupdate_int_entry(0),
>> -	vupdate_int_entry(1),
>> -	vupdate_int_entry(2),
>> -	vupdate_int_entry(3),
>> -	vupdate_int_entry(4),
>> -	vupdate_int_entry(5),

I don't think we should be necessarily dropping these, but I guess it 
could go either way since these IRQs technically aren't defined.

>> +	vupdate_no_lock_int_entry(0),
>> +	vupdate_no_lock_int_entry(1),
>> +	vupdate_no_lock_int_entry(2),
>> +	vupdate_no_lock_int_entry(3),
>> +	vupdate_no_lock_int_entry(4),
>> +	vupdate_no_lock_int_entry(5),
>>    	vblank_int_entry(0),
>>    	vblank_int_entry(1),
>>    	vblank_int_entry(2),
>>
> 
> Nicholas Kazlauskas
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

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

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Mario Kleiner]

Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.

On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> > On 3/18/19 1:19 PM, Mario Kleiner wrote:
> >> In VRR mode, proper vblank/pageflip timestamps can only be computed
> >> after the display scanout position has left front-porch. Therefore
> >> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> >> drm_update_vblank_count() and pageflip event delivery, to after the
> >> end of front-porch when in VRR mode.
> >>
> >> We add a new vupdate irq, which triggers at the end of the vupdate
> >> interval, ie. at the end of vblank, and calls the core vblank handler
> >> function. The new irq handler is not executed in standard non-VRR
> >> mode, so vblank handling for fixed refresh rate mode is identical
> >> to the past implementation.
> >>
> >> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
>
> Looks I lost some of my comments I wanted to send in my last email. Just
> a few nitpicks (including some things Paul mentioned).
>
> Also meant to CC Harry on this as well.
>
> >> ---
> >>    drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
> >>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
> >>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
> >>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
> >>    .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
> >>    .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
> >>    .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
> >>    .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
> >>    8 files changed, 205 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> index f88761a..64167dd 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> @@ -827,6 +827,7 @@ struct amdgpu_device {
> >>      /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
> >>      struct work_struct              hotplug_work;
> >>      struct amdgpu_irq_src           crtc_irq;
> >> +    struct amdgpu_irq_src           vupdate_irq;
> >>      struct amdgpu_irq_src           pageflip_irq;
> >>      struct amdgpu_irq_src           hpd_irq;
> >>
> >> 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 85e4f87..555d9e9f 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
> >>      drm_crtc_vblank_put(&amdgpu_crtc->base);
> >>    }
> >>
> >> +static void dm_vupdate_high_irq(void *interrupt_params)
> >> +{
> >> +    struct common_irq_params *irq_params = interrupt_params;
> >> +    struct amdgpu_device *adev = irq_params->adev;
> >> +    struct amdgpu_crtc *acrtc;
> >> +    struct dm_crtc_state *acrtc_state;
> >> +
> >> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> >> +
> >> +    if (acrtc) {
> >> +            acrtc_state = to_dm_crtc_state(acrtc->base.state);
> >> +
> >> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> >> +                             amdgpu_dm_vrr_active(acrtc_state));
> >> +
> >> +            /* Core vblank handling is done here after end of front-porch in
> >> +             * vrr mode, as vblank timestamping will give valid results
> >> +             * while now done after front-porch. This will also deliver
> >> +             * page-flip completion events that have been queued to us
> >> +             * if a pageflip happened inside front-porch.
> >> +             */
> >> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >> +                    drm_crtc_handle_vblank(&acrtc->base)
> > I was thinking that 3 and 4 might have to be merged, but VRR pflip
> > timestamping seems to be the same as it was before (off by a line or
> > two) since it's not handled here yet. This seems to fix vblank events
> > and timestamping at least.
> >
> >> +    }
> >> +}
> >> +
> >>    static void dm_crtc_high_irq(void *interrupt_params)
> >>    {
> >>      struct common_irq_params *irq_params = interrupt_params;
> >> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >>      acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> >>
> >>      if (acrtc) {
> >> -            drm_crtc_handle_vblank(&acrtc->base);
> >> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >> -
> >>              acrtc_state = to_dm_crtc_state(acrtc->base.state);
> >>
> >> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> >> +                             amdgpu_dm_vrr_active(acrtc_state));
> >> +
> >> +            /* Core vblank handling at start of front-porch is only possible
> >> +             * in non-vrr mode, as only there vblank timestamping will give
> >> +             * valid results while done in front-porch. Otherwise defer it
> >> +             * to dm_vupdate_high_irq after end of front-porch.
> >> +             */
> >> +            if (!amdgpu_dm_vrr_active(acrtc_state))
> >> +                    drm_crtc_handle_vblank(&acrtc->base);
> >> +
> >> +            /* Following stuff must happen at start of vblank, for crc
> >> +             * computation and below-the-range btr support in vrr mode.
> >> +             */
> >> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >> +
> >>              if (acrtc_state->stream &&
> >>                  acrtc_state->vrr_params.supported &&
> >>                  acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> >> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
> >>                              dm_crtc_high_irq, c_irq_params);
> >>      }
> >>
> >> +    /* Use VUPDATE interrupt */
> >> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> >> +            r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
> >> +            if (r) {
> >> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
> >> +                    return r;
> >> +            }
> >> +
> >> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> >> +            int_params.irq_source =
> >> +                    dc_interrupt_to_irq_source(dc, i, 0);
> >> +
> >> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> >> +
> >> +            c_irq_params->adev = adev;
> >> +            c_irq_params->irq_src = int_params.irq_source;
> >> +
> >> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
> >> +                            dm_vupdate_high_irq, c_irq_params);
> >> +    }
> >> +
> >>      /* Use GRPH_PFLIP interrupt */
> >>      for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
> >>                      i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> >> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> >>                              dm_crtc_high_irq, c_irq_params);
> >>      }
> >>
> >> +    /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
> >> +     * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
> >> +     * to trigger at end of each vblank, regardless of state of the lock,
> >> +     * matching DCE behaviour.
> >> +     */
> >> +    for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
> >> +         i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
> >> +         i++) {
> >> +            r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
> >> +
> >> +            if (r) {
> >> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
> >> +                    return r;
> >> +            }
> >> +
> >> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> >> +            int_params.irq_source =
> >> +                    dc_interrupt_to_irq_source(dc, i, 0);
> >> +
> >> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> >> +
> >> +            c_irq_params->adev = adev;
> >> +            c_irq_params->irq_src = int_params.irq_source;
> >> +
> >> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
> >> +                            dm_vupdate_high_irq, c_irq_params);
> >> +    }
> >> +
> >>      /* Use GRPH_PFLIP interrupt */
> >>      for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
> >>                      i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
> >> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> >>      return &state->base;
> >>    }
> >>
> >> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> >> +{
> >> +    enum dc_irq_source irq_source;
> >> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >> +    struct amdgpu_device *adev = crtc->dev->dev_private;
> >> +    int rc;
> >> +
> >> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> >> +
> >> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >> +
> >> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> >> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
>
> This extra __func__ is redundant here.
>

Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?

> >> +    return rc;
> >> +}
> >>
> >>    static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
> >>    {
> >>      enum dc_irq_source irq_source;
> >>      struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>      struct amdgpu_device *adev = crtc->dev->dev_private;
> >> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> >> +    int rc = 0;
> >> +
> >> +    if (enable) {
> >> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
> >> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >> +                    rc = dm_set_vupdate_irq(crtc, true);
> >> +    }
> >> +    else {
> >
> > should be } else {
> >
> >> +            /* vblank irq off -> vupdate irq off */
> >> +            rc = dm_set_vupdate_irq(crtc, false);
> >> +    }
> >> +
> >> +    if (rc)
> >> +            return rc;
> >>
> >>      irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
> >>      return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>               * 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.
> >> +             *
> >> +             * We also need vupdate irq for the actual core vblank handling
> >> +             * at end of vblank.
> >>               */
> >> +            dm_set_vupdate_irq(new_state->base.crtc, true);
> >>              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);
> >> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>              /* Transition VRR active -> inactive:
> >>               * Allow vblank irq disable again for fixed refresh rate.
> >>               */
> >> +            dm_set_vupdate_irq(new_state->base.crtc, false);
> >>              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);
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> index f741ea3..0b35f84 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
> >>      struct common_irq_params
> >>      vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
> >>
> >> +    /**
> >> +     * @vupdate_params:
> >> +     *
> >> +     * Vertical update IRQ parameters, passed to registered handlers when
> >> +     * triggered.
> >> +     */
> >> +    struct common_irq_params
> >> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> >> +
> >>      spinlock_t irq_handler_list_table_lock;
> >>
> >>      struct backlight_device *backlight_dev;
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> index cd10f77..fee7837 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
> >>              __func__);
> >>    }
> >>
> >> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> >> +                                       struct amdgpu_irq_src *source,
> >> +                                       unsigned crtc_id,
> >
> > should be unsigned int crtc_id
> >
> >> +                                       enum amdgpu_interrupt_state state)
> >> +{
> >> +    return dm_irq_state(
> >> +            adev,
> >> +            source,
> >> +            crtc_id,
> >> +            state,
> >> +            IRQ_TYPE_VUPDATE,
> >> +            __func__);
> >> +}
> >> +
> >>    static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
> >>      .set = amdgpu_dm_set_crtc_irq_state,
> >>      .process = amdgpu_dm_irq_handler,
> >>    };
> >>
> >> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> >> +    .set = amdgpu_dm_set_vupdate_irq_state,
> >> +    .process = amdgpu_dm_irq_handler,
> >> +};
> >> +
> >>    static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
> >>      .set = amdgpu_dm_set_pflip_irq_state,
> >>      .process = amdgpu_dm_irq_handler,
> >> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
> >>      adev->crtc_irq.num_types = adev->mode_info.num_crtc;
> >>      adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
> >>
> >> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> >> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> >> +
> >>      adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
> >>      adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> index afe0876..86987f5 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define hpd_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
> >>              .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> >> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>              .ack_value =\
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> index 1ea7256..750ba0a 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define BASE_INNER(seg) \
> >>      DCE_BASE__INST0_SEG ## seg
> >>
> >> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              IRQ_REG_ENTRY(CRTC, reg_num,\
> >>                      CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
> >>                      CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> index 8a2066c..de218fe 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >>
> >>    #define hpd_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_INVALID + reg_num] = {\
> >> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>              .ack_value =\
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> index e04ae49..10ac6de 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
> >>              return DC_IRQ_SOURCE_VBLANK5;
> >>      case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
> >>              return DC_IRQ_SOURCE_VBLANK6;
> >> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE1;
> >> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE2;
> >> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE3;
> >> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE4;
> >> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE5;
> >> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE6;
>
> I'm not sure we necessarily want to reuse the same
> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
> V_UPDATE_NO_LOCK.
>

Hm. The problem is that if we use different ones for DCE and DCN we
need special-case handling in amdgpu_dm.c, e.g., in
amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
if it is caling DCE or DCN (how to detect this?) to select different
irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
naming?

Or we'd name the IRQ source and all relevant data structures and
functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
it signals instead of to what it corresponds in hardware? That would
be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
start of vblank, but the underlying hw interrupts are actually a
properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.

Of course this all assumes we need to use the NO_LOCK variant on DCN?
We haven't tested what the regular VUPDATE_IRQ does, because i don't
have a suitable test machine, and my use of the NO_LOCK variant was
just based on my interpretation of this little comment in the DCN
header file:

#define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
"OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
for double buffered registers"

and the absence of any explanatory comment in the define of regular V_UPDATE:

#define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
OTG1_IHC_V_UPDATE_INTERRUPT

I assumed the NO_LOCK means not affected by the state of any of the hw
locks, so regular V_UPDATE might be affected by them. You agreed with
that, but we never tested what regular V_UPDATE would do on DCN. Do we
actually know for sure from hw specs that it would not work, ie. not
unconditionally fire the IRQ at every end of VBLANK, as we need?


> >>      case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
> >>              return DC_IRQ_SOURCE_PFLIP1;
> >>      case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> >> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define BASE_INNER(seg) \
> >>      DCE_BASE__INST0_SEG ## seg
> >>
> >> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              .funcs = &pflip_irq_info_funcs\
> >>      }
> >>
> >> -#define vupdate_int_entry(reg_num)\
> >> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> >> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> >> + */
> >> +#define vupdate_no_lock_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
> >>              IRQ_REG_ENTRY(OTG, reg_num,\
> >> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> >> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> >> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> >> +            .funcs = &vupdate_no_lock_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
> >>      dc_underflow_int_entry(6),
> >>      [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
> >>      [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> >> -    vupdate_int_entry(0),
> >> -    vupdate_int_entry(1),
> >> -    vupdate_int_entry(2),
> >> -    vupdate_int_entry(3),
> >> -    vupdate_int_entry(4),
> >> -    vupdate_int_entry(5),
>
> I don't think we should be necessarily dropping these, but I guess it
> could go either way since these IRQs technically aren't defined.
>

We could keep both definitions around, original vupdate_int_entry +
the new vupdate_no_lock_int_entry.

-mario

> >> +    vupdate_no_lock_int_entry(0),
> >> +    vupdate_no_lock_int_entry(1),
> >> +    vupdate_no_lock_int_entry(2),
> >> +    vupdate_no_lock_int_entry(3),
> >> +    vupdate_no_lock_int_entry(4),
> >> +    vupdate_no_lock_int_entry(5),
> >>      vblank_int_entry(0),
> >>      vblank_int_entry(1),
> >>      vblank_int_entry(2),
> >>
> >
> > Nicholas Kazlauskas
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Kazlauskas, Nicholas]

On 3/20/19 3:51 AM, Mario Kleiner wrote:
> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
> 
> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com> wrote:
>>
>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
>>>> In VRR mode, proper vblank/pageflip timestamps can only be computed
>>>> after the display scanout position has left front-porch. Therefore
>>>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
>>>> drm_update_vblank_count() and pageflip event delivery, to after the
>>>> end of front-porch when in VRR mode.
>>>>
>>>> We add a new vupdate irq, which triggers at the end of the vupdate
>>>> interval, ie. at the end of vblank, and calls the core vblank handler
>>>> function. The new irq handler is not executed in standard non-VRR
>>>> mode, so vblank handling for fixed refresh rate mode is identical
>>>> to the past implementation.
>>>>
>>>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
>>
>> Looks I lost some of my comments I wanted to send in my last email. Just
>> a few nitpicks (including some things Paul mentioned).
>>
>> Also meant to CC Harry on this as well.
>>
>>>> ---
>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>>>>     .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>>>>     .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>>>>     .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>>>>     .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>>>>     8 files changed, 205 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index f88761a..64167dd 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -827,6 +827,7 @@ struct amdgpu_device {
>>>>       /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>>>>       struct work_struct              hotplug_work;
>>>>       struct amdgpu_irq_src           crtc_irq;
>>>> +    struct amdgpu_irq_src           vupdate_irq;
>>>>       struct amdgpu_irq_src           pageflip_irq;
>>>>       struct amdgpu_irq_src           hpd_irq;
>>>>
>>>> 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 85e4f87..555d9e9f 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>>>>       drm_crtc_vblank_put(&amdgpu_crtc->base);
>>>>     }
>>>>
>>>> +static void dm_vupdate_high_irq(void *interrupt_params)
>>>> +{
>>>> +    struct common_irq_params *irq_params = interrupt_params;
>>>> +    struct amdgpu_device *adev = irq_params->adev;
>>>> +    struct amdgpu_crtc *acrtc;
>>>> +    struct dm_crtc_state *acrtc_state;
>>>> +
>>>> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>>>> +
>>>> +    if (acrtc) {
>>>> +            acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>>> +
>>>> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>>>> +                             amdgpu_dm_vrr_active(acrtc_state));
>>>> +
>>>> +            /* Core vblank handling is done here after end of front-porch in
>>>> +             * vrr mode, as vblank timestamping will give valid results
>>>> +             * while now done after front-porch. This will also deliver
>>>> +             * page-flip completion events that have been queued to us
>>>> +             * if a pageflip happened inside front-porch.
>>>> +             */
>>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    drm_crtc_handle_vblank(&acrtc->base)
>>> I was thinking that 3 and 4 might have to be merged, but VRR pflip
>>> timestamping seems to be the same as it was before (off by a line or
>>> two) since it's not handled here yet. This seems to fix vblank events
>>> and timestamping at least.
>>>
>>>> +    }
>>>> +}
>>>> +
>>>>     static void dm_crtc_high_irq(void *interrupt_params)
>>>>     {
>>>>       struct common_irq_params *irq_params = interrupt_params;
>>>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>>>       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>>>
>>>>       if (acrtc) {
>>>> -            drm_crtc_handle_vblank(&acrtc->base);
>>>> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>>> -
>>>>               acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>>>
>>>> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>>>> +                             amdgpu_dm_vrr_active(acrtc_state));
>>>> +
>>>> +            /* Core vblank handling at start of front-porch is only possible
>>>> +             * in non-vrr mode, as only there vblank timestamping will give
>>>> +             * valid results while done in front-porch. Otherwise defer it
>>>> +             * to dm_vupdate_high_irq after end of front-porch.
>>>> +             */
>>>> +            if (!amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    drm_crtc_handle_vblank(&acrtc->base);
>>>> +
>>>> +            /* Following stuff must happen at start of vblank, for crc
>>>> +             * computation and below-the-range btr support in vrr mode.
>>>> +             */
>>>> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>>> +
>>>>               if (acrtc_state->stream &&
>>>>                   acrtc_state->vrr_params.supported &&
>>>>                   acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>>>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>>>>                               dm_crtc_high_irq, c_irq_params);
>>>>       }
>>>>
>>>> +    /* Use VUPDATE interrupt */
>>>> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
>>>> +            r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
>>>> +            if (r) {
>>>> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
>>>> +                    return r;
>>>> +            }
>>>> +
>>>> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
>>>> +            int_params.irq_source =
>>>> +                    dc_interrupt_to_irq_source(dc, i, 0);
>>>> +
>>>> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
>>>> +
>>>> +            c_irq_params->adev = adev;
>>>> +            c_irq_params->irq_src = int_params.irq_source;
>>>> +
>>>> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
>>>> +                            dm_vupdate_high_irq, c_irq_params);
>>>> +    }
>>>> +
>>>>       /* Use GRPH_PFLIP interrupt */
>>>>       for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>>>>                       i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
>>>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>>>                               dm_crtc_high_irq, c_irq_params);
>>>>       }
>>>>
>>>> +    /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>>>> +     * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
>>>> +     * to trigger at end of each vblank, regardless of state of the lock,
>>>> +     * matching DCE behaviour.
>>>> +     */
>>>> +    for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
>>>> +         i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
>>>> +         i++) {
>>>> +            r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
>>>> +
>>>> +            if (r) {
>>>> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
>>>> +                    return r;
>>>> +            }
>>>> +
>>>> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
>>>> +            int_params.irq_source =
>>>> +                    dc_interrupt_to_irq_source(dc, i, 0);
>>>> +
>>>> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
>>>> +
>>>> +            c_irq_params->adev = adev;
>>>> +            c_irq_params->irq_src = int_params.irq_source;
>>>> +
>>>> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
>>>> +                            dm_vupdate_high_irq, c_irq_params);
>>>> +    }
>>>> +
>>>>       /* Use GRPH_PFLIP interrupt */
>>>>       for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
>>>>                       i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
>>>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>>>       return &state->base;
>>>>     }
>>>>
>>>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>>>> +{
>>>> +    enum dc_irq_source irq_source;
>>>> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>> +    struct amdgpu_device *adev = crtc->dev->dev_private;
>>>> +    int rc;
>>>> +
>>>> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>>>> +
>>>> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>>>> +
>>>> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
>>>> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
>>
>> This extra __func__ is redundant here.
>>
> 
> Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?
> 
>>>> +    return rc;
>>>> +}
>>>>
>>>>     static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>>>     {
>>>>       enum dc_irq_source irq_source;
>>>>       struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>       struct amdgpu_device *adev = crtc->dev->dev_private;
>>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>>>> +    int rc = 0;
>>>> +
>>>> +    if (enable) {
>>>> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
>>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    rc = dm_set_vupdate_irq(crtc, true);
>>>> +    }
>>>> +    else {
>>>
>>> should be } else {
>>>
>>>> +            /* vblank irq off -> vupdate irq off */
>>>> +            rc = dm_set_vupdate_irq(crtc, false);
>>>> +    }
>>>> +
>>>> +    if (rc)
>>>> +            return rc;
>>>>
>>>>       irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>>>>       return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>>>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>>>                * 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.
>>>> +             *
>>>> +             * We also need vupdate irq for the actual core vblank handling
>>>> +             * at end of vblank.
>>>>                */
>>>> +            dm_set_vupdate_irq(new_state->base.crtc, true);
>>>>               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);
>>>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>>>               /* Transition VRR active -> inactive:
>>>>                * Allow vblank irq disable again for fixed refresh rate.
>>>>                */
>>>> +            dm_set_vupdate_irq(new_state->base.crtc, false);
>>>>               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);
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> index f741ea3..0b35f84 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>>>>       struct common_irq_params
>>>>       vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>>>>
>>>> +    /**
>>>> +     * @vupdate_params:
>>>> +     *
>>>> +     * Vertical update IRQ parameters, passed to registered handlers when
>>>> +     * triggered.
>>>> +     */
>>>> +    struct common_irq_params
>>>> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
>>>> +
>>>>       spinlock_t irq_handler_list_table_lock;
>>>>
>>>>       struct backlight_device *backlight_dev;
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> index cd10f77..fee7837 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>>>>               __func__);
>>>>     }
>>>>
>>>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
>>>> +                                       struct amdgpu_irq_src *source,
>>>> +                                       unsigned crtc_id,
>>>
>>> should be unsigned int crtc_id
>>>
>>>> +                                       enum amdgpu_interrupt_state state)
>>>> +{
>>>> +    return dm_irq_state(
>>>> +            adev,
>>>> +            source,
>>>> +            crtc_id,
>>>> +            state,
>>>> +            IRQ_TYPE_VUPDATE,
>>>> +            __func__);
>>>> +}
>>>> +
>>>>     static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>>>>       .set = amdgpu_dm_set_crtc_irq_state,
>>>>       .process = amdgpu_dm_irq_handler,
>>>>     };
>>>>
>>>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
>>>> +    .set = amdgpu_dm_set_vupdate_irq_state,
>>>> +    .process = amdgpu_dm_irq_handler,
>>>> +};
>>>> +
>>>>     static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>>>>       .set = amdgpu_dm_set_pflip_irq_state,
>>>>       .process = amdgpu_dm_irq_handler,
>>>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>>>>       adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>>>>       adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>>>>
>>>> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
>>>> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
>>>> +
>>>>       adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>>>>       adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> index afe0876..86987f5 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define hpd_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>>>>               .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
>>>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>>               .ack_value =\
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> index 1ea7256..750ba0a 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define BASE_INNER(seg) \
>>>>       DCE_BASE__INST0_SEG ## seg
>>>>
>>>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               IRQ_REG_ENTRY(CRTC, reg_num,\
>>>>                       CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>>>>                       CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> index 8a2066c..de218fe 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>>
>>>>     #define hpd_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_INVALID + reg_num] = {\
>>>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>>               .ack_value =\
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> index e04ae49..10ac6de 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>               return DC_IRQ_SOURCE_VBLANK5;
>>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>               return DC_IRQ_SOURCE_VBLANK6;
>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>
>> I'm not sure we necessarily want to reuse the same
>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>> V_UPDATE_NO_LOCK.
>>
> 
> Hm. The problem is that if we use different ones for DCE and DCN we
> need special-case handling in amdgpu_dm.c, e.g., in
> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
> if it is caling DCE or DCN (how to detect this?) to select different
> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
> naming?
> 
> Or we'd name the IRQ source and all relevant data structures and
> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
> it signals instead of to what it corresponds in hardware? That would
> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
> start of vblank, but the underlying hw interrupts are actually a
> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.


Ah, I see what you mean. Maybe this is for the better to share the same 
names then. I'll defer this to Harry.

> 
> Of course this all assumes we need to use the NO_LOCK variant on DCN?
> We haven't tested what the regular VUPDATE_IRQ does, because i don't
> have a suitable test machine, and my use of the NO_LOCK variant was
> just based on my interpretation of this little comment in the DCN
> header file:
> 
> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
> for double buffered registers"
> 
> and the absence of any explanatory comment in the define of regular V_UPDATE:
> 
> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
> OTG1_IHC_V_UPDATE_INTERRUPT
> 
> I assumed the NO_LOCK means not affected by the state of any of the hw
> locks, so regular V_UPDATE might be affected by them. You agreed with
> that, but we never tested what regular V_UPDATE would do on DCN. Do we
> actually know for sure from hw specs that it would not work, ie. not
> unconditionally fire the IRQ at every end of VBLANK, as we need?

FWIW, I did try your original patches with V_UPDATE. I don't know off 
the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK 
doesn't, but at the very least the HW won't be flipping anything to the 
screen if you're using the former. You'll end up with a static screen 
that looks like a system hang.

Nicholas Kazlauskas

> 
> 
>>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>               return DC_IRQ_SOURCE_PFLIP1;
>>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define BASE_INNER(seg) \
>>>>       DCE_BASE__INST0_SEG ## seg
>>>>
>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               .funcs = &pflip_irq_info_funcs\
>>>>       }
>>>>
>>>> -#define vupdate_int_entry(reg_num)\
>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>> + */
>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>               IRQ_REG_ENTRY(OTG, reg_num,\
>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>       dc_underflow_int_entry(6),
>>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>> -    vupdate_int_entry(0),
>>>> -    vupdate_int_entry(1),
>>>> -    vupdate_int_entry(2),
>>>> -    vupdate_int_entry(3),
>>>> -    vupdate_int_entry(4),
>>>> -    vupdate_int_entry(5),
>>
>> I don't think we should be necessarily dropping these, but I guess it
>> could go either way since these IRQs technically aren't defined.
>>
> 
> We could keep both definitions around, original vupdate_int_entry +
> the new vupdate_no_lock_int_entry.
> 
> -mario
> 
>>>> +    vupdate_no_lock_int_entry(0),
>>>> +    vupdate_no_lock_int_entry(1),
>>>> +    vupdate_no_lock_int_entry(2),
>>>> +    vupdate_no_lock_int_entry(3),
>>>> +    vupdate_no_lock_int_entry(4),
>>>> +    vupdate_no_lock_int_entry(5),
>>>>       vblank_int_entry(0),
>>>>       vblank_int_entry(1),
>>>>       vblank_int_entry(2),
>>>>
>>>
>>> Nicholas Kazlauskas
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>

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

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Mario Kleiner]

On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 3/20/19 3:51 AM, Mario Kleiner wrote:
> > Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
> >
> > On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com> wrote:
> >>
> >> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> >>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
> >>>> In VRR mode, proper vblank/pageflip timestamps can only be computed
> >>>> after the display scanout position has left front-porch. Therefore
> >>>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> >>>> drm_update_vblank_count() and pageflip event delivery, to after the
> >>>> end of front-porch when in VRR mode.
> >>>>
> >>>> We add a new vupdate irq, which triggers at the end of the vupdate
> >>>> interval, ie. at the end of vblank, and calls the core vblank handler
> >>>> function. The new irq handler is not executed in standard non-VRR
> >>>> mode, so vblank handling for fixed refresh rate mode is identical
> >>>> to the past implementation.
> >>>>
> >>>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> >>
> >> Looks I lost some of my comments I wanted to send in my last email. Just
> >> a few nitpicks (including some things Paul mentioned).
> >>
> >> Also meant to CC Harry on this as well.
> >>
> >>>> ---
> >>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
> >>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
> >>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
> >>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
> >>>>     .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
> >>>>     .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
> >>>>     .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
> >>>>     .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
> >>>>     8 files changed, 205 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> index f88761a..64167dd 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> @@ -827,6 +827,7 @@ struct amdgpu_device {
> >>>>       /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
> >>>>       struct work_struct              hotplug_work;
> >>>>       struct amdgpu_irq_src           crtc_irq;
> >>>> +    struct amdgpu_irq_src           vupdate_irq;
> >>>>       struct amdgpu_irq_src           pageflip_irq;
> >>>>       struct amdgpu_irq_src           hpd_irq;
> >>>>
> >>>> 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 85e4f87..555d9e9f 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
> >>>>       drm_crtc_vblank_put(&amdgpu_crtc->base);
> >>>>     }
> >>>>
> >>>> +static void dm_vupdate_high_irq(void *interrupt_params)
> >>>> +{
> >>>> +    struct common_irq_params *irq_params = interrupt_params;
> >>>> +    struct amdgpu_device *adev = irq_params->adev;
> >>>> +    struct amdgpu_crtc *acrtc;
> >>>> +    struct dm_crtc_state *acrtc_state;
> >>>> +
> >>>> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> >>>> +
> >>>> +    if (acrtc) {
> >>>> +            acrtc_state = to_dm_crtc_state(acrtc->base.state);
> >>>> +
> >>>> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> >>>> +                             amdgpu_dm_vrr_active(acrtc_state));
> >>>> +
> >>>> +            /* Core vblank handling is done here after end of front-porch in
> >>>> +             * vrr mode, as vblank timestamping will give valid results
> >>>> +             * while now done after front-porch. This will also deliver
> >>>> +             * page-flip completion events that have been queued to us
> >>>> +             * if a pageflip happened inside front-porch.
> >>>> +             */
> >>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    drm_crtc_handle_vblank(&acrtc->base)
> >>> I was thinking that 3 and 4 might have to be merged, but VRR pflip
> >>> timestamping seems to be the same as it was before (off by a line or
> >>> two) since it's not handled here yet. This seems to fix vblank events
> >>> and timestamping at least.
> >>>
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>     static void dm_crtc_high_irq(void *interrupt_params)
> >>>>     {
> >>>>       struct common_irq_params *irq_params = interrupt_params;
> >>>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >>>>       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> >>>>
> >>>>       if (acrtc) {
> >>>> -            drm_crtc_handle_vblank(&acrtc->base);
> >>>> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >>>> -
> >>>>               acrtc_state = to_dm_crtc_state(acrtc->base.state);
> >>>>
> >>>> +            DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
> >>>> +                             amdgpu_dm_vrr_active(acrtc_state));
> >>>> +
> >>>> +            /* Core vblank handling at start of front-porch is only possible
> >>>> +             * in non-vrr mode, as only there vblank timestamping will give
> >>>> +             * valid results while done in front-porch. Otherwise defer it
> >>>> +             * to dm_vupdate_high_irq after end of front-porch.
> >>>> +             */
> >>>> +            if (!amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    drm_crtc_handle_vblank(&acrtc->base);
> >>>> +
> >>>> +            /* Following stuff must happen at start of vblank, for crc
> >>>> +             * computation and below-the-range btr support in vrr mode.
> >>>> +             */
> >>>> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >>>> +
> >>>>               if (acrtc_state->stream &&
> >>>>                   acrtc_state->vrr_params.supported &&
> >>>>                   acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> >>>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
> >>>>                               dm_crtc_high_irq, c_irq_params);
> >>>>       }
> >>>>
> >>>> +    /* Use VUPDATE interrupt */
> >>>> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> >>>> +            r = amdgpu_irq_add_id(adev, client_id, i, &adev->vupdate_irq);
> >>>> +            if (r) {
> >>>> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
> >>>> +                    return r;
> >>>> +            }
> >>>> +
> >>>> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> >>>> +            int_params.irq_source =
> >>>> +                    dc_interrupt_to_irq_source(dc, i, 0);
> >>>> +
> >>>> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> >>>> +
> >>>> +            c_irq_params->adev = adev;
> >>>> +            c_irq_params->irq_src = int_params.irq_source;
> >>>> +
> >>>> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
> >>>> +                            dm_vupdate_high_irq, c_irq_params);
> >>>> +    }
> >>>> +
> >>>>       /* Use GRPH_PFLIP interrupt */
> >>>>       for (i = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
> >>>>                       i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> >>>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> >>>>                               dm_crtc_high_irq, c_irq_params);
> >>>>       }
> >>>>
> >>>> +    /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
> >>>> +     * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
> >>>> +     * to trigger at end of each vblank, regardless of state of the lock,
> >>>> +     * matching DCE behaviour.
> >>>> +     */
> >>>> +    for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
> >>>> +         i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
> >>>> +         i++) {
> >>>> +            r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
> >>>> +
> >>>> +            if (r) {
> >>>> +                    DRM_ERROR("Failed to add vupdate irq id!\n");
> >>>> +                    return r;
> >>>> +            }
> >>>> +
> >>>> +            int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
> >>>> +            int_params.irq_source =
> >>>> +                    dc_interrupt_to_irq_source(dc, i, 0);
> >>>> +
> >>>> +            c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
> >>>> +
> >>>> +            c_irq_params->adev = adev;
> >>>> +            c_irq_params->irq_src = int_params.irq_source;
> >>>> +
> >>>> +            amdgpu_dm_irq_register_interrupt(adev, &int_params,
> >>>> +                            dm_vupdate_high_irq, c_irq_params);
> >>>> +    }
> >>>> +
> >>>>       /* Use GRPH_PFLIP interrupt */
> >>>>       for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
> >>>>                       i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
> >>>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> >>>>       return &state->base;
> >>>>     }
> >>>>
> >>>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> >>>> +{
> >>>> +    enum dc_irq_source irq_source;
> >>>> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>>> +    struct amdgpu_device *adev = crtc->dev->dev_private;
> >>>> +    int rc;
> >>>> +
> >>>> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> >>>> +
> >>>> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >>>> +
> >>>> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> >>>> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
> >>
> >> This extra __func__ is redundant here.
> >>
> >
> > Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?
> >
> >>>> +    return rc;
> >>>> +}
> >>>>
> >>>>     static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
> >>>>     {
> >>>>       enum dc_irq_source irq_source;
> >>>>       struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>>>       struct amdgpu_device *adev = crtc->dev->dev_private;
> >>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> >>>> +    int rc = 0;
> >>>> +
> >>>> +    if (enable) {
> >>>> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
> >>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    rc = dm_set_vupdate_irq(crtc, true);
> >>>> +    }
> >>>> +    else {
> >>>
> >>> should be } else {
> >>>
> >>>> +            /* vblank irq off -> vupdate irq off */
> >>>> +            rc = dm_set_vupdate_irq(crtc, false);
> >>>> +    }
> >>>> +
> >>>> +    if (rc)
> >>>> +            return rc;
> >>>>
> >>>>       irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
> >>>>       return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >>>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>>>                * 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.
> >>>> +             *
> >>>> +             * We also need vupdate irq for the actual core vblank handling
> >>>> +             * at end of vblank.
> >>>>                */
> >>>> +            dm_set_vupdate_irq(new_state->base.crtc, true);
> >>>>               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);
> >>>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>>>               /* Transition VRR active -> inactive:
> >>>>                * Allow vblank irq disable again for fixed refresh rate.
> >>>>                */
> >>>> +            dm_set_vupdate_irq(new_state->base.crtc, false);
> >>>>               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);
> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> index f741ea3..0b35f84 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
> >>>>       struct common_irq_params
> >>>>       vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
> >>>>
> >>>> +    /**
> >>>> +     * @vupdate_params:
> >>>> +     *
> >>>> +     * Vertical update IRQ parameters, passed to registered handlers when
> >>>> +     * triggered.
> >>>> +     */
> >>>> +    struct common_irq_params
> >>>> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> >>>> +
> >>>>       spinlock_t irq_handler_list_table_lock;
> >>>>
> >>>>       struct backlight_device *backlight_dev;
> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> index cd10f77..fee7837 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
> >>>>               __func__);
> >>>>     }
> >>>>
> >>>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> >>>> +                                       struct amdgpu_irq_src *source,
> >>>> +                                       unsigned crtc_id,
> >>>
> >>> should be unsigned int crtc_id
> >>>
> >>>> +                                       enum amdgpu_interrupt_state state)
> >>>> +{
> >>>> +    return dm_irq_state(
> >>>> +            adev,
> >>>> +            source,
> >>>> +            crtc_id,
> >>>> +            state,
> >>>> +            IRQ_TYPE_VUPDATE,
> >>>> +            __func__);
> >>>> +}
> >>>> +
> >>>>     static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
> >>>>       .set = amdgpu_dm_set_crtc_irq_state,
> >>>>       .process = amdgpu_dm_irq_handler,
> >>>>     };
> >>>>
> >>>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> >>>> +    .set = amdgpu_dm_set_vupdate_irq_state,
> >>>> +    .process = amdgpu_dm_irq_handler,
> >>>> +};
> >>>> +
> >>>>     static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
> >>>>       .set = amdgpu_dm_set_pflip_irq_state,
> >>>>       .process = amdgpu_dm_irq_handler,
> >>>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
> >>>>       adev->crtc_irq.num_types = adev->mode_info.num_crtc;
> >>>>       adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
> >>>>
> >>>> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> >>>> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> >>>> +
> >>>>       adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
> >>>>       adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> index afe0876..86987f5 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define hpd_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
> >>>>               .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> >>>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>>               .ack_value =\
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> index 1ea7256..750ba0a 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define BASE_INNER(seg) \
> >>>>       DCE_BASE__INST0_SEG ## seg
> >>>>
> >>>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               IRQ_REG_ENTRY(CRTC, reg_num,\
> >>>>                       CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
> >>>>                       CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> index 8a2066c..de218fe 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>>
> >>>>     #define hpd_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_INVALID + reg_num] = {\
> >>>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>>               .ack_value =\
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> index e04ae49..10ac6de 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
> >>>>               return DC_IRQ_SOURCE_VBLANK5;
> >>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
> >>>>               return DC_IRQ_SOURCE_VBLANK6;
> >>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE1;
> >>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE2;
> >>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE3;
> >>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE4;
> >>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE5;
> >>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE6;
> >>
> >> I'm not sure we necessarily want to reuse the same
> >> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
> >> V_UPDATE_NO_LOCK.
> >>
> >
> > Hm. The problem is that if we use different ones for DCE and DCN we
> > need special-case handling in amdgpu_dm.c, e.g., in
> > amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
> > if it is caling DCE or DCN (how to detect this?) to select different
> > irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
> > And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
> > also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
> > naming?
> >
> > Or we'd name the IRQ source and all relevant data structures and
> > functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
> > it signals instead of to what it corresponds in hardware? That would
> > be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
> > start of vblank, but the underlying hw interrupts are actually a
> > properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>
>
> Ah, I see what you mean. Maybe this is for the better to share the same
> names then. I'll defer this to Harry.
>
> >
> > Of course this all assumes we need to use the NO_LOCK variant on DCN?
> > We haven't tested what the regular VUPDATE_IRQ does, because i don't
> > have a suitable test machine, and my use of the NO_LOCK variant was
> > just based on my interpretation of this little comment in the DCN
> > header file:
> >
> > #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
> > "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
> > for double buffered registers"
> >
> > and the absence of any explanatory comment in the define of regular V_UPDATE:
> >
> > #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
> > OTG1_IHC_V_UPDATE_INTERRUPT
> >
> > I assumed the NO_LOCK means not affected by the state of any of the hw
> > locks, so regular V_UPDATE might be affected by them. You agreed with
> > that, but we never tested what regular V_UPDATE would do on DCN. Do we
> > actually know for sure from hw specs that it would not work, ie. not
> > unconditionally fire the IRQ at every end of VBLANK, as we need?
>
> FWIW, I did try your original patches with V_UPDATE. I don't know off
> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
> doesn't, but at the very least the HW won't be flipping anything to the
> screen if you're using the former. You'll end up with a static screen
> that looks like a system hang.
>

Yes, but that was because that iteration of the patch had a bug, where
i defined the irq sources / irq hw sources as _NO_LOCK variants as i
intended, but forgot to adapt the irq en/disable register macros. You
fixed my bug during your testing and so we know the _NO_LOCK variant
works perfectly for our purpose. But we never tesed if the standard
variant would have worked just as well.

I will test this later today, because since today i'm the proud owner
of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
give it a try. Would be good though if you could have a look at the
hardware docs i assume you have internally to verify what the V_UPDATE
irq hw source actually does?

-mario

> Nicholas Kazlauskas
>
> >
> >
> >>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
> >>>>               return DC_IRQ_SOURCE_PFLIP1;
> >>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> >>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define BASE_INNER(seg) \
> >>>>       DCE_BASE__INST0_SEG ## seg
> >>>>
> >>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               .funcs = &pflip_irq_info_funcs\
> >>>>       }
> >>>>
> >>>> -#define vupdate_int_entry(reg_num)\
> >>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> >>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> >>>> + */
> >>>> +#define vupdate_no_lock_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
> >>>>               IRQ_REG_ENTRY(OTG, reg_num,\
> >>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> >>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> >>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> >>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
> >>>>       dc_underflow_int_entry(6),
> >>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
> >>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> >>>> -    vupdate_int_entry(0),
> >>>> -    vupdate_int_entry(1),
> >>>> -    vupdate_int_entry(2),
> >>>> -    vupdate_int_entry(3),
> >>>> -    vupdate_int_entry(4),
> >>>> -    vupdate_int_entry(5),
> >>
> >> I don't think we should be necessarily dropping these, but I guess it
> >> could go either way since these IRQs technically aren't defined.
> >>
> >
> > We could keep both definitions around, original vupdate_int_entry +
> > the new vupdate_no_lock_int_entry.
> >
> > -mario
> >
> >>>> +    vupdate_no_lock_int_entry(0),
> >>>> +    vupdate_no_lock_int_entry(1),
> >>>> +    vupdate_no_lock_int_entry(2),
> >>>> +    vupdate_no_lock_int_entry(3),
> >>>> +    vupdate_no_lock_int_entry(4),
> >>>> +    vupdate_no_lock_int_entry(5),
> >>>>       vblank_int_entry(0),
> >>>>       vblank_int_entry(1),
> >>>>       vblank_int_entry(2),
> >>>>
> >>>
> >>> Nicholas Kazlauskas
> >>> _______________________________________________
> >>> amd-gfx mailing list
> >>> amd-gfx@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>
> >>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Wentland, Harry]

On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com> wrote:
>>
>> On 3/20/19 3:51 AM, Mario Kleiner wrote:
>>> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
>>>
>>> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
>>> <Nicholas.Kazlauskas@amd.com> wrote:
>>>>
>>>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:

...snip...

>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> index e04ae49..10ac6de 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>>>               return DC_IRQ_SOURCE_VBLANK5;
>>>>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>>>               return DC_IRQ_SOURCE_VBLANK6;
>>>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>>>
>>>> I'm not sure we necessarily want to reuse the same
>>>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>>>> V_UPDATE_NO_LOCK.
>>>>
>>>
>>> Hm. The problem is that if we use different ones for DCE and DCN we
>>> need special-case handling in amdgpu_dm.c, e.g., in
>>> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
>>> if it is caling DCE or DCN (how to detect this?) to select different
>>> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
>>> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
>>> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
>>> naming?
>>>
>>> Or we'd name the IRQ source and all relevant data structures and
>>> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
>>> it signals instead of to what it corresponds in hardware? That would
>>> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
>>> start of vblank, but the underlying hw interrupts are actually a
>>> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>>
>>
>> Ah, I see what you mean. Maybe this is for the better to share the same
>> names then. I'll defer this to Harry.
>>

I'd tend to agree with Mario. I think it's best to keep the same semantic for VUPDATE.

The regular VUPDATE has some interaction with the master lock. I imagine we want something that fires independently of it, which looks to be the _NO_LOCK version.

That said, both VSTARTUP and VUPDATE can occur before VSYNC on DCN. With most timings this probably won't occur but if our back porch is really small we'd see that happening. If we need to guarantee that this interrupt occurs no sooner than vline 0 we will need to register a vline IRQ.

As for VSTARTUP and VUPDATE, VSTARTUP is the IRQ that has an impact on the behavior of render clients as it's the deadline for frame submissions. After VSTARTUP any new framebuffer address update is postponed to the next frame (unless we do immediate flip).

So for now VUPDATE_NO_LOCK is probably fine and will improve our situation except for VRR displays with a really short back porch. Not sure those even exist.

>>>
>>> Of course this all assumes we need to use the NO_LOCK variant on DCN?
>>> We haven't tested what the regular VUPDATE_IRQ does, because i don't
>>> have a suitable test machine, and my use of the NO_LOCK variant was
>>> just based on my interpretation of this little comment in the DCN
>>> header file:
>>>
>>> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
>>> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
>>> for double buffered registers"
>>>
>>> and the absence of any explanatory comment in the define of regular V_UPDATE:
>>>
>>> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
>>> OTG1_IHC_V_UPDATE_INTERRUPT
>>>
>>> I assumed the NO_LOCK means not affected by the state of any of the hw
>>> locks, so regular V_UPDATE might be affected by them. You agreed with
>>> that, but we never tested what regular V_UPDATE would do on DCN. Do we
>>> actually know for sure from hw specs that it would not work, ie. not
>>> unconditionally fire the IRQ at every end of VBLANK, as we need?
>>
>> FWIW, I did try your original patches with V_UPDATE. I don't know off
>> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
>> doesn't, but at the very least the HW won't be flipping anything to the
>> screen if you're using the former. You'll end up with a static screen
>> that looks like a system hang.
>>
> 
> Yes, but that was because that iteration of the patch had a bug, where
> i defined the irq sources / irq hw sources as _NO_LOCK variants as i
> intended, but forgot to adapt the irq en/disable register macros. You
> fixed my bug during your testing and so we know the _NO_LOCK variant
> works perfectly for our purpose. But we never tesed if the standard
> variant would have worked just as well.
> 
> I will test this later today, because since today i'm the proud owner
> of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
> give it a try. Would be good though if you could have a look at the
> hardware docs i assume you have internally to verify what the V_UPDATE
> irq hw source actually does?
> 

I couldn't find much regarding the _NO_LOCK part in the HW docs, unfortunately. The other stuff is in my blurb up top.

I think this change is fine. With the comments from Nick and Paul addressed this is
Acked-by: Harry Wentland <harry.wentland@amd.com>

I recommend trying the VLINE IRQ, though. Nick can you create a task for us to look into that?

Harry

> -mario
> 
>> Nicholas Kazlauskas
>>
>>>
>>>
>>>>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>>>               return DC_IRQ_SOURCE_PFLIP1;
>>>>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>       .ack = NULL
>>>>>>     };
>>>>>>
>>>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>>>> +    .set = NULL,
>>>>>> +    .ack = NULL
>>>>>> +};
>>>>>> +
>>>>>>     #define BASE_INNER(seg) \
>>>>>>       DCE_BASE__INST0_SEG ## seg
>>>>>>
>>>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>               .funcs = &pflip_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>> -#define vupdate_int_entry(reg_num)\
>>>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>>>> + */
>>>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>>>               IRQ_REG_ENTRY(OTG, reg_num,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>>>> -            .funcs = &vblank_irq_info_funcs\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>>     #define vblank_int_entry(reg_num)\
>>>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>>>       dc_underflow_int_entry(6),
>>>>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>>>> -    vupdate_int_entry(0),
>>>>>> -    vupdate_int_entry(1),
>>>>>> -    vupdate_int_entry(2),
>>>>>> -    vupdate_int_entry(3),
>>>>>> -    vupdate_int_entry(4),
>>>>>> -    vupdate_int_entry(5),
>>>>
>>>> I don't think we should be necessarily dropping these, but I guess it
>>>> could go either way since these IRQs technically aren't defined.
>>>>
>>>
>>> We could keep both definitions around, original vupdate_int_entry +
>>> the new vupdate_no_lock_int_entry.
>>>
>>> -mario
>>>
>>>>>> +    vupdate_no_lock_int_entry(0),
>>>>>> +    vupdate_no_lock_int_entry(1),
>>>>>> +    vupdate_no_lock_int_entry(2),
>>>>>> +    vupdate_no_lock_int_entry(3),
>>>>>> +    vupdate_no_lock_int_entry(4),
>>>>>> +    vupdate_no_lock_int_entry(5),
>>>>>>       vblank_int_entry(0),
>>>>>>       vblank_int_entry(1),
>>>>>>       vblank_int_entry(2),
>>>>>>
>>>>>
>>>>> Nicholas Kazlauskas
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

[Kazlauskas, Nicholas]

On 3/21/19 11:38 AM, Wentland, Harry wrote:
> 
> 
> On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
>> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
>> <Nicholas.Kazlauskas@amd.com> wrote:
>>>
>>> On 3/20/19 3:51 AM, Mario Kleiner wrote:
>>>> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
>>>>
>>>> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
>>>> <Nicholas.Kazlauskas@amd.com> wrote:
>>>>>
>>>>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>>>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
> 
> ...snip...
> 
>>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> index e04ae49..10ac6de 100644
>>>>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>>>>                return DC_IRQ_SOURCE_VBLANK5;
>>>>>>>        case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>>>>                return DC_IRQ_SOURCE_VBLANK6;
>>>>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>>>>
>>>>> I'm not sure we necessarily want to reuse the same
>>>>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>>>>> V_UPDATE_NO_LOCK.
>>>>>
>>>>
>>>> Hm. The problem is that if we use different ones for DCE and DCN we
>>>> need special-case handling in amdgpu_dm.c, e.g., in
>>>> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
>>>> if it is caling DCE or DCN (how to detect this?) to select different
>>>> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
>>>> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
>>>> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
>>>> naming?
>>>>
>>>> Or we'd name the IRQ source and all relevant data structures and
>>>> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
>>>> it signals instead of to what it corresponds in hardware? That would
>>>> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
>>>> start of vblank, but the underlying hw interrupts are actually a
>>>> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>>>
>>>
>>> Ah, I see what you mean. Maybe this is for the better to share the same
>>> names then. I'll defer this to Harry.
>>>
> 
> I'd tend to agree with Mario. I think it's best to keep the same semantic for VUPDATE.
> 
> The regular VUPDATE has some interaction with the master lock. I imagine we want something that fires independently of it, which looks to be the _NO_LOCK version.
> 
> That said, both VSTARTUP and VUPDATE can occur before VSYNC on DCN. With most timings this probably won't occur but if our back porch is really small we'd see that happening. If we need to guarantee that this interrupt occurs no sooner than vline 0 we will need to register a vline IRQ.
> 
> As for VSTARTUP and VUPDATE, VSTARTUP is the IRQ that has an impact on the behavior of render clients as it's the deadline for frame submissions. After VSTARTUP any new framebuffer address update is postponed to the next frame (unless we do immediate flip).
> 
> So for now VUPDATE_NO_LOCK is probably fine and will improve our situation except for VRR displays with a really short back porch. Not sure those even exist.
Mapping the DC_IRQ_SOURCE_VUPDATE1...6 to the _NO_LOCK interrupt seems 
fine to me then.

FWIW on the few displays I tested on Raven the event was usually sent 
out rather late - near or after the end of the back porch. I don't think 
we have to necessarily worry that it'll happen before vline 0 at least.

It's more about it happening too late I suppose. A vline 0 IRQ would 
probably be ideal here I think, but this works for now.

> 
>>>>
>>>> Of course this all assumes we need to use the NO_LOCK variant on DCN?
>>>> We haven't tested what the regular VUPDATE_IRQ does, because i don't
>>>> have a suitable test machine, and my use of the NO_LOCK variant was
>>>> just based on my interpretation of this little comment in the DCN
>>>> header file:
>>>>
>>>> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
>>>> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
>>>> for double buffered registers"
>>>>
>>>> and the absence of any explanatory comment in the define of regular V_UPDATE:
>>>>
>>>> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
>>>> OTG1_IHC_V_UPDATE_INTERRUPT
>>>>
>>>> I assumed the NO_LOCK means not affected by the state of any of the hw
>>>> locks, so regular V_UPDATE might be affected by them. You agreed with
>>>> that, but we never tested what regular V_UPDATE would do on DCN. Do we
>>>> actually know for sure from hw specs that it would not work, ie. not
>>>> unconditionally fire the IRQ at every end of VBLANK, as we need?
>>>
>>> FWIW, I did try your original patches with V_UPDATE. I don't know off
>>> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
>>> doesn't, but at the very least the HW won't be flipping anything to the
>>> screen if you're using the former. You'll end up with a static screen
>>> that looks like a system hang.
>>>
>>
>> Yes, but that was because that iteration of the patch had a bug, where
>> i defined the irq sources / irq hw sources as _NO_LOCK variants as i
>> intended, but forgot to adapt the irq en/disable register macros. You
>> fixed my bug during your testing and so we know the _NO_LOCK variant
>> works perfectly for our purpose. But we never tesed if the standard
>> variant would have worked just as well.
>>
>> I will test this later today, because since today i'm the proud owner
>> of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
>> give it a try. Would be good though if you could have a look at the
>> hardware docs i assume you have internally to verify what the V_UPDATE
>> irq hw source actually does?
>>
> 
> I couldn't find much regarding the _NO_LOCK part in the HW docs, unfortunately. The other stuff is in my blurb up top.
> 
> I think this change is fine. With the comments from Nick and Paul addressed this is
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> I recommend trying the VLINE IRQ, though. Nick can you create a task for us to look into that?
> 
> Harry
Sure.

Nicholas Kazlauskas

> 
>> -mario
>>
>>> Nicholas Kazlauskas
>>>
>>>>
>>>>
>>>>>>>        case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>>>>                return DC_IRQ_SOURCE_PFLIP1;
>>>>>>>        case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>>        .ack = NULL
>>>>>>>      };
>>>>>>>
>>>>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>>>>> +    .set = NULL,
>>>>>>> +    .ack = NULL
>>>>>>> +};
>>>>>>> +
>>>>>>>      #define BASE_INNER(seg) \
>>>>>>>        DCE_BASE__INST0_SEG ## seg
>>>>>>>
>>>>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>>                .funcs = &pflip_irq_info_funcs\
>>>>>>>        }
>>>>>>>
>>>>>>> -#define vupdate_int_entry(reg_num)\
>>>>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>>>>> + */
>>>>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>>>>        [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>>>>                IRQ_REG_ENTRY(OTG, reg_num,\
>>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>>>>> -            .funcs = &vblank_irq_info_funcs\
>>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>>>>        }
>>>>>>>
>>>>>>>      #define vblank_int_entry(reg_num)\
>>>>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>>>>        dc_underflow_int_entry(6),
>>>>>>>        [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>>>>        [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>>>>> -    vupdate_int_entry(0),
>>>>>>> -    vupdate_int_entry(1),
>>>>>>> -    vupdate_int_entry(2),
>>>>>>> -    vupdate_int_entry(3),
>>>>>>> -    vupdate_int_entry(4),
>>>>>>> -    vupdate_int_entry(5),
>>>>>
>>>>> I don't think we should be necessarily dropping these, but I guess it
>>>>> could go either way since these IRQs technically aren't defined.
>>>>>
>>>>
>>>> We could keep both definitions around, original vupdate_int_entry +
>>>> the new vupdate_no_lock_int_entry.
>>>>
>>>> -mario
>>>>
>>>>>>> +    vupdate_no_lock_int_entry(0),
>>>>>>> +    vupdate_no_lock_int_entry(1),
>>>>>>> +    vupdate_no_lock_int_entry(2),
>>>>>>> +    vupdate_no_lock_int_entry(3),
>>>>>>> +    vupdate_no_lock_int_entry(4),
>>>>>>> +    vupdate_no_lock_int_entry(5),
>>>>>>>        vblank_int_entry(0),
>>>>>>>        vblank_int_entry(1),
>>>>>>>        vblank_int_entry(2),
>>>>>>>
>>>>>>
>>>>>> Nicholas Kazlauskas
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>
>>>>>
>>>

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

AMD Freesync patches v2

[Mario Kleiner]

The current patch series, with feedback from Paul, Nicholas and Harry
applied and r-b / acked-by tags added. Thanks for the feedback.
Rebased to current drm-5.2-wip branch.

Patch 1/4 is still the same though. Don't know if i or Nicholas could
fix it in a followup patch, or this one needs more work directly, or
who does what. Nicholas?

thanks,
-mario


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

[PATCH 2/4] drm/amd/display: Rework vrr flip throttling for late vblank irq.

[Mario Kleiner]

For throttling to work correctly, we always need a baseline vblank
count last_flip_vblank that increments at start of front-porch.

This is the case for drm_crtc_vblank_count() in non-VRR mode, where
the vblank irq fires at start of front-porch and triggers DRM core
vblank handling, but it is no longer the case in VRR mode, where
core vblank handling is done later, after end of front-porch.

Therefore drm_crtc_vblank_count() is no longer useful for this.
We also can't use drm_crtc_accurate_vblank_count(), as that would
screw up vblank timestamps in VRR mode when called in front-porch.

To solve this, use the cooked hardware vblank counter returned by
amdgpu_get_vblank_counter_kms() instead, as that one is cooked to
always increment at start of front-porch, independent of when
vblank related irq's fire.

This patch allows vblank irq handling to happen anywhere within
vblank of even after it, without a negative impact on flip
throttling, so followup patches can shift the vblank core
handling trigger point wherever they need it.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  2 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++--------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 889e443eeee7..add238fe4b57 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -406,7 +406,7 @@ struct amdgpu_crtc {
 	struct amdgpu_flip_work *pflip_works;
 	enum amdgpu_flip_status pflip_status;
 	int deferred_flip_completion;
-	u64 last_flip_vblank;
+	u32 last_flip_vblank;
 	/* pll sharing */
 	struct amdgpu_atom_ss ss;
 	bool ss_enabled;
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 b73313d6450f..aabd23fa0766 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -286,7 +286,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	}
 
 	/* Update to correct count(s) if racing with vblank irq */
-	amdgpu_crtc->last_flip_vblank = drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
+	drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
 
 	/* wake up userspace */
 	if (amdgpu_crtc->event) {
@@ -298,6 +298,14 @@ static void dm_pflip_high_irq(void *interrupt_params)
 	} else
 		WARN_ON(1);
 
+	/* Keep track of vblank of this flip for flip throttling. We use the
+	 * cooked hw counter, as that one incremented at start of this vblank
+	 * of pageflip completion, so last_flip_vblank is the forbidden count
+	 * for queueing new pageflips if vsync + VRR is enabled.
+	 */
+	amdgpu_crtc->last_flip_vblank = amdgpu_get_vblank_counter_kms(adev->ddev,
+							amdgpu_crtc->crtc_id);
+
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
@@ -4764,9 +4772,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 	struct amdgpu_bo *abo;
 	uint64_t tiling_flags;
-	uint32_t target, target_vblank;
-	uint64_t last_flip_vblank;
-	bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE;
+	uint32_t target_vblank, last_flip_vblank;
+	bool vrr_active = amdgpu_dm_vrr_active(acrtc_state);
 	bool pflip_present = false;
 
 	struct {
@@ -4910,7 +4917,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			 * clients using the GLX_OML_sync_control extension or
 			 * DRI3/Present extension with defined target_msc.
 			 */
-			last_flip_vblank = drm_crtc_vblank_count(pcrtc);
+			last_flip_vblank = amdgpu_get_vblank_counter_kms(dm->ddev, acrtc_attach->crtc_id);
 		}
 		else {
 			/* For variable refresh rate mode only:
@@ -4926,11 +4933,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
 
-		target = (uint32_t)last_flip_vblank + wait_for_vblank;
-
-		/* Prepare wait for target vblank early - before the fence-waits */
-		target_vblank = target - (uint32_t)drm_crtc_vblank_count(pcrtc) +
-				amdgpu_get_vblank_counter_kms(pcrtc->dev, acrtc_attach->crtc_id);
+		target_vblank = last_flip_vblank + wait_for_vblank;
 
 		/*
 		 * Wait until we're out of the vertical blank period before the one
-- 
2.17.1

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