On 12/14/18 10:01 AM, Michel Dänzer wrote: > On 2018-12-14 3:48 p.m., Nicholas Kazlauskas wrote: >> [Why] >> The behavior of dm_plane_helper_cleanup_fb differs depending on >> whether the commit was asynchronous or not. When it's called from >> amdgpu_dm_atomic_commit_tail during a typical atomic commit the >> plane state has been swapped so it calls cleanup_fb on the old plane >> state. >> >> However, in the asynchronous commit codepath the call to >> drm_atomic_helper_commit also calls dm_plane_helper_cleanup_fb after >> atomic_async_update has been called. Since the plane state has not >> been swapped, the cleanup_fb call affects the new plane state with the >> active fb. >> >> The slow, full path is only ever used for the cursor update when the >> cursor CRTC changes. If there was previously a fb in the state before >> that had gone through the fast asynchronous path then cleanup_fb will >> be called a second time on an fb that was previously unpinned/unref'd. >> >> This results in a use after free on the next cursor update. >> >> [How] >> Regardless of whether this was intentional behavior in DRM or not, >> the fix is to manually call cleanup_fb on the old plane state's fb. >> >> Since the pointer to the old_plane_state is actually the current >> plane->state in this function, a backup is needed. This has a minor >> change to behavior for handle_cursor_update since it now correctly uses >> the old state. >> >> The next step is to prevent the cleanup_fb call from DRM from unpinning >> the active new_state->fb. Setting new_state->fb to NULL would be >> enough to handle this, but DRM has a warning in the >> drm_atomic_helper_async_commit helper if >> plane->state->fb != new_state->fb. >> >> A new field was added (cursor_update) to dm_plane_state that's only >> ever set to true during the fast path. If it's true, then cleanup_fb >> doesn't unpin and unref the fb. >> >> Cc: Leo Li <sunpeng.li@xxxxxxx> >> Cc: Harry Wentland <harry.wentland@xxxxxxx> >> Cc: Michel Dänzer <michel.daenzer@xxxxxxx> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> >> >> [...] >> >> @@ -3653,7 +3654,15 @@ static void dm_plane_atomic_async_update(struct drm_plane *plane, >> plane->state->crtc_w = new_state->crtc_w; >> plane->state->crtc_h = new_state->crtc_h; >> >> - handle_cursor_update(plane, old_state); >> + handle_cursor_update(plane, &old_state); >> + >> + /* >> + * While DRM already takes care of drm_atomic_helper_cleanup_planes, >> + * it does it on the wrong state (new_state instead of old_state). >> + * This is a workaround for that behavior. >> + */ >> + dm_plane_helper_cleanup_fb(plane, &old_state); >> + dm_plane_state->cursor_update = true; > > We shouldn't work around DRM core code in driver code. Please work on a > solution with the core code developers on the dri-devel list, and revert > the cursor fast path for now. > > I'd rather not revert the patch given how much it improves the user experience. Some compositors (and wine games) are unusable or unplayable without it. In general I agree with not doing core workarounds, but I'm not even sure if this is a bug in core DRM to begin with. Since the plane state modification is in place if you free the old_state then you're freeing what's currently active on plane->state. This works for us since we only work on the ->fb during prepare/cleanup, but this isn't true in general. The only other way to do this is not using async_commit_check/async_commit_update and reimplementing atomic commit behavior for the fast path. Intel does this with their legacy cursor update fast path - it's a lot more code and it involves using "deprecated" elements of the atomic API. Nicholas Kazlauskas _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
