diff mbox

[1/2] drm/i915: Combine pinning after setting to the display plane

Message ID 1302847454-716-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson April 15, 2011, 6:04 a.m. UTC
We need to perform a few operations in order to move the object into the
display plane (where it can be accessed coherently by the display
engine) that are important for future safety to forbid whilst pinned. As a
result, we want to need to perform some of operations before pinning,
but some are required once we have been bound into the GTT. So combine
the pinning performed by all the callers with set_to_display_plane(), so
this complication is contained within the single function.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    3 ++-
 drivers/gpu/drm/i915/i915_gem.c      |   24 ++++++++++--------------
 drivers/gpu/drm/i915/intel_display.c |   22 +++++-----------------
 drivers/gpu/drm/i915/intel_overlay.c |    8 ++------
 4 files changed, 19 insertions(+), 38 deletions(-)

Comments

Daniel Vetter April 15, 2011, 12:11 p.m. UTC | #1
On Fri, Apr 15, 2011 at 07:04:13AM +0100, Chris Wilson wrote:
> We need to perform a few operations in order to move the object into the
> display plane (where it can be accessed coherently by the display
> engine) that are important for future safety to forbid whilst pinned. As a
> result, we want to need to perform some of operations before pinning,
> but some are required once we have been bound into the GTT. So combine
> the pinning performed by all the callers with set_to_display_plane(), so
> this complication is contained within the single function.

The latest version from -next-proposed with the fail_unpin: hunk
reinstated does indeed get rid of the cacheline dirt I've seen on my ilk.

But in that version you're calling set_to_gtt_domain which calls
flush_gpu_write_domain and then waits synchronously when
obj->pending_gpu_write is set. Which is almost guaranteed to happen for
pageflips.
-Daniel
Chris Wilson April 16, 2011, 6:26 a.m. UTC | #2
On Fri, 15 Apr 2011 14:11:25 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> But in that version you're calling set_to_gtt_domain which calls
> flush_gpu_write_domain and then waits synchronously when
> obj->pending_gpu_write is set. Which is almost guaranteed to happen for
> pageflips.

Gah. Completely destroying the sole reason why set_to_display_plane()
was first introduced.

I've slept, had some coffee, even felt inspired to add some more
comments...

Thanks,
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61ccbeb..759045a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1182,7 +1182,8 @@  int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
 				  bool write);
 int __must_check
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined);
 int i915_gem_attach_phys_object(struct drm_device *dev,
 				struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7271956..9c1ff7d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3099,21 +3099,16 @@  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
  * wait, as in modesetting process we're not supposed to be interrupted.
  */
 int
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined)
 {
-	uint32_t old_read_domains;
 	int ret;
 
-	/* Not valid to be called on unbound objects. */
-	if (obj->gtt_space == NULL)
-		return -EINVAL;
-
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
 	if (ret)
 		return ret;
 
-
 	/* Currently, we are always called from an non-interruptible context. */
 	if (pipelined != obj->ring) {
 		ret = i915_gem_object_wait_rendering(obj);
@@ -3121,14 +3116,15 @@  i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
-	i915_gem_object_flush_cpu_write_domain(obj);
-
-	old_read_domains = obj->base.read_domains;
-	obj->base.read_domains |= I915_GEM_DOMAIN_GTT;
+	ret = i915_gem_object_pin(obj, alignment, true);
+	if (ret)
+		return ret;
 
-	trace_i915_gem_object_change_domain(obj,
-					    old_read_domains,
-					    obj->base.write_domain);
+	ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	if (ret) {
+		i915_gem_object_unpin(obj);
+		return ret;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56741c6..c3e61ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1807,14 +1807,10 @@  intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	}
 
 	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_pin(obj, alignment, true);
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)
 		goto err_interruptible;
 
-	ret = i915_gem_object_set_to_display_plane(obj, pipelined);
-	if (ret)
-		goto err_unpin;
-
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
 	 * framebuffer compression.  For simplicity, we always install
@@ -5354,22 +5350,16 @@  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
-		ret = i915_gem_object_pin(obj, PAGE_SIZE, true);
-		if (ret) {
-			DRM_ERROR("failed to pin cursor bo\n");
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_set_to_display_plane(obj, NULL);
+		ret = i915_gem_object_pin_to_display_plane(obj, 0, NULL);
 		if (ret) {
 			DRM_ERROR("failed to move cursor bo into the GTT\n");
-			goto fail_unpin;
+			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
-			DRM_ERROR("failed to move cursor bo into the GTT\n");
-			goto fail_unpin;
+			DRM_ERROR("failed to release fence for cursor");
+			goto fail_locked;
 		}
 
 		addr = obj->gtt_offset;
@@ -5408,8 +5398,6 @@  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	return 0;
-fail_unpin:
-	i915_gem_object_unpin(obj);
 fail_locked:
 	mutex_unlock(&dev->struct_mutex);
 fail:
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e0903c5..67fd337 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -773,17 +773,13 @@  static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin(new_bo, PAGE_SIZE, true);
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
-	if (ret != 0)
-		goto out_unpin;
-
 	ret = i915_gem_object_put_fence(new_bo);
 	if (ret)
-		goto out_unpin;
+		return ret;
 
 	if (!overlay->active) {
 		regs = intel_overlay_map_regs(overlay);