Re: [PATCH 7/9] drm/i915: Consolidate plane 'cleanup' operations

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> All plane update functions need to unpin the old framebuffer when
> flipping to a new one.  Pull this logic into a separate function to ease
> the integration with atomic plane helpers.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++++++++----------
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   drivers/gpu/drm/i915/intel_sprite.c  | 27 ++++++-----------
>   3 files changed, 52 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 50d4299..20890a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11634,6 +11634,31 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   	return ret;
>   }
>
> +/**
> + * intel_cleanup_plane_fb - Cleans up an fb after plane use
> + * @plane: drm plane to clean up for
> + * @fb: old framebuffer that was on plane
> + *
> + * Cleans up a framebuffer that has just been removed from a plane.
> + */
> +void
> +intel_cleanup_plane_fb(struct drm_plane *plane,
> +		       struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
> +	if (WARN_ON(!obj))
> +		return;
> +
> +	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
> +	    !INTEL_INFO(dev)->cursor_needs_physical) {
> +		mutex_lock(&dev->struct_mutex);
> +		intel_unpin_fb_obj(obj);
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +}
> +
>   static int
>   intel_check_primary_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11661,9 +11686,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *old_fb = plane->fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
>
> @@ -11734,15 +11757,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   		intel_update_fbc(dev);
>   		mutex_unlock(&dev->struct_mutex);
>   	}
> -
> -	if (old_fb && old_fb != fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>   }
>
>   static int
> @@ -11752,6 +11766,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>   			     uint32_t src_x, uint32_t src_y,
>   			     uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -11799,6 +11814,13 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>
>   	intel_commit_primary_plane(plane, &state);
>
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);

So now we wait for vblank even if there's no fb to unpin?

Ander


> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
> @@ -11980,11 +12002,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	else
>   		addr = obj->phys_handle->busaddr;
>
> -	if (intel_crtc->cursor_bo) {
> -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -	}
> -
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> @@ -12009,6 +12026,7 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   			  uint32_t src_x, uint32_t src_y,
>   			  uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane_state state;
> @@ -12049,6 +12067,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>
>   	intel_commit_cursor_plane(plane, &state);
>
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e9f4dc1..299fc4f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -926,6 +926,8 @@ void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>   void intel_check_page_flip(struct drm_device *dev, int pipe);
>   int intel_prepare_plane_fb(struct drm_plane *plane,
>   			   struct drm_framebuffer *fb);
> +void intel_cleanup_plane_fb(struct drm_plane *plane,
> +			    struct drm_framebuffer *fb);
>
>   /* shared dpll functions */
>   struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 5d8c2e0..152a32d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1268,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	enum pipe pipe = intel_crtc->pipe;
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>   	int crtc_x, crtc_y;
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
> @@ -1326,23 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   		if (!primary_was_enabled && primary_enabled)
>   			intel_post_enable_primary(crtc);
>   	}
> -
> -	/* Unpin old obj after new one is active to avoid ugliness */
> -	if (old_obj && old_obj != obj) {
> -
> -		/*
> -		 * It's fairly common to simply update the position of
> -		 * an existing object.  In that case, we don't need to
> -		 * wait for vblank to avoid ugliness, we only need to
> -		 * do the pin & ref bookkeeping.
> -		 */
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>   }
>
>   static int
> @@ -1352,6 +1334,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		   uint32_t src_x, uint32_t src_y,
>   		   uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -1390,6 +1373,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	}
>
>   	intel_commit_sprite_plane(plane, &state);
> +
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx