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

[Daniel Vetter]

On Sat, Aug 06, 2016 at 10:26:09PM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@xxxxxxxx> 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 <keithp@xxxxxxxxxx>
> 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: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> cc: David Airlie <airlied@xxxxxxxx>
> cc: Daniel Vetter <daniel@xxxxxxxx>
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>

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




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