drm: don't block fb changes for async plane updates

Commit Message

Helen Mae Koike Fornazier June 10, 2019, 1:36 p.m. UTC

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: <[e-mail]> # v4.14+
Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
Suggested-by: Boris Brezillon <[e-mail]>
Signed-off-by: Helen Koike <[e-mail]>
Reviewed-by: Boris Brezillon <[e-mail]>
Reviewed-by: Nicholas Kazlauskas <[e-mail]>
Signed-off-by: Boris Brezillon <[e-mail]>
Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
---

Hi,

This patch failed to apply on kernel stable v4.14, I'm re-sending it
fixing the conflict.

Thanks
Helen

 drivers/gpu/drm/drm_atomic_helper.c      | 10 ++++++++++
 include/drm/drm_modeset_helper_vtables.h |  8 ++++++++
 2 files changed, 18 insertions(+)

Comments

Helen Mae Koike Fornazier June 10, 2019, 1:39 p.m. UTC | #1

Hi,

Correcting the tag, just for the record, is v5 of the patch.

On 6/10/19 10:36 AM, Helen Koike wrote:
> 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: <[e-mail]> # v4.14+
> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
> Suggested-by: Boris Brezillon <[e-mail]>
> Signed-off-by: Helen Koike <[e-mail]>
> Reviewed-by: Boris Brezillon <[e-mail]>
> Reviewed-by: Nicholas Kazlauskas <[e-mail]>
> Signed-off-by: Boris Brezillon <[e-mail]>
> Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
> ---
> 
> Hi,
> 
> This patch failed to apply on kernel stable v4.14, I'm re-sending it
> fixing the conflict.

fyi, this patch was already applied in other stable branches.

Thanks
Helen

> 
> Thanks
> Helen
> 
>  drivers/gpu/drm/drm_atomic_helper.c      | 10 ++++++++++
>  include/drm/drm_modeset_helper_vtables.h |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d05ed0521e20..331478bd2ff8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1462,6 +1462,8 @@ EXPORT_SYMBOL(drm_atomic_helper_async_check);
>   * 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)
> @@ -1472,8 +1474,16 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
>  	int i;
>  
>  	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		struct drm_framebuffer *old_fb = plane->state->fb;
> +
>  		funcs = plane->helper_private;
>  		funcs->atomic_async_update(plane, plane_state);
> +
> +		/*
> +		 * 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);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c55cf3ff6847..2f6d0f3ae985 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1159,6 +1159,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
>

Greg KH June 13, 2019, 7:50 a.m. UTC | #2

On Mon, Jun 10, 2019 at 10:36:27AM -0300, Helen Koike wrote:
> 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: <[e-mail]> # v4.14+
> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
> Suggested-by: Boris Brezillon <[e-mail]>
> Signed-off-by: Helen Koike <[e-mail]>
> Reviewed-by: Boris Brezillon <[e-mail]>
> Reviewed-by: Nicholas Kazlauskas <[e-mail]>
> Signed-off-by: Boris Brezillon <[e-mail]>
> Link: https://patchwork.freedesktop.org/patch/msgid/[e-mail]
> ---
> 
> Hi,
> 
> This patch failed to apply on kernel stable v4.14, I'm re-sending it
> fixing the conflict.

Now applied, thanks.

greg k-h

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d05ed0521e20..331478bd2ff8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1462,6 +1462,8 @@  EXPORT_SYMBOL(drm_atomic_helper_async_check);
  * 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)
@@ -1472,8 +1474,16 @@  void drm_atomic_helper_async_commit(struct drm_device *dev,
 	int i;
 
 	for_each_new_plane_in_state(state, plane, plane_state, i) {
+		struct drm_framebuffer *old_fb = plane->state->fb;
+
 		funcs = plane->helper_private;
 		funcs->atomic_async_update(plane, plane_state);
+
+		/*
+		 * 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);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index c55cf3ff6847..2f6d0f3ae985 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1159,6 +1159,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