drm: Don't prepare or cleanup unchanging frame buffers [v2]

Commit Message

Keith Packard Aug. 7, 2016, 5:26 a.m. UTC

Daniel Vetter <[e-mail]> writes:

> Hm, I can't see v1 anywhere, but I think it'd be better. You can't store
> any transient state related to the current update in struct drm_plane. In
> this case the cleanup_buffers from a previous update might overlap (for
> nonblocking atomic commits) with the prepare_planes for the next one.
> Either we need special cleanup vs. error-path code, or some flag somewhere
> in the drm_plane_state.

Ok, here's pretty much the previous version, which works now that I've
fixed the intel driver. Instead of just comparing the fb's, I'm using
the framebuffer_changed function, which seems like a nice bit of
documentation if nothing else.

Comments

Daniel Vetter Aug. 8, 2016, 9 a.m. UTC | #1

On Sat, Aug 06, 2016 at 10:26:09PM -0700, Keith Packard wrote:
> Daniel Vetter <[e-mail]> writes:
> 
> > Hm, I can't see v1 anywhere, but I think it'd be better. You can't store
> > any transient state related to the current update in struct drm_plane. In
> > this case the cleanup_buffers from a previous update might overlap (for
> > nonblocking atomic commits) with the prepare_planes for the next one.
> > Either we need special cleanup vs. error-path code, or some flag somewhere
> > in the drm_plane_state.
> 
> Ok, here's pretty much the previous version, which works now that I've
> fixed the intel driver. Instead of just comparing the fb's, I'm using
> the framebuffer_changed function, which seems like a nice bit of
> documentation if nothing else.
> 

> From a75251d5762b1c200ed2f3dca2a5b00cc85bea95 Mon Sep 17 00:00:00 2001
> From: Keith Packard <[e-mail]>
> Date: Sat, 4 Jun 2016 01:16:22 -0700
> Subject: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers [v3]
> 
> When reconfiguring a plane position (as in moving the cursor), the
> frame buffer for the cursor isn't changing, so don't call the prepare
> or cleanup driver functions.
> 
> This avoids making cursor position updates block on all pending rendering.
> 
> v3: use drm_atomic_helper_framebuffer_changed in both prepare and
>     cleanup phases instead of keeping state in the plane.
> 
> cc: [e-mail]
> cc: David Airlie <[e-mail]>
> cc: Daniel Vetter <[e-mail]>
> Signed-off-by: Keith Packard <[e-mail]>

Rebased onto 4.8 while applying, which did simplify the patch a lot (4.8
is already using for_each_plane_in_state, but slightly differently).

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddfa0d1..72e50bc 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1246,18 +1246,19 @@ EXPORT_SYMBOL(drm_atomic_helper_commit);
>   * Returns:
>   * 0 on success, negative error code on failure.
>   */
> +
>  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  				     struct drm_atomic_state *state)
>  {
> -	int nplanes = dev->mode_config.num_total_plane;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
>  	int ret, i;
> +	int max_prepared_i = 0;
>  
> -	for (i = 0; i < nplanes; i++) {
> +	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
> -		struct drm_plane *plane = state->planes[i];
> -		struct drm_plane_state *plane_state = state->plane_states[i];
>  
> -		if (!plane)
> +		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
>  			continue;
>  
>  		funcs = plane->helper_private;
> @@ -1267,24 +1268,25 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  			if (ret)
>  				goto fail;
>  		}
> +		max_prepared_i = i;
>  	}
>  
>  	return 0;
>  
>  fail:
> -	for (i--; i >= 0; i--) {
> +	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
> -		struct drm_plane *plane = state->planes[i];
> -		struct drm_plane_state *plane_state = state->plane_states[i];
>  
> -		if (!plane)
> +		if (i > max_prepared_i)
> +			break;
> +
> +		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
>  			continue;
>  
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
>  			funcs->cleanup_fb(plane, plane_state);
> -
>  	}
>  
>  	return ret;
> @@ -1527,6 +1529,9 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>  	for_each_plane_in_state(old_state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
>  
> +		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
> +			continue;
> +
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
> -- 
> 2.8.1
> 

> 
> -- 
> -keith

Keith Packard Aug. 8, 2016, 3:17 p.m. UTC | #2

Daniel Vetter <[e-mail]> writes:

> Rebased onto 4.8 while applying, which did simplify the patch a lot (4.8
> is already using for_each_plane_in_state, but slightly differently).

Your rebase looks great, thanks!

Patch

From a75251d5762b1c200ed2f3dca2a5b00cc85bea95 Mon Sep 17 00:00:00 2001
From: Keith Packard <[e-mail]>
Date: Sat, 4 Jun 2016 01:16:22 -0700
Subject: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers [v3]

When reconfiguring a plane position (as in moving the cursor), the
frame buffer for the cursor isn't changing, so don't call the prepare
or cleanup driver functions.

This avoids making cursor position updates block on all pending rendering.

v3: use drm_atomic_helper_framebuffer_changed in both prepare and
    cleanup phases instead of keeping state in the plane.

cc: [e-mail]
cc: David Airlie <[e-mail]>
cc: Daniel Vetter <[e-mail]>
Signed-off-by: Keith Packard <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ddfa0d1..72e50bc 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1246,18 +1246,19 @@  EXPORT_SYMBOL(drm_atomic_helper_commit);
  * Returns:
  * 0 on success, negative error code on failure.
  */
+
 int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 				     struct drm_atomic_state *state)
 {
-	int nplanes = dev->mode_config.num_total_plane;
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
 	int ret, i;
+	int max_prepared_i = 0;
 
-	for (i = 0; i < nplanes; i++) {
+	for_each_plane_in_state(state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
-		struct drm_plane *plane = state->planes[i];
-		struct drm_plane_state *plane_state = state->plane_states[i];
 
-		if (!plane)
+		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
 			continue;
 
 		funcs = plane->helper_private;
@@ -1267,24 +1268,25 @@  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 			if (ret)
 				goto fail;
 		}
+		max_prepared_i = i;
 	}
 
 	return 0;
 
 fail:
-	for (i--; i >= 0; i--) {
+	for_each_plane_in_state(state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
-		struct drm_plane *plane = state->planes[i];
-		struct drm_plane_state *plane_state = state->plane_states[i];
 
-		if (!plane)
+		if (i > max_prepared_i)
+			break;
+
+		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
 			continue;
 
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
-
 	}
 
 	return ret;
@@ -1527,6 +1529,9 @@  void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
+		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
+			continue;
+
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
-- 
2.8.1