[PATCH v2] drm/amd/display: Signal hw_done() after waiting for flip_done()

[Harry Wentland]

On 2018-10-01 07:26 PM, sunpeng.li at amd.com wrote:
> From: Shirish S <shirish.s at amd.com>
> 
> In amdgpu_dm_commit_tail(), wait until flip_done() is signaled before
> we signal hw_done().
> 
> [Why]
> 
> This is to temporary address a paging error that occurs when a

s/temporary/temporarily/g

> nonblocking commit contends with another commit, particularly in a
> mirrored multi-display configuration. The error occurs in
> drm_atomic_helper_wait_for_flip_done(), when we attempt to access the
> contents of new_crtc_state->commit.
> 
> Here's the sequence for a mirrored 2 display setup (irrelevant steps
> left out for clarity):
> 
> **THREAD 1**                        | **THREAD 2**
>                                     |
> Initialize atomic state for flip    |
>                                     |
> Queue worker                        |
>                                    ...
> 
>                                     | Do work for flip
>                                     |
>                                     | Signal hw_done() on CRTC 1
>                                     | Signal hw_done() on CRTC 2
>                                     |
>                                     | Wait for flip_done() on CRTC 1
> 
>                                 <---- **PREEMPTED BY THREAD 1**
> 
> Initialize atomic state for cursor
> update (1)                          |
>                                     |
> Do cursor update worker             |
>                                     |
> Clear atomic state (2)              |
> **DONE**                            |
>                                    ...
>                                     |
>                                     | Wait for flip_done() on CRTC 2
>                                     | *ERROR*
>                                     |
> 
> The issue starts with (1). When the atomic state is initialized, the
> current CRTC states are duplicated to be the new_crtc_states, and
> referenced to be the old_crtc_states. (The new_crtc_states are to be
> filled with update data.)
> 
> Some things to note:
> 
> * Due to the mirrored configuration, the cursor updates on both CRTCs.
> 
> * At this point, the pflip IRQ has already been handled, and flip_done
>   signaled on all CRTCs. The cursor commit can therefore continue.
> 
> * The old_crtc_states used by the cursor update are the **same states**
>   as the new_crtc_states used by the flip worker.
> 
> At (2), the old_crtc_state is freed (*), and the cursor commit
> completes. We then context switch back to the flip worker, where we
> attempt to access the new_crtc_state->commit object. This is
> problematic, as this state has already been freed.
> 
> (*) Technically, 'state->crtcs[i].state' is freed, which was made to
>     reference old_crtc_state in drm_atomic_helper_swap_state()
> 
> [How]
> 
> By moving hw_done() after wait_for_flip_done(), we're guaranteed that
> the new_crtc_state (from the flip worker's perspective) still exists.
> This is because any other commit will be blocked, waiting for the
> hw_done() signal.
> 
> Note that both the i915 and imx drivers have this sequence flipped
> already, masking this problem.
> 
> Signed-off-by: Shirish S <shirish.s at amd.com>
> Signed-off-by: Leo Li <sunpeng.li at amd.com>

Thanks for the additional explanation. Took me a while to understand what was happening here. Let's keep pursuing a proper solution that allows us to flip hw_done and wait_for_flip_done again but since that might be non-trivial this change is

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

Harry

> ---
> 
> Hi Shirish,
> 
> Sorry for the late reply. I took some time to dig at this issue, and found
> that a proper fix will need more thought. I'm OK with this fix for now, until
> we can address it with a proper fix.
> 
> I've updated the commit message and comments below to reflect the underlying
> issue.
> 
> Thanks,
> Leo
> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 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 774db34..bbfffaf 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4755,12 +4755,18 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>  
> -	/* Signal HW programming completion */
> -	drm_atomic_helper_commit_hw_done(state);
>  
>  	if (wait_for_vblank)
>  		drm_atomic_helper_wait_for_flip_done(dev, state);
>  
> +	/*
> +	 * FIXME:
> +	 * Delay hw_done() until flip_done() is signaled. This is to block
> +	 * another commit from freeing the CRTC state while we're still
> +	 * waiting on flip_done.
> +	 */
> +	drm_atomic_helper_commit_hw_done(state);
> +
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  
>  	/*
>