Re: [PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

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

On 11/24/2014 09:53 PM, Matt Roper wrote:
> Primary and sprite planes have already been refactored to include a
> 'prepare' step which handles all the commit-time operations that could
> fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
> similar manner.
>
> For simplicity and consistency with other plane types, we also switch to
> using intel_pin_and_fence_fb_obj() to perform our pinning for
> non-physical cursors.  This will allow us to more easily migrate the
> code into the atomic 'begin' handler in a plane-agnostic manner in a
> future patchset.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++---------------------
>   1 file changed, 44 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17788f8..ff94e43 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   }
>
>   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->fb;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->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));

Shouldn't the frontbuffer bits be updated in the else case too? At least 
they were, prior to this patch.

> +		} 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)
>   {
>   	struct drm_crtc *crtc = state->crtc;
>   	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>   	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->fb);
>   	enum pipe pipe = intel_crtc->pipe;
>   	unsigned old_width;
>   	uint32_t addr;
> -	int ret;
>
>   	crtc->cursor_x = state->orig_dst.x1;
>   	crtc->cursor_y = state->orig_dst.y1;
> @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	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");
> +	if (!obj)
>   		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;
> -		}
> -
> +	else if (!INTEL_INFO(dev)->cursor_needs_physical)
>   		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;
> -		}
> +	else
>   		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);

This is now being called without holding dev->struct_mutex. I don't know 
if that is necessary or not.


Ander

>   	}
>
> -	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:
> @@ -12013,13 +11988,6 @@ update:
>
>   		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 int
> @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	return intel_commit_cursor_plane(plane, &state);
> +	ret = intel_prepare_cursor_plane(plane, &state);
> +	if (ret)
> +		return ret;
> +
> +	intel_commit_cursor_plane(plane, &state);
> +
> +	return 0;
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>

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