| From 89a4aac0ab0e6f5eea10d7bf4869dd15c3de2cd4 Mon Sep 17 00:00:00 2001 |
| From: Helen Koike <helen.koike@collabora.com> |
| Date: Mon, 3 Jun 2019 13:56:10 -0300 |
| Subject: drm: don't block fb changes for async plane updates |
| |
| From: Helen Koike <helen.koike@collabora.com> |
| |
| commit 89a4aac0ab0e6f5eea10d7bf4869dd15c3de2cd4 upstream. |
| |
| In the case of a normal sync update, the preparation of framebuffers (be |
| it calling drm_atomic_helper_prepare_planes() or doing setups with |
| drm_framebuffer_get()) are performed in the new_state and the respective |
| cleanups are performed in the old_state. |
| |
| In the case of async updates, the preparation is also done in the |
| new_state but the cleanups are done in the new_state (because updates |
| are performed in place, i.e. in the current state). |
| |
| The current code blocks async udpates when the fb is changed, turning |
| async updates into sync updates, slowing down cursor updates and |
| introducing regressions in igt tests with errors of type: |
| |
| "CRITICAL: completed 97 cursor updated in a period of 30 flips, we |
| expect to complete approximately 15360 updates, with the threshold set |
| at 7680" |
| |
| Fb changes in async updates were prevented to avoid the following scenario: |
| |
| - 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 (wrong) |
| Where we have a single call to prepare fb2 but double cleanup call to fb2. |
| |
| To solve the above problems, instead of blocking async fb changes, we |
| place the old framebuffer in the new_state object, so when the code |
| performs cleanups in the new_state it will cleanup the old_fb and we |
| will have the following scenario instead: |
| |
| - Async update, oldfb = NULL, newfb = fb1, prepare fb1, no cleanup |
| - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb1 |
| - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 |
| |
| Where calls to prepare/cleanup are balanced. |
| |
| Cc: <stable@vger.kernel.org> # v4.14+ |
| Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates") |
| Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> |
| Signed-off-by: Helen Koike <helen.koike@collabora.com> |
| Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> |
| Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> |
| Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> |
| Link: https://patchwork.freedesktop.org/patch/msgid/20190603165610.24614-6-helen.koike@collabora.com |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/gpu/drm/drm_atomic_helper.c | 22 ++++++++++++---------- |
| include/drm/drm_modeset_helper_vtables.h | 8 ++++++++ |
| 2 files changed, 20 insertions(+), 10 deletions(-) |
| |
| --- a/drivers/gpu/drm/drm_atomic_helper.c |
| +++ b/drivers/gpu/drm/drm_atomic_helper.c |
| @@ -1607,15 +1607,6 @@ int drm_atomic_helper_async_check(struct |
| 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; |
| @@ -1646,6 +1637,8 @@ EXPORT_SYMBOL(drm_atomic_helper_async_ch |
| * drm_atomic_async_check() succeeds. Async commits are not supposed to swap |
| * the states like normal sync commits, but just do in-place changes on the |
| * current state. |
| + * |
| + * TODO: Implement full swap instead of doing in-place changes. |
| */ |
| void drm_atomic_helper_async_commit(struct drm_device *dev, |
| struct drm_atomic_state *state) |
| @@ -1656,6 +1649,9 @@ void drm_atomic_helper_async_commit(stru |
| int i; |
| |
| for_each_new_plane_in_state(state, plane, plane_state, i) { |
| + struct drm_framebuffer *new_fb = plane_state->fb; |
| + struct drm_framebuffer *old_fb = plane->state->fb; |
| + |
| funcs = plane->helper_private; |
| funcs->atomic_async_update(plane, plane_state); |
| |
| @@ -1664,11 +1660,17 @@ void drm_atomic_helper_async_commit(stru |
| * plane->state in-place, make sure at least common |
| * properties have been properly updated. |
| */ |
| - WARN_ON_ONCE(plane->state->fb != plane_state->fb); |
| + WARN_ON_ONCE(plane->state->fb != new_fb); |
| WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x); |
| WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y); |
| WARN_ON_ONCE(plane->state->src_x != plane_state->src_x); |
| WARN_ON_ONCE(plane->state->src_y != plane_state->src_y); |
| + |
| + /* |
| + * Make sure the FBs have been swapped so that cleanups in the |
| + * new_state performs a cleanup in the old FB. |
| + */ |
| + WARN_ON_ONCE(plane_state->fb != old_fb); |
| } |
| } |
| EXPORT_SYMBOL(drm_atomic_helper_async_commit); |
| --- a/include/drm/drm_modeset_helper_vtables.h |
| +++ b/include/drm/drm_modeset_helper_vtables.h |
| @@ -1178,6 +1178,14 @@ struct drm_plane_helper_funcs { |
| * current one with the new plane configurations in the new |
| * plane_state. |
| * |
| + * Drivers should also swap the framebuffers between current plane |
| + * state (&drm_plane.state) and new_state. |
| + * This is required since cleanup for async commits is performed on |
| + * the new state, rather than old state like for traditional commits. |
| + * Since we want to give up the reference on the current (old) fb |
| + * instead of our brand new one, swap them in the driver during the |
| + * async commit. |
| + * |
| * FIXME: |
| * - It only works for single plane updates |
| * - Async Pageflips are not supported yet |