• Shirish S's avatar
    drm/amd/display: Signal hw_done() after waiting for flip_done() · 987bf116
    Shirish S authored
    In amdgpu_dm_commit_tail(), wait until flip_done() is signaled before
    we signal hw_done().
    
    [Why]
    
    This is to temporarily address a paging error that occurs when a
    nonblocking commit contends with another commit, particularly in a
    mirrored display configuration where at least 2 CRTCs are updated.
    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 work on both CRTCs |
                                        |
    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: default avatarShirish S <shirish.s@amd.com>
    Signed-off-by: default avatarLeo Li <sunpeng.li@amd.com>
    Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
    Signed-off-by: default avatarAlex Deucher <alexander.deucher@amd.com>
    987bf116
amdgpu_dm.c 146 KB