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

Message ID 1469952534-6516-1-git-send-email-keithp@keithp.com
State New
Headers show

Commit Message

Keith Packard July 31, 2016, 8:08 a.m. UTC
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.

v2: Track which planes have been prepared to know which to
    cleanup. Otherwise, failure paths and success paths would need
    different tests in the cleanup code as the plane state points to
    different places in the two cases.

cc: dri-devel@lists.freedesktop.org
cc: David Airlie <airlied@linux.ie>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 23 +++++++++++++----------
 include/drm/drm_crtc.h              |  1 +
 2 files changed, 14 insertions(+), 10 deletions(-)

Comments

Daniel Vetter Aug. 2, 2016, 1:54 p.m. UTC | #1
On Sun, Jul 31, 2016 at 01:08:54AM -0700, Keith Packard wrote:
> 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.

Oops, I thought we've had this, but nope :(

> v2: Track which planes have been prepared to know which to
>     cleanup. Otherwise, failure paths and success paths would need
>     different tests in the cleanup code as the plane state points to
>     different places in the two cases.
> 
> cc: dri-devel@lists.freedesktop.org
> cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Keith Packard <keithp@keithp.com>

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.

btw if you go with the flag, vc4 could be cleaned up to use that too. At
least I thought Eric hand-rolled this logic in there somewhere, can't find
it quickly right now.

Another bit worth considering: We could use this "has the plane switched"
instead ->legacy_cursor_update in drm_atomic_helper_wait_for_vblanks -
some drivers have iommus which get really angry when we unpin too early,
and your patch alone might be good enough.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 23 +++++++++++++----------
>  include/drm/drm_crtc.h              |  1 +
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddfa0d1..f7f3a51 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1246,18 +1246,20 @@ 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;
>  
> -	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)
> +		plane->prepared = false;
> +
> +		if (plane->state->fb == plane_state->fb)
>  			continue;
>  
>  		funcs = plane->helper_private;
> @@ -1267,24 +1269,22 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  			if (ret)
>  				goto fail;
>  		}
> +		plane->prepared = true;
>  	}
>  
>  	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 (!plane->prepared)
>  			continue;
>  
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
>  			funcs->cleanup_fb(plane, plane_state);
> -
>  	}
>  
>  	return ret;
> @@ -1527,6 +1527,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 (!plane->prepared)
> +			continue;
> +
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index d1559cd..08b2033 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1531,6 +1531,7 @@ struct drm_plane {
>  	uint32_t *format_types;
>  	unsigned int format_count;
>  	bool format_default;
> +	bool prepared;
>  
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb;
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ddfa0d1..f7f3a51 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1246,18 +1246,20 @@  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;
 
-	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)
+		plane->prepared = false;
+
+		if (plane->state->fb == plane_state->fb)
 			continue;
 
 		funcs = plane->helper_private;
@@ -1267,24 +1269,22 @@  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 			if (ret)
 				goto fail;
 		}
+		plane->prepared = true;
 	}
 
 	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 (!plane->prepared)
 			continue;
 
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, plane_state);
-
 	}
 
 	return ret;
@@ -1527,6 +1527,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 (!plane->prepared)
+			continue;
+
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d1559cd..08b2033 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1531,6 +1531,7 @@  struct drm_plane {
 	uint32_t *format_types;
 	unsigned int format_count;
 	bool format_default;
+	bool prepared;
 
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;