Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Merge it into the plane update_plane() callback and make other
> users use the update_plane() functions instead.
> 
> The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> and merge both paths into one.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 221 +++++++++++++++++------------------
>  1 file changed, 106 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f91a5b0..a583abd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
>  	return true;
>  }
>  
> -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> -				     struct drm_i915_gem_object *obj,
> -				     uint32_t width, uint32_t height)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	unsigned old_width;
> -	uint32_t addr;
> -	int ret;
> -
> -	/* if we want to turn off the cursor ignore width and height */
> -	if (!obj) {
> -		DRM_DEBUG_KMS("cursor off\n");
> -		addr = 0;
> -		mutex_lock(&dev->struct_mutex);
> -		goto finish;
> -	}
> -
> -	/* we only need to pin inside GTT if cursor is non-phy */
> -	mutex_lock(&dev->struct_mutex);
> -	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> -		unsigned alignment;
> -
> -		/*
> -		 * Global gtt pte registers are special registers which actually
> -		 * forward writes to a chunk of system memory. Which means that
> -		 * there is no risk that the register values disappear as soon
> -		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> -		 * only the pin/unpin/fence and not more.
> -		 */
> -		intel_runtime_pm_get(dev_priv);
> -
> -		/* Note that the w/a also requires 2 PTE of padding following
> -		 * the bo. We currently fill all unused PTE with the shadow
> -		 * page and so we should always have valid PTE following the
> -		 * cursor preventing the VT-d warning.
> -		 */
> -		alignment = 0;
> -		if (need_vtd_wa(dev))
> -			alignment = 64*1024;
> -
> -		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_locked;
> -		}
> -
> -		ret = i915_gem_object_put_fence(obj);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to release fence for cursor");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_unpin;
> -		}
> -
> -		addr = i915_gem_obj_ggtt_offset(obj);
> -
> -		intel_runtime_pm_put(dev_priv);
> -	} else {
> -		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");
> -			goto fail_locked;
> -		}
> -		addr = obj->phys_handle->busaddr;
> -	}
> -
> - finish:
> -	if (intel_crtc->cursor_bo) {
> -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -	}
> -
> -	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> -			  INTEL_FRONTBUFFER_CURSOR(pipe));
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	old_width = intel_crtc->cursor_width;
> -
> -	intel_crtc->cursor_addr = addr;
> -	intel_crtc->cursor_bo = obj;
> -	intel_crtc->cursor_width = width;
> -	intel_crtc->cursor_height = height;
> -
> -	if (intel_crtc->active) {
> -		if (old_width != width)
> -			intel_update_watermarks(crtc);
> -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -	}
> -
> -	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> -
> -	return 0;
> -fail_unpin:
> -	i915_gem_object_unpin_from_display_plane(obj);
> -fail_locked:
> -	mutex_unlock(&dev->struct_mutex);
> -	return ret;
> -}
> -
>  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
>  				 u16 *blue, uint32_t start, uint32_t size)
>  {
> @@ -11897,7 +11794,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
>  
>  	BUG_ON(!plane->crtc);
>  
> -	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> +					  0, 0, 0, 0, 0, 0, 0, 0);
>  }
>  
>  static int
> @@ -11961,26 +11859,119 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_framebuffer *fb = state->fb;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> -	struct drm_i915_gem_object *obj = intel_fb->obj;
> -	int crtc_w, crtc_h;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	enum pipe pipe = intel_crtc->pipe;
> +	unsigned old_width;
> +	uint32_t addr;
> +	bool on;
> +	int ret;
>  
>  	crtc->cursor_x = state->orig_dst.x1;
>  	crtc->cursor_y = state->orig_dst.y1;
> -	if (fb != crtc->cursor->fb) {
> -		crtc_w = drm_rect_width(&state->orig_dst);
> -		crtc_h = drm_rect_height(&state->orig_dst);
> -		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +
> +	if (intel_crtc->cursor_bo == obj)
> +		goto update;
> +
> +	/* if we want to turn off the cursor ignore width and height */
> +	if (!obj) {
> +		DRM_DEBUG_KMS("cursor off\n");
> +		addr = 0;
> +		mutex_lock(&dev->struct_mutex);
> +		goto finish;
> +	}
> +
> +	/* we only need to pin inside GTT if cursor is non-phy */
> +	mutex_lock(&dev->struct_mutex);
> +	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> +		unsigned alignment;
> +
> +		/*
> +		 * Global gtt pte registers are special registers which actually
> +		 * forward writes to a chunk of system memory. Which means that
> +		 * there is no risk that the register values disappear as soon
> +		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> +		 * only the pin/unpin/fence and not more.
> +		 */
> +		intel_runtime_pm_get(dev_priv);
> +
> +		/* Note that the w/a also requires 2 PTE of padding following
> +		 * the bo. We currently fill all unused PTE with the shadow
> +		 * page and so we should always have valid PTE following the
> +		 * cursor preventing the VT-d warning.
> +		 */
> +		alignment = 0;
> +		if (need_vtd_wa(dev))
> +			alignment = 64*1024;
> +
> +		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> +		if (ret) {
> +			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> +			intel_runtime_pm_put(dev_priv);
> +			goto fail_locked;
> +		}
> +
> +		ret = i915_gem_object_put_fence(obj);
> +		if (ret) {
> +			DRM_DEBUG_KMS("failed to release fence for cursor");
> +			intel_runtime_pm_put(dev_priv);
> +			goto fail_unpin;
> +		}
> +
> +		addr = i915_gem_obj_ggtt_offset(obj);
> +
> +		intel_runtime_pm_put(dev_priv);
>  	} else {
> -		intel_crtc_update_cursor(crtc, state->visible);
> +		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");
> +			goto fail_locked;
> +		}
> +		addr = obj->phys_handle->busaddr;
> +	}
>  
> -		intel_frontbuffer_flip(crtc->dev,
> -				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> +finish:
> +	if (intel_crtc->cursor_bo) {
> +		if (!INTEL_INFO(dev)->cursor_needs_physical)
> +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> +	}
>  
> -		return 0;
> +	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> +			  INTEL_FRONTBUFFER_CURSOR(pipe));
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_crtc->cursor_addr = addr;
> +	intel_crtc->cursor_bo = obj;
> +update:
> +	old_width = intel_crtc->cursor_width;
> +
> +	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> +	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> +
> +	if (intel_crtc->cursor_bo == obj)
> +		on = state->visible;
> +	else
> +		on = !obj;

That looks fishy. Why do we need to care if the bo changed here, and why
would we turn on the cursor when there's no bo?

The cursor is either visible or it isn't, and that's all that
intel_crtc_update_cursor() should care about.

> +
> +	if (intel_crtc->active) {
> +		if (old_width != intel_crtc->cursor_width)
> +			intel_update_watermarks(crtc);
> +		intel_crtc_update_cursor(crtc, on);
>  	}
> +
> +	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));

This should probably be done only when crtc->active==true. I see we're
not doing it that way currently, so I guess we could have a separate
patch to change that.

> +
> +	return 0;
> +fail_unpin:
> +	i915_gem_object_unpin_from_display_plane(obj);
> +fail_locked:
> +	mutex_unlock(&dev->struct_mutex);
> +	drm_gem_object_unreference_unlocked(&obj->base);
> +	return ret;
>  }
>  
>  static int
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC