Message-ID: <20160802135417.GO6232@phenom.ffwll.local>
Date:	Tue, 2 Aug 2016 15:54:17 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Keith Packard <keithp@...thp.com>
Cc:	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers
 [v2]

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@...ts.freedesktop.org
> cc: David Airlie <airlied@...ux.ie>
> Signed-off-by: Keith Packard <keithp@...thp.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@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists