[PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.

[Kazlauskas, Nicholas]

On 4/17/19 11:51 PM, Mario Kleiner wrote:
> Pre-DCE12 needs special treatment for BTR / low framerate
> compensation for more stable behaviour:
> 
> According to comments in the code and some testing on DCE-8
> and DCE-11, DCE-11 and earlier only apply VTOTAL_MIN/MAX
> programming with a lag of one frame, so the special BTR hw
> programming for intermediate fixed duration frames must be
> done inside the current frame at flip submission in atomic
> commit tail, ie. one vblank earlier, and the fixed refresh
> intermediate frame mode must be also terminated one vblank
> earlier on pre-DCE12 display engines.
> 
> To achieve proper termination on < DCE-12 shift the point
> when the switch-back from fixed vblank duration to variable
> vblank duration happens from the start of VBLANK (vblank irq,
> as done on DCE-12+) to back-porch or end of VBLANK (handled
> by vupdate irq handler). We must leave the switch-back code
> inside VBLANK irq for DCE12+, as before.
> 
> Doing this, we get much better behaviour of BTR for up-sweeps,
> ie. going from short to long frame durations (~high to low fps)
> and for constant framerate flips, as tested on DCE-8 and
> DCE-11. Behaviour is still not quite as good as on DCN-1
> though.
> 
> On down-sweeps, going from long to short frame durations
> (low fps to high fps) < DCE-12 is a little bit improved,
> although by far not as much as for up-sweeps and constant
> fps.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> ---

I did some debugging/testing with this patch and it certainly does 
improve things quite a bit for pre DCE12 (since this really should have 
been handled in VUPDATE before).

I have one comment inline below:


>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++++++++++++-
>   1 file changed, 31 insertions(+), 1 deletion(-)
> 
> 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 76b6e621793f..9c8c94f82b35 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -364,6 +364,7 @@ static void dm_vupdate_high_irq(void *interrupt_params)
>   	struct amdgpu_device *adev = irq_params->adev;
>   	struct amdgpu_crtc *acrtc;
>   	struct dm_crtc_state *acrtc_state;
> +	unsigned long flags;
>   
>   	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>   
> @@ -381,6 +382,22 @@ static void dm_vupdate_high_irq(void *interrupt_params)
>   		 */
>   		if (amdgpu_dm_vrr_active(acrtc_state))
>   			drm_crtc_handle_vblank(&acrtc->base);
> +
> +		if (acrtc_state->stream && adev->family < AMDGPU_FAMILY_AI &&
> +		    acrtc_state->vrr_params.supported &&
> +		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> +			spin_lock_irqsave(&adev->ddev->event_lock, flags);
> +			mod_freesync_handle_v_update(
> +			    adev->dm.freesync_module,
> +			    acrtc_state->stream,
> +			    &acrtc_state->vrr_params);
> +
> +			dc_stream_adjust_vmin_vmax(
> +			    adev->dm.dc,
> +			    acrtc_state->stream,
> +			    &acrtc_state->vrr_params.adjust);
> +			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> +		}
>   	}
>   }
>   
> @@ -390,6 +407,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   	struct amdgpu_device *adev = irq_params->adev;
>   	struct amdgpu_crtc *acrtc;
>   	struct dm_crtc_state *acrtc_state;
> +	unsigned long flags;
>   
>   	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>   
> @@ -412,9 +430,10 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   		 */
>   		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>   
> -		if (acrtc_state->stream &&
> +		if (acrtc_state->stream && adev->family >= AMDGPU_FAMILY_AI &&
>   		    acrtc_state->vrr_params.supported &&
>   		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> +			spin_lock_irqsave(&adev->ddev->event_lock, flags);
>   			mod_freesync_handle_v_update(
>   				adev->dm.freesync_module,
>   				acrtc_state->stream,
> @@ -424,6 +443,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   				adev->dm.dc,
>   				acrtc_state->stream,
>   				&acrtc_state->vrr_params.adjust);
> +			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>   		}
>   	}
>   }
> @@ -4880,6 +4900,8 @@ static void update_freesync_state_on_stream(
>   {
>   	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
>   	struct dc_info_packet vrr_infopacket = {0};
> +	struct amdgpu_device *adev = dm->adev;
> +	unsigned long flags;
>   
>   	if (!new_stream)
>   		return;
> @@ -4899,6 +4921,14 @@ static void update_freesync_state_on_stream(
>   			new_stream,
>   			flip_timestamp_in_us,
>   			&vrr_params);
> +
> +		if (adev->family < AMDGPU_FAMILY_AI &&
> +		    amdgpu_dm_vrr_active(new_crtc_state)) {
> +			spin_lock_irqsave(&adev->ddev->event_lock, flags);
> +			mod_freesync_handle_v_update(dm->freesync_module,
> +						     new_stream, &vrr_params);
> +			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> +		}

Shouldn't the locking here actually be around setting the 
new_crtc_state->adjust/vrr_infopacket/vrr_params?

I forgot the locking here before but it seems like we should be doing it 
there since all that state is accessed from the IRQs.

>   	}
>   
>   	mod_freesync_build_vrr_infopacket(
> 

Nicholas Kazlauskas