[PATCH] drm/amd/display: Always get CRTC updated constant values inside commit tail

[Harry Wentland]

On 2020-11-17 10:51 a.m., Rodrigo Siqueira wrote:
> We recently improved our display atomic commit and tail sequence to
> avoid some issues related to concurrency. One of the major changes
> consisted of moving the interrupt disable and the stream release from
> our atomic commit to our atomic tail (commit 6d90a208cfff
> ("drm/amd/display: Move disable interrupt into commit tail")) .
> However, the new code introduced inside our commit tail function was
> inserted right after the function
> drm_atomic_helper_update_legacy_modeset_state(), which has routines for
> updating internal data structs related to timestamps. As a result, in
> certain conditions, the display module can reach a situation where we
> update our constants and, after that, clean it. This situation generates
> the following warning:
> 
>   amdgpu 0000:03:00.0: drm_WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev))
>   WARNING: CPU: 6 PID: 1269 at drivers/gpu/drm/drm_vblank.c:722
>   drm_crtc_vblank_helper_get_vblank_timestamp_internal+0x32b/0x340 [drm]
>   ...
>   RIP:
>   0010:drm_crtc_vblank_helper_get_vblank_timestamp_internal+0x32b/0x340
>   [drm]
>   ...
>   Call Trace:
>    ? dc_stream_get_vblank_counter+0x57/0x60 [amdgpu]
>    drm_crtc_vblank_helper_get_vblank_timestamp+0x1c/0x20 [drm]
>    drm_get_last_vbltimestamp+0xad/0xc0 [drm]
>    drm_reset_vblank_timestamp+0x63/0xd0 [drm]
>    drm_crtc_vblank_on+0x85/0x150 [drm]
>    amdgpu_dm_atomic_commit_tail+0xaf1/0x2330 [amdgpu]
>    commit_tail+0x99/0x130 [drm_kms_helper]
>    drm_atomic_helper_commit+0x123/0x150 [drm_kms_helper]
>    amdgpu_dm_atomic_commit+0x11/0x20 [amdgpu]
>    drm_atomic_commit+0x4a/0x50 [drm]
>    drm_atomic_helper_set_config+0x7c/0xc0 [drm_kms_helper]
>    drm_mode_setcrtc+0x20b/0x7e0 [drm]
>    ? tomoyo_path_number_perm+0x6f/0x200
>    ? drm_mode_getcrtc+0x190/0x190 [drm]
>    drm_ioctl_kernel+0xae/0xf0 [drm]
>    drm_ioctl+0x245/0x400 [drm]
>    ? drm_mode_getcrtc+0x190/0x190 [drm]
>    amdgpu_drm_ioctl+0x4e/0x80 [amdgpu]
>    __x64_sys_ioctl+0x91/0xc0
>    do_syscall_64+0x38/0x90
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
>   ...
> 
> For fixing this issue we rely upon a refactor introduced on
> drm_atomic_helper_update_legacy_modeset_state ("Remove the timestamping
> constant update from drm_atomic_helper_update_legacy_modeset_state()")
> which decouples constant values update from
> drm_atomic_helper_update_legacy_modeset_state to a new helper.
> Basically, this commit uses this new helper and place it right after our
> release module to avoid a situation where our CRTC struct gets wrong
> values.
> 
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>

Good work.

Reviewed-by: Harry Wentland <harry.wentland at amd.com>

Harry

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
>   1 file changed, 2 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 1772adcf9f98..91f7fdeee758 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8121,7 +8121,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   	trace_amdgpu_dm_atomic_commit_tail_begin(state);
>   
>   	drm_atomic_helper_update_legacy_modeset_state(dev, state);
> -	drm_atomic_helper_calc_timestamping_constants(state);
>   
>   	dm_state = dm_atomic_get_new_state(state);
>   	if (dm_state && dm_state->context) {
> @@ -8148,6 +8147,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   		}
>   	}
>   
> +	drm_atomic_helper_calc_timestamping_constants(state);
> +
>   	/* update changed items */
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>