Re: [PATCH 3/5] drm/atomic: add a drm_atomic_clean_old_fb helper.

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 11, 2015 at 11:29:09AM +0100, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> This is useful for all the boilerplate code about cleaning old_fb.
> ---
>  drivers/gpu/drm/drm_atomic.c | 58 ++++++++++++++++++++++++++++++--------------
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0ac31b1ecb67..aeee083c7f95 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1432,6 +1432,45 @@ static int atomic_set_prop(struct drm_atomic_state *state,
>  	return ret;
>  }
>  
> +/**
> + * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
> + *
> + * @dev: drm device to check.
> + * @plane_mask: plane mask for planes that were updated.
> + * @ret: return value, can be -EDEADLK for a retry.
> + *
> + * Before doing an update plane->old_fb is set to plane->fb,
> + * but before dropping the locks old_fb needs to be set to NULL
> + * and plane->fb updated. This is a common operation for each
> + * atomic update, so this call is split off as a helper.
> + */
> +void drm_atomic_clean_old_fb(struct drm_device *dev,
> +			     unsigned plane_mask,
> +			     int ret)

A drm_atomic_prepare_old_fb inline to encapsulate the prep step would have
been neat imo. Anyway: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +{
> +	struct drm_plane *plane;
> +
> +	/* if succeeded, fixup legacy plane crtc/fb ptrs before dropping
> +	 * locks (ie. while it is still safe to deref plane->state).  We
> +	 * need to do this here because the driver entry points cannot
> +	 * distinguish between legacy and atomic ioctls.
> +	 */
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		if (ret == 0) {
> +			struct drm_framebuffer *new_fb = plane->state->fb;
> +			if (new_fb)
> +				drm_framebuffer_reference(new_fb);
> +			plane->fb = new_fb;
> +			plane->crtc = plane->state->crtc;
> +
> +			if (plane->old_fb)
> +				drm_framebuffer_unreference(plane->old_fb);
> +		}
> +		plane->old_fb = NULL;
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1577,24 +1616,7 @@ retry:
>  	}
>  
>  out:
> -	/* if succeeded, fixup legacy plane crtc/fb ptrs before dropping
> -	 * locks (ie. while it is still safe to deref plane->state).  We
> -	 * need to do this here because the driver entry points cannot
> -	 * distinguish between legacy and atomic ioctls.
> -	 */
> -	drm_for_each_plane_mask(plane, dev, plane_mask) {
> -		if (ret == 0) {
> -			struct drm_framebuffer *new_fb = plane->state->fb;
> -			if (new_fb)
> -				drm_framebuffer_reference(new_fb);
> -			plane->fb = new_fb;
> -			plane->crtc = plane->state->crtc;
> -
> -			if (plane->old_fb)
> -				drm_framebuffer_unreference(plane->old_fb);
> -		}
> -		plane->old_fb = NULL;
> -	}
> +	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  
>  	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>  		/*
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e67aeac2aee0..4b74c97d297a 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -136,6 +136,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
>  
>  void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
>  
> +void
> +drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret);
> +
>  int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
>  int __must_check drm_atomic_commit(struct drm_atomic_state *state);
>  int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx