Re: [PATCH 6/9] drm/i915: Consolidate plane 'prepare' functions

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

On 11/24/2014 09:53 PM, Matt Roper wrote:
> The 'prepare' step for all types of planes are pretty similar;
> consolidate the three 'prepare' functions into a single function.  This
> paves the way for future integration with the atomic plane handlers.
>
> Note that we pull the 'wait for pending flips' functionality out of the
> primary plane's prepare step and place it directly in the 'setplane'
> code.  When we move to the atomic plane handlers, this code will be in
> the 'atomic begin' step.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++----------------
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
>   drivers/gpu/drm/i915/intel_sprite.c  |  44 ++-------
>   3 files changed, 99 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 217d1e3..50d4299 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11574,6 +11574,66 @@ disable_unpin:
>   	return 0;
>   }
>
> +/**
> + * intel_prepare_plane_fb - Prepare fb for usage on plane
> + * @plane: drm plane to prepare for
> + * @fb: framebuffer to prepare for presentation
> + *
> + * Prepares a framebuffer for usage on a display plane.  Generally this
> + * involves pinning the underlying object and updating the frontbuffer tracking
> + * bits.  Some older platforms need special physical address handling for
> + * cursor planes.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int
> +intel_prepare_plane_fb(struct drm_plane *plane,
> +		       struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	enum pipe pipe = intel_plane->pipe;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> +	unsigned frontbuffer_bits = 0;
> +	int ret;
> +
> +	if (WARN_ON(fb == plane->fb || !obj))
> +		return 0;
> +
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
> +		break;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	    INTEL_INFO(dev)->cursor_needs_physical) {
> +		int align = IS_I830(dev) ? 16 * 1024 : 256;
> +		ret = i915_gem_object_attach_phys(obj, align);
> +		if (ret)
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +	} else {
> +		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +		if (ret == 0)
> +			i915_gem_track_fb(old_obj, obj, frontbuffer_bits);
> +		else
> +			DRM_DEBUG_KMS("pin & fence failed\n");
> +	}

As commented on a previous patch, shouldn't i915_gem_track_fb() be 
called on the INTEL_INFO(dev)->cursor_needs_physical case too?

I can't see anything else wrong with the patch.

Ander

> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;
> +}
> +
>   static int
>   intel_check_primary_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11591,42 +11651,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>   					     false, true, &state->visible);
>   }
>
> -static int
> -intel_prepare_primary_plane(struct drm_plane *plane,
> -			    struct intel_plane_state *state)
> -{
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	int ret;
> -
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> -	}
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret != 0) {
> -			DRM_DEBUG_KMS("pin & fence failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void
>   intel_commit_primary_plane(struct drm_plane *plane,
>   			   struct intel_plane_state *state)
> @@ -11728,6 +11752,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_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int ret;
> @@ -11759,9 +11784,18 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	intel_crtc_wait_for_pending_flips(crtc);
> +
> +	if (intel_crtc_has_pending_flip(crtc)) {
> +		DRM_ERROR("pipe is still busy with an old pageflip\n");
> +		return -EBUSY;
> +	}
> +
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_primary_plane(plane, &state);
>
> @@ -11898,40 +11932,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   	return ret;
>   }
>
> -static int
> -intel_prepare_cursor_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	enum pipe pipe = intel_crtc->pipe;
> -	int ret = 0;
> -
> -	if (old_obj != obj) {
> -		/* we only need to pin inside GTT if cursor is non-phy */
> -		mutex_lock(&dev->struct_mutex);
> -		if (!INTEL_INFO(dev)->cursor_needs_physical) {
> -			if (obj)
> -				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -			if (ret == 0)
> -				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> -						  INTEL_FRONTBUFFER_CURSOR(pipe));
> -		} else {
> -			int align = IS_I830(dev) ? 16 * 1024 : 256;
> -			if (obj)
> -				ret = i915_gem_object_attach_phys(obj, align);
> -			if (ret)
> -				DRM_DEBUG_KMS("failed to attach phys object\n");
> -		}
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	return ret;
> -}
> -
>   static void
>   intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11941,6 +11941,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>   	enum pipe pipe = intel_crtc->pipe;
>   	unsigned old_width;
>   	uint32_t addr;
> @@ -11961,6 +11962,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	if (intel_crtc->cursor_bo == obj)
>   		goto update;
>
> +	/*
> +	 * 'prepare' is only called when fb != NULL; we still need to update
> +	 * frontbuffer tracking for the 'disable' case here.
> +	 */
> +	if (!obj) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(old_obj, NULL,
> +				  INTEL_FRONTBUFFER_CURSOR(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
>   	if (!obj)
>   		addr = 0;
>   	else if (!INTEL_INFO(dev)->cursor_needs_physical)
> @@ -11997,6 +12009,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_framebuffer *old_fb = plane->fb;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane_state state;
>   	int ret;
> @@ -12028,9 +12041,11 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_cursor_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_cursor_plane(plane, &state);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a6eafb4..e9f4dc1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -924,6 +924,8 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
>   void intel_finish_page_flip(struct drm_device *dev, int pipe);
>   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);
>
>   /* 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 fc96d13..5d8c2e0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1257,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	return 0;
>   }
>
> -static int
> -intel_prepare_sprite_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane = to_intel_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 ret;
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/* Note that this will apply the VT-d workaround for scanouts,
> -		 * which is more restrictive than required for sprites. (The
> -		 * primary plane requires 256KiB alignment with 64 PTE padding,
> -		 * the sprite planes only require 128KiB alignment and 32 PTE
> -		 * padding.
> -		 */
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   static void
>   intel_commit_sprite_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -1387,6 +1352,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_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int ret;
> @@ -1417,9 +1383,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_sprite_plane(plane, &state);
>   	return 0;
>

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