[PATCH] drm/amd/display: Don't cleanup new cursor fb in fast cursor update

[Nicholas Kazlauskas]

[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 at amd.com>
Cc: Harry Wentland <harry.wentland at amd.com>
Cc: Michel Dänzer <michel.daenzer at amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++++++----
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h   |  2 ++
 2 files changed, 15 insertions(+), 4 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 d01315965af0..b71c834a959d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3589,9 +3589,10 @@ static void dm_plane_helper_cleanup_fb(struct drm_plane *plane,
 				       struct drm_plane_state *old_state)
 {
 	struct amdgpu_bo *rbo;
+	struct dm_plane_state *dm_plane_state = to_dm_plane_state(old_state);
 	int r;
 
-	if (!old_state->fb)
+	if (!old_state->fb || dm_plane_state->cursor_update)
 		return;
 
 	rbo = gem_to_amdgpu_bo(old_state->fb->obj[0]);
@@ -3638,8 +3639,8 @@ static int dm_plane_atomic_async_check(struct drm_plane *plane,
 static void dm_plane_atomic_async_update(struct drm_plane *plane,
 					 struct drm_plane_state *new_state)
 {
-	struct drm_plane_state *old_state =
-		drm_atomic_get_old_plane_state(new_state->state, plane);
+	struct drm_plane_state old_state = *plane->state;
+	struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_state);
 
 	if (plane->state->fb != new_state->fb)
 		drm_atomic_set_fb_for_plane(plane->state, new_state->fb);
@@ -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;
 }
 
 static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 25bb91ee80ba..ecac91036864 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -254,6 +254,8 @@ struct dc_plane_state;
 struct dm_plane_state {
 	struct drm_plane_state base;
 	struct dc_plane_state *dc_state;
+
+	bool cursor_update;
 };
 
 struct dm_crtc_state {
-- 
2.17.1