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

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>]

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