On 2019-01-07 12:41 p.m., Nicholas Kazlauskas wrote:
The prepare_fb call always happens on new_plane_state.
The drm_atomic_helper_cleanup_planes checks to see if plane state pointer has changed when deciding to call cleanup_fb on either the new_plane_state or the old_plane_state.
For a non-async atomic commit the state pointer is swapped, so this helper calls prepare_fb on the new_plane_state and cleanup_fb on the old_plane_state. This makes sense, since we want to prepare the framebuffer we are going to use and cleanup the the framebuffer we are no longer using.
For the async atomic update helpers this differs. The async atomic update helpers perform in-place updates on the existing state. They call drm_atomic_helper_cleanup_planes but the state pointer is not swapped. This means that prepare_fb is called on the new_plane_state and cleanup_fb is called on the new_plane_state (not the old).
In the case where old_plane_state->fb == new_plane_state->fb then there should be no behavioral difference between an async update and a non-async commit. But there are issues that arise when old_plane_state->fb != new_plane_state->fb.
The first is that the new_plane_state->fb is immediately cleaned up after it has been prepared, so we're using a fb that we shouldn't be.
The second occurs during a sequence of async atomic updates and non-async regular atomic commits. Suppose there are two framebuffers being interleaved in a double-buffering scenario, fb1 and fb2:
- Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
- Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
- Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
We call cleanup_fb on fb2 twice in this example scenario, and any further use will result in use-after-free.
The simple fix to this problem is to block framebuffer changes in the drm_atomic_helper_async_check function for now.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Harry Wentland harry.wentland@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: stable@vger.kernel.org # v4.14+ Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update") Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Patch is merged to drm-misc-fixes with Daniel's RB from IRC and Andrey's and my AB.
Harry
drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 54e2ae614dcc..f4290f6b0c38 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev, old_plane_state->crtc != new_plane_state->crtc) return -EINVAL;
- /*
* FIXME: Since prepare_fb and cleanup_fb are always called on* the new_plane_state for async updates we need to block framebuffer* changes. This prevents use of a fb that's been cleaned up and* double cleanups from occuring.*/- if (old_plane_state->fb != new_plane_state->fb)
return -EINVAL;- funcs = plane->helper_private; if (!funcs->atomic_async_update) return -EINVAL;