Skip to content

Commit bd200d1

Browse files
Nicholas Kazlauskasalexdeucher
Nicholas Kazlauskas
authored andcommitted
drm/amd/display: Don't replace the dc_state for fast updates
[Why] DRM private objects have no hw_done/flip_done fencing mechanism on their own and cannot be used to sequence commits accordingly. When issuing commits that don't touch the same set of hardware resources like page-flips on different CRTCs we can run into the issue below because of this: 1. Client requests non-blocking Commit #1, has a new dc_state #1, state is swapped, commit tail is deferred to work queue 2. Client requests non-blocking Commit #2, has a new dc_state #2, state is swapped, commit tail is deferred to work queue 3. Commit #2 work starts, commit tail finishes, atomic state is cleared, dc_state #1 is freed 4. Commit #1 work starts, commit tail encounters null pointer deref on dc_state #1 In order to change the DC state as in the private object we need to ensure that we wait for all outstanding commits to finish and that any other pending commits must wait for the current one to finish as well. We do this for MEDIUM and FULL updates. But not for FAST updates, nor would we want to since it would cause stuttering from the delays. FAST updates that go through dm_determine_update_type_for_commit always create a new dc_state and lock the DRM private object if there are any changed planes. We need the old state to validate, but we don't actually need the new state here. [How] If the commit isn't a full update then the use after free can be resolved by simply discarding the new state entirely and retaining the existing one instead. With this change the sequence above can be reexamined. Commit #2 will still free Commit #1's reference, but before this happens we actually added an additional reference as part of Commit #2. If an update comes in during this that needs to change the dc_state it will need to wait on Commit #1 and Commit #2 to finish. Then it'll swap the state, finish the work in commit tail and drop the last reference on Commit #2's dc_state. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204181 Fixes: 004b393 ("drm/amd/display: Check scaling info when determing update type") Signed-off-by: Nicholas Kazlauskas <[email protected]> Acked-by: Alex Deucher <[email protected]> Reviewed-by: David Francis <[email protected]> Signed-off-by: Alex Deucher <[email protected]>
1 parent 43d10d3 commit bd200d1

File tree

1 file changed

+23
-0
lines changed

1 file changed

+23
-0
lines changed

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7347,6 +7347,29 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
73477347
ret = -EINVAL;
73487348
goto fail;
73497349
}
7350+
} else {
7351+
/*
7352+
* The commit is a fast update. Fast updates shouldn't change
7353+
* the DC context, affect global validation, and can have their
7354+
* commit work done in parallel with other commits not touching
7355+
* the same resource. If we have a new DC context as part of
7356+
* the DM atomic state from validation we need to free it and
7357+
* retain the existing one instead.
7358+
*/
7359+
struct dm_atomic_state *new_dm_state, *old_dm_state;
7360+
7361+
new_dm_state = dm_atomic_get_new_state(state);
7362+
old_dm_state = dm_atomic_get_old_state(state);
7363+
7364+
if (new_dm_state && old_dm_state) {
7365+
if (new_dm_state->context)
7366+
dc_release_state(new_dm_state->context);
7367+
7368+
new_dm_state->context = old_dm_state->context;
7369+
7370+
if (old_dm_state->context)
7371+
dc_retain_state(old_dm_state->context);
7372+
}
73507373
}
73517374

73527375
/* Must be success */

0 commit comments

Comments
 (0)