All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb()
@ 2016-11-15  8:58 Chris Wilson
  2016-11-15  8:58 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  8:58 UTC (permalink / raw)
  To: intel-gfx

In the next patch, a few rearrangements are made to make these static.
First, we move them so the changes are not lost in the noise.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 255 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 2 files changed, 130 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb9377de456e..8e04f31bf12e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14144,6 +14144,134 @@ static int intel_atomic_check(struct drm_device *dev,
 	return calc_watermark_data(state);
 }
 
+/**
+ * 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.
+ *
+ * Must be called with struct_mutex held.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int
+intel_prepare_plane_fb(struct drm_plane *plane,
+		       struct drm_plane_state *new_state)
+{
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(new_state->state);
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
+	int ret;
+
+	if (!obj && !old_obj)
+		return 0;
+
+	if (old_obj) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(new_state->state,
+							   plane->state->crtc);
+
+		/* Big Hammer, we also need to ensure that any pending
+		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
+		 * current scanout is retired before unpinning the old
+		 * framebuffer. Note that we rely on userspace rendering
+		 * into the buffer attached to the pipe they are waiting
+		 * on. If not, userspace generates a GPU hang with IPEHR
+		 * point to the MI_WAIT_FOR_EVENT.
+		 *
+		 * This should only fail upon a hung GPU, in which case we
+		 * can safely continue.
+		 */
+		if (needs_modeset(crtc_state)) {
+			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
+							      old_obj->resv, NULL,
+							      false, 0,
+							      GFP_KERNEL);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	if (new_state->fence) { /* explicit fencing */
+		ret = i915_sw_fence_await_dma_fence(&intel_state->commit_ready,
+						    new_state->fence,
+						    I915_FENCE_TIMEOUT,
+						    GFP_KERNEL);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!obj)
+		return 0;
+
+	if (!new_state->fence) { /* implicit fencing */
+		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
+						      obj->resv, NULL,
+						      false, I915_FENCE_TIMEOUT,
+						      GFP_KERNEL);
+		if (ret < 0)
+			return ret;
+
+		i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
+	}
+
+	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
+		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			return ret;
+		}
+	} else {
+		struct i915_vma *vma;
+
+		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
+		if (IS_ERR(vma)) {
+			DRM_DEBUG_KMS("failed to pin object\n");
+			return PTR_ERR(vma);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * 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.
+ *
+ * Must be called with struct_mutex held.
+ */
+void
+intel_cleanup_plane_fb(struct drm_plane *plane,
+		       struct drm_plane_state *old_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	struct intel_plane_state *old_intel_state;
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
+	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
+
+	old_intel_state = to_intel_plane_state(old_state);
+
+	if (!obj && !old_obj)
+		return;
+
+	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
+	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
+		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
+}
+
+
 static int intel_atomic_prepare_commit(struct drm_device *dev,
 				       struct drm_atomic_state *state)
 {
@@ -14716,133 +14844,6 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
 
-/**
- * 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.
- *
- * Must be called with struct_mutex held.
- *
- * Returns 0 on success, negative error code on failure.
- */
-int
-intel_prepare_plane_fb(struct drm_plane *plane,
-		       struct drm_plane_state *new_state)
-{
-	struct intel_atomic_state *intel_state =
-		to_intel_atomic_state(new_state->state);
-	struct drm_i915_private *dev_priv = to_i915(plane->dev);
-	struct drm_framebuffer *fb = new_state->fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
-	int ret;
-
-	if (!obj && !old_obj)
-		return 0;
-
-	if (old_obj) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_existing_crtc_state(new_state->state,
-							   plane->state->crtc);
-
-		/* Big Hammer, we also need to ensure that any pending
-		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
-		 * current scanout is retired before unpinning the old
-		 * framebuffer. Note that we rely on userspace rendering
-		 * into the buffer attached to the pipe they are waiting
-		 * on. If not, userspace generates a GPU hang with IPEHR
-		 * point to the MI_WAIT_FOR_EVENT.
-		 *
-		 * This should only fail upon a hung GPU, in which case we
-		 * can safely continue.
-		 */
-		if (needs_modeset(crtc_state)) {
-			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
-							      old_obj->resv, NULL,
-							      false, 0,
-							      GFP_KERNEL);
-			if (ret < 0)
-				return ret;
-		}
-	}
-
-	if (new_state->fence) { /* explicit fencing */
-		ret = i915_sw_fence_await_dma_fence(&intel_state->commit_ready,
-						    new_state->fence,
-						    I915_FENCE_TIMEOUT,
-						    GFP_KERNEL);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (!obj)
-		return 0;
-
-	if (!new_state->fence) { /* implicit fencing */
-		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
-						      obj->resv, NULL,
-						      false, I915_FENCE_TIMEOUT,
-						      GFP_KERNEL);
-		if (ret < 0)
-			return ret;
-
-		i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
-	}
-
-	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
-	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
-		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			return ret;
-		}
-	} else {
-		struct i915_vma *vma;
-
-		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
-		if (IS_ERR(vma)) {
-			DRM_DEBUG_KMS("failed to pin object\n");
-			return PTR_ERR(vma);
-		}
-	}
-
-	return 0;
-}
-
-/**
- * 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.
- *
- * Must be called with struct_mutex held.
- */
-void
-intel_cleanup_plane_fb(struct drm_plane *plane,
-		       struct drm_plane_state *old_state)
-{
-	struct drm_i915_private *dev_priv = to_i915(plane->dev);
-	struct intel_plane_state *old_intel_state;
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
-	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
-
-	old_intel_state = to_intel_plane_state(old_state);
-
-	if (!obj && !old_obj)
-		return;
-
-	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
-		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
-}
-
 int
 skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 75252ecaa613..4cb8254c66dd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -772,6 +772,8 @@ struct intel_plane {
 	int max_downscale;
 	uint32_t frontbuffer_bit;
 
+	struct i915_sw_fence *last_commit;
+
 	/* Since we need to change the watermarks before/after
 	 * enabling/disabling the planes, we need to store the parameters here
 	 * as the other pieces of the struct may not reflect the values we want
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
@ 2016-11-15  8:58 ` Chris Wilson
  2016-11-15  9:37   ` Daniel Vetter
  2016-11-15  8:58 ` [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state Chris Wilson
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  8:58 UTC (permalink / raw)
  To: intel-gfx

The generic atomic helper likes to skip a prepare_plane_fb() if it
decides that the plane->fb is unchanged. This is wrong for us for a
couple of reasons:

 - if the pipe is reconfigured (i.e. a size change) but the framebuffer
   is untouched, we still have to flush any rendering prior to the
   reconfiguration to prevent wait-for-scanline GPU hangs

 - if the framebuffer is rotated, it remains the same but has a
   different view and a different address.

Finally, even if the framebuffer is unchanged the flip/modeset should be
ordered with respect to rendering to the frontbuffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  2 --
 drivers/gpu/drm/i915/intel_display.c      | 60 ++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h          |  4 ---
 3 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index ff821649486e..89a34350a35d 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -201,8 +201,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 }
 
 const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
-	.prepare_fb = intel_prepare_plane_fb,
-	.cleanup_fb = intel_cleanup_plane_fb,
 	.atomic_check = intel_plane_atomic_check,
 	.atomic_update = intel_plane_atomic_update,
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8e04f31bf12e..dca1e0b512e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14158,7 +14158,7 @@ static int intel_atomic_check(struct drm_device *dev,
  *
  * Returns 0 on success, negative error code on failure.
  */
-int
+static int
 intel_prepare_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *new_state)
 {
@@ -14252,7 +14252,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  *
  * Must be called with struct_mutex held.
  */
-void
+static void
 intel_cleanup_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *old_state)
 {
@@ -14271,6 +14271,49 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
 }
 
+static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i, j, ret;
+
+	ret = mutex_lock_interruptible(&state->dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ret = intel_prepare_plane_fb(plane, plane_state);
+		if (ret)
+			break;
+	}
+
+	if (ret) {
+		for_each_plane_in_state(state, plane, plane_state, j) {
+			if (j >= i)
+				break;
+
+			intel_cleanup_plane_fb(plane, plane_state);
+		}
+	}
+
+	mutex_unlock(&state->dev->struct_mutex);
+
+	return ret;
+}
+
+static void intel_atomic_commit_cleanup_planes(struct drm_atomic_state *state)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i;
+
+	mutex_lock(&state->dev->struct_mutex);
+
+	for_each_plane_in_state(state, plane, plane_state, i)
+		intel_cleanup_plane_fb(plane, plane_state);
+
+	mutex_unlock(&state->dev->struct_mutex);
+}
 
 static int intel_atomic_prepare_commit(struct drm_device *dev,
 				       struct drm_atomic_state *state)
@@ -14292,14 +14335,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 			flush_workqueue(dev_priv->wq);
 	}
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
+	return intel_atomic_commit_prepare_planes(state);
 }
 
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
@@ -14622,9 +14658,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	if (intel_state->modeset)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 
-	mutex_lock(&dev->struct_mutex);
-	drm_atomic_helper_cleanup_planes(dev, state);
-	mutex_unlock(&dev->struct_mutex);
+	intel_atomic_commit_cleanup_planes(state);
 
 	drm_atomic_helper_commit_cleanup_done(state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4cb8254c66dd..fff55d93ca73 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1281,10 +1281,6 @@ __intel_framebuffer_create(struct drm_device *dev,
 void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe);
 void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe);
 void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe);
-int intel_prepare_plane_fb(struct drm_plane *plane,
-			   struct drm_plane_state *new_state);
-void intel_cleanup_plane_fb(struct drm_plane *plane,
-			    struct drm_plane_state *old_state);
 int intel_plane_atomic_get_property(struct drm_plane *plane,
 				    const struct drm_plane_state *state,
 				    struct drm_property *property,
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
  2016-11-15  8:58 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson
@ 2016-11-15  8:58 ` Chris Wilson
  2016-11-15  9:52   ` Daniel Vetter
  2016-11-15 10:24   ` Daniel Vetter
  2016-11-15  8:58 ` [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb() Chris Wilson
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  8:58 UTC (permalink / raw)
  To: intel-gfx

With atomic plane states we are able to track an allocation right from
preparation, during use and through to the final free after being
swapped out for a new plane. We can couple the VMA we pin for the
framebuffer (and its rotation) to this lifetime and avoid all the clumsy
lookups in between.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h           | 16 ++---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
 drivers/gpu/drm/i915/intel_display.c      | 97 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h          |  9 ++-
 drivers/gpu/drm/i915/intel_fbc.c          | 52 +++++++----------
 drivers/gpu/drm/i915/intel_fbdev.c        |  4 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  8 +--
 7 files changed, 78 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e7148a3ee8b..2bc285e2ff82 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -989,6 +989,8 @@ struct intel_fbc {
 	struct work_struct underrun_work;
 
 	struct intel_fbc_state_cache {
+		struct i915_vma *vma;
+
 		struct {
 			unsigned int mode_flags;
 			uint32_t hsw_bdw_pixel_rate;
@@ -1002,15 +1004,14 @@ struct intel_fbc {
 		} plane;
 
 		struct {
-			u64 ilk_ggtt_offset;
 			uint32_t pixel_format;
 			unsigned int stride;
-			int fence_reg;
-			unsigned int tiling_mode;
 		} fb;
 	} state_cache;
 
 	struct intel_fbc_reg_params {
+		struct i915_vma *vma;
+
 		struct {
 			enum pipe pipe;
 			enum plane plane;
@@ -1018,10 +1019,8 @@ struct intel_fbc {
 		} crtc;
 
 		struct {
-			u64 ggtt_offset;
 			uint32_t pixel_format;
 			unsigned int stride;
-			int fence_reg;
 		} fb;
 
 		int cfb_size;
@@ -3150,13 +3149,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
 	return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
 }
 
-static inline unsigned long
-i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
-			    const struct i915_ggtt_view *view)
-{
-	return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
-}
-
 /* i915_gem_fence_reg.c */
 int __must_check i915_vma_get_fence(struct i915_vma *vma);
 int __must_check i915_vma_put_fence(struct i915_vma *vma);
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 89a34350a35d..5a86e0d52409 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
 
 	__drm_atomic_helper_plane_duplicate_state(plane, state);
 
+	intel_state->vma = NULL;
+
 	return state;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dca1e0b512e5..cb52116f8577 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2241,24 +2241,19 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
 			i915_vma_pin_fence(vma);
 	}
 
+	i915_vma_get(vma);
 err:
 	intel_runtime_pm_put(dev_priv);
 	return vma;
 }
 
-void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
+void intel_unpin_fb_vma(struct i915_vma *vma)
 {
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct i915_ggtt_view view;
-	struct i915_vma *vma;
-
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
-
-	intel_fill_fb_ggtt_view(&view, fb, rotation);
-	vma = i915_gem_object_to_ggtt(obj, &view);
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
 
 	i915_vma_unpin_fence(vma);
 	i915_gem_object_unpin_from_display_plane(vma);
+	i915_vma_put(vma);
 }
 
 static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane,
@@ -2752,7 +2747,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *c;
-	struct intel_crtc *i;
 	struct drm_i915_gem_object *obj;
 	struct drm_plane *primary = intel_crtc->base.primary;
 	struct drm_plane_state *plane_state = primary->state;
@@ -2777,20 +2771,20 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	 * an fb with another CRTC instead
 	 */
 	for_each_crtc(dev, c) {
-		i = to_intel_crtc(c);
+		struct intel_plane_state *state;
 
 		if (c == &intel_crtc->base)
 			continue;
 
-		if (!i->active)
+		if (!to_intel_crtc(c)->active)
 			continue;
 
-		fb = c->primary->fb;
-		if (!fb)
+		state = to_intel_plane_state(c->primary->state);
+		if (!state->vma)
 			continue;
 
-		obj = intel_fb_obj(fb);
-		if (i915_gem_object_ggtt_offset(obj, NULL) == plane_config->base) {
+		if (i915_ggtt_offset(state->vma) == plane_config->base) {
+			fb = c->primary->fb;
 			drm_framebuffer_reference(fb);
 			goto valid_fb;
 		}
@@ -2830,6 +2824,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 
 	drm_framebuffer_reference(fb);
 	primary->fb = primary->state->fb = fb;
+
+	mutex_lock(&dev->struct_mutex);
+	intel_state->vma =
+		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
+	mutex_unlock(&dev->struct_mutex);
+
 	primary->crtc = primary->state->crtc = &intel_crtc->base;
 	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
 	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
@@ -3104,13 +3104,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
 	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
 	if (INTEL_INFO(dev)->gen >= 4) {
 		I915_WRITE(DSPSURF(plane),
-			   intel_fb_gtt_offset(fb, rotation) +
+			   intel_plane_ggtt_offset(plane_state) +
 			   intel_crtc->dspaddr_offset);
 		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
 		I915_WRITE(DSPLINOFF(plane), linear_offset);
 	} else {
 		I915_WRITE(DSPADDR(plane),
-			   intel_fb_gtt_offset(fb, rotation) +
+			   intel_plane_ggtt_offset(plane_state) +
 			   intel_crtc->dspaddr_offset);
 	}
 	POSTING_READ(reg);
@@ -3207,7 +3207,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
 
 	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
 	I915_WRITE(DSPSURF(plane),
-		   intel_fb_gtt_offset(fb, rotation) +
+		   intel_plane_ggtt_offset(plane_state) +
 		   intel_crtc->dspaddr_offset);
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		I915_WRITE(DSPOFFSET(plane), (y << 16) | x);
@@ -3230,23 +3230,6 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
 	}
 }
 
-u32 intel_fb_gtt_offset(struct drm_framebuffer *fb,
-			unsigned int rotation)
-{
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct i915_ggtt_view view;
-	struct i915_vma *vma;
-
-	intel_fill_fb_ggtt_view(&view, fb, rotation);
-
-	vma = i915_gem_object_to_ggtt(obj, &view);
-	if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n",
-		 view.type))
-		return -1;
-
-	return i915_ggtt_offset(vma);
-}
-
 static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
@@ -3447,7 +3430,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	}
 
 	I915_WRITE(PLANE_SURF(pipe, 0),
-		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
+		   intel_plane_ggtt_offset(plane_state) + surf_addr);
 
 	POSTING_READ(PLANE_SURF(pipe, 0));
 }
@@ -11576,7 +11559,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 		flush_work(&work->mmio_work);
 
 	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(work->old_fb, primary->state->rotation);
+	intel_unpin_fb_vma(work->old_vma);
 	i915_gem_object_put(work->pending_flip_obj);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -12286,8 +12269,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		goto cleanup_pending;
 	}
 
-	work->gtt_offset = intel_fb_gtt_offset(fb, primary->state->rotation);
-	work->gtt_offset += intel_crtc->dspaddr_offset;
+	work->old_vma = to_intel_plane_state(primary->state)->vma;
+	to_intel_plane_state(primary->state)->vma = vma;
+
+	work->gtt_offset = i915_ggtt_offset(vma) + intel_crtc->dspaddr_offset;
 	work->rotation = crtc->primary->state->rotation;
 
 	/*
@@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 cleanup_request:
 	i915_add_request_no_flush(request);
 cleanup_unpin:
-	intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
+	to_intel_plane_state(primary->state)->vma = work->old_vma;
+	intel_unpin_fb_vma(vma);
 cleanup_pending:
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
@@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		struct i915_vma *vma;
 
 		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
-		if (IS_ERR(vma)) {
-			DRM_DEBUG_KMS("failed to pin object\n");
-			return PTR_ERR(vma);
-		}
+		if (IS_ERR(vma))
+			ret = PTR_ERR(vma);
+
+		to_intel_plane_state(new_state)->vma = vma;
 	}
 
 	return 0;
@@ -14256,19 +14242,12 @@ static void
 intel_cleanup_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *old_state)
 {
-	struct drm_i915_private *dev_priv = to_i915(plane->dev);
-	struct intel_plane_state *old_intel_state;
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
-	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
-
-	old_intel_state = to_intel_plane_state(old_state);
-
-	if (!obj && !old_obj)
-		return;
+	struct i915_vma *vma;
 
-	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
-		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
+	/* Should only called after a successful intel_prepare_plane_fb()! */
+	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
+	if (vma)
+		intel_unpin_fb_vma(vma);
 }
 
 static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
@@ -15215,7 +15194,7 @@ intel_update_cursor_plane(struct drm_plane *plane,
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev_priv)->cursor_needs_physical)
-		addr = i915_gem_object_ggtt_offset(obj, NULL);
+		addr = i915_ggtt_offset(state->vma);
 	else
 		addr = obj->phys_handle->busaddr;
 
@@ -17135,6 +17114,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
 			update_state_fb(c->primary);
 			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
 		}
+
+		to_intel_plane_state(c->primary->state)->vma = vma;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fff55d93ca73..d3d26d69930a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -372,6 +372,7 @@ struct intel_atomic_state {
 struct intel_plane_state {
 	struct drm_plane_state base;
 	struct drm_rect clip;
+	struct i915_vma *vma;
 
 	struct {
 		u32 offset;
@@ -1046,6 +1047,7 @@ struct intel_flip_work {
 	struct work_struct mmio_work;
 
 	struct drm_crtc *crtc;
+	struct i915_vma *old_vma;
 	struct drm_framebuffer *old_fb;
 	struct drm_i915_gem_object *pending_flip_obj;
 	struct drm_pending_vblank_event *event;
@@ -1273,7 +1275,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct drm_modeset_acquire_ctx *ctx);
 struct i915_vma *
 intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
-void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
+void intel_unpin_fb_vma(struct i915_vma *vma);
 struct drm_framebuffer *
 __intel_framebuffer_create(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
@@ -1358,7 +1360,10 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
 
-u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, unsigned int rotation);
+static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
+{
+	return i915_ggtt_offset(state->vma);
+}
 
 u32 skl_plane_ctl_format(uint32_t pixel_format);
 u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 62f215b12eb5..f3a1d6a5cabe 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -173,7 +173,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv)
 	if (IS_I945GM(dev_priv))
 		fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */
 	fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
-	fbc_ctl |= params->fb.fence_reg;
+	fbc_ctl |= params->vma->fence->id;
 	I915_WRITE(FBC_CONTROL, fbc_ctl);
 }
 
@@ -193,8 +193,8 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
 	else
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
 
-	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
-		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
+	if (params->vma->fence) {
+		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id;
 		I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
 	} else {
 		I915_WRITE(DPFC_FENCE_YOFF, 0);
@@ -251,13 +251,14 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
 		break;
 	}
 
-	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
+	if (params->vma->fence) {
 		dpfc_ctl |= DPFC_CTL_FENCE_EN;
 		if (IS_GEN5(dev_priv))
-			dpfc_ctl |= params->fb.fence_reg;
+			dpfc_ctl |= params->vma->fence->id;
 		if (IS_GEN6(dev_priv)) {
 			I915_WRITE(SNB_DPFC_CTL_SA,
-				   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+				   SNB_CPU_FENCE_ENABLE |
+				   params->vma->fence->id);
 			I915_WRITE(DPFC_CPU_FENCE_OFFSET,
 				   params->crtc.fence_y_offset);
 		}
@@ -269,7 +270,8 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
 	}
 
 	I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
-	I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID);
+	I915_WRITE(ILK_FBC_RT_BASE,
+		   i915_ggtt_offset(params->vma) | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
@@ -319,10 +321,11 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 		break;
 	}
 
-	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
+	if (params->vma->fence) {
 		dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
 		I915_WRITE(SNB_DPFC_CTL_SA,
-			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+			   SNB_CPU_FENCE_ENABLE |
+			   params->vma->fence->id);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
 	} else {
 		I915_WRITE(SNB_DPFC_CTL_SA,0);
@@ -727,14 +730,6 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 	return effective_w <= max_w && effective_h <= max_h;
 }
 
-/* XXX replace me when we have VMA tracking for intel_plane_state */
-static int get_fence_id(struct drm_framebuffer *fb)
-{
-	struct i915_vma *vma = i915_gem_object_to_ggtt(intel_fb_obj(fb), NULL);
-
-	return vma && vma->fence ? vma->fence->id : I915_FENCE_REG_NONE;
-}
-
 static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 					 struct intel_crtc_state *crtc_state,
 					 struct intel_plane_state *plane_state)
@@ -743,7 +738,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct intel_fbc_state_cache *cache = &fbc->state_cache;
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	struct drm_i915_gem_object *obj;
+
+	cache->vma = NULL;
 
 	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
@@ -758,16 +754,10 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 	if (!cache->plane.visible)
 		return;
 
-	obj = intel_fb_obj(fb);
-
-	/* FIXME: We lack the proper locking here, so only run this on the
-	 * platforms that need. */
-	if (IS_GEN(dev_priv, 5, 6))
-		cache->fb.ilk_ggtt_offset = i915_gem_object_ggtt_offset(obj, NULL);
 	cache->fb.pixel_format = fb->pixel_format;
 	cache->fb.stride = fb->pitches[0];
-	cache->fb.fence_reg = get_fence_id(fb);
-	cache->fb.tiling_mode = i915_gem_object_get_tiling(obj);
+
+	cache->vma = plane_state->vma;
 }
 
 static bool intel_fbc_can_activate(struct intel_crtc *crtc)
@@ -784,7 +774,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
-	if (!cache->plane.visible) {
+	if (!cache->vma) {
 		fbc->no_fbc_reason = "primary plane not visible";
 		return false;
 	}
@@ -807,8 +797,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 * so have no fence associated with it) due to aperture constaints
 	 * at the time of pinning.
 	 */
-	if (cache->fb.tiling_mode != I915_TILING_X ||
-	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
+	if (!cache->vma->fence) {
 		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
 		return false;
 	}
@@ -888,17 +877,16 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 	 * zero. */
 	memset(params, 0, sizeof(*params));
 
+	params->vma = cache->vma;
+
 	params->crtc.pipe = crtc->pipe;
 	params->crtc.plane = crtc->plane;
 	params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc);
 
 	params->fb.pixel_format = cache->fb.pixel_format;
 	params->fb.stride = cache->fb.stride;
-	params->fb.fence_reg = cache->fb.fence_reg;
 
 	params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache);
-
-	params->fb.ggtt_offset = cache->fb.ilk_ggtt_offset;
 }
 
 static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index fc958d5ed0dc..ca29a5314f0e 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -284,7 +284,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
-	intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0);
+	intel_unpin_fb_vma(vma);
 out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
@@ -549,7 +549,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
 
 	if (ifbdev->fb) {
 		mutex_lock(&ifbdev->helper.dev->struct_mutex);
-		intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0);
+		intel_unpin_fb_vma(ifbdev->vma);
 		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
 
 		drm_framebuffer_remove(&ifbdev->fb->base);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8b2fc67acbba..ae8ce621e3fc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -281,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
 	I915_WRITE(PLANE_SURF(pipe, plane),
-		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
+		   intel_plane_ggtt_offset(plane_state) + surf_addr);
 	POSTING_READ(PLANE_SURF(pipe, plane));
 }
 
@@ -476,7 +476,7 @@ vlv_update_plane(struct drm_plane *dplane,
 	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_WRITE(SPSURF(pipe, plane),
-		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
+		   intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
 }
 
@@ -612,7 +612,7 @@ ivb_update_plane(struct drm_plane *plane,
 		I915_WRITE(SPRSCALE(pipe), sprscale);
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_WRITE(SPRSURF(pipe),
-		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
+		   intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
 }
 
@@ -739,7 +739,7 @@ ilk_update_plane(struct drm_plane *plane,
 	I915_WRITE(DVSSCALE(pipe), dvsscale);
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_WRITE(DVSSURF(pipe),
-		   intel_fb_gtt_offset(fb, rotation) + dvssurf_offset);
+		   intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
 }
 
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb()
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
  2016-11-15  8:58 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson
  2016-11-15  8:58 ` [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state Chris Wilson
@ 2016-11-15  8:58 ` Chris Wilson
  2016-11-15 10:23   ` Daniel Vetter
  2016-11-15  8:58 ` [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed Chris Wilson
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  8:58 UTC (permalink / raw)
  To: intel-gfx

Just a quick tidy now to make the next patch neater.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb52116f8577..4d578dc6d23f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14130,6 +14130,17 @@ static int intel_atomic_check(struct drm_device *dev,
 	return calc_watermark_data(state);
 }
 
+static bool old_plane_needs_modeset(struct drm_plane *plane,
+				    struct drm_plane_state *new_state)
+{
+	struct drm_crtc_state *crtc_state;
+
+	crtc_state = drm_atomic_get_existing_crtc_state(new_state->state,
+							plane->state->crtc);
+
+	return needs_modeset(crtc_state);
+}
+
 /**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
@@ -14153,16 +14164,11 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->dev);
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
 	int ret;
 
-	if (!obj && !old_obj)
-		return 0;
-
-	if (old_obj) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_existing_crtc_state(new_state->state,
-							   plane->state->crtc);
+	if (plane->state->fb && old_plane_needs_modeset(plane, new_state)) {
+		struct drm_i915_gem_object *old_obj =
+			intel_fb_obj(plane->state->fb);
 
 		/* Big Hammer, we also need to ensure that any pending
 		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
@@ -14175,14 +14181,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		 * This should only fail upon a hung GPU, in which case we
 		 * can safely continue.
 		 */
-		if (needs_modeset(crtc_state)) {
-			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
-							      old_obj->resv, NULL,
-							      false, 0,
-							      GFP_KERNEL);
-			if (ret < 0)
-				return ret;
-		}
+		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
+						      old_obj->resv, NULL,
+						      false, 0,
+						      GFP_KERNEL);
+		if (ret < 0)
+			return ret;
 	}
 
 	if (new_state->fence) { /* explicit fencing */
@@ -14221,7 +14225,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
 		if (IS_ERR(vma))
-			ret = PTR_ERR(vma);
+			return PTR_ERR(vma);
 
 		to_intel_plane_state(new_state)->vma = vma;
 	}
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
                   ` (2 preceding siblings ...)
  2016-11-15  8:58 ` [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb() Chris Wilson
@ 2016-11-15  8:58 ` Chris Wilson
  2016-11-15 10:15   ` Daniel Vetter
  2016-11-15  9:15 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Patchwork
  2016-11-15  9:28 ` [PATCH 1/5] " Daniel Vetter
  5 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  8:58 UTC (permalink / raw)
  To: intel-gfx

Since an fb may have multiple VMA (due to rotations etc), we need to
wait a vblank and unpin the old VMA not if the fb itself is changed, but
if the underlying VMA is changed.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d578dc6d23f..a92adce033ab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12476,9 +12476,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
-	if (fb != old_plane_state->base.fb)
-		pipe_config->fb_changed = true;
-
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
@@ -14228,6 +14225,13 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 			return PTR_ERR(vma);
 
 		to_intel_plane_state(new_state)->vma = vma;
+		if (to_intel_plane_state(plane->state)->vma != vma) {
+			struct intel_crtc_state *crtc_state;
+
+			crtc_state = intel_atomic_get_crtc_state(new_state->state,
+								 to_intel_crtc(new_state->crtc));
+			crtc_state->fb_changed = true;
+		}
 	}
 
 	return 0;
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb()
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
                   ` (3 preceding siblings ...)
  2016-11-15  8:58 ` [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed Chris Wilson
@ 2016-11-15  9:15 ` Patchwork
  2016-11-15  9:28 ` [PATCH 1/5] " Daniel Vetter
  5 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2016-11-15  9:15 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb()
URL   : https://patchwork.freedesktop.org/series/15325/
State : warning

== Summary ==

Series 15325v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/15325/revisions/1/mbox/

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3520m)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6770hq)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:244  pass:228  dwarn:1   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:203  dwarn:1   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:223  dwarn:1   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:221  dwarn:1   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:221  dwarn:1   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:229  dwarn:1   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:227  dwarn:3   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

6cdc55e3a3822b1a0faff0124828816384bd04e1 drm-intel-nightly: 2016y-11m-15d-08h-13m-16s UTC integration manifest
c1e9b34 drm/i915: Set crtc_state->fb_changed whenever a VMA is changed
7a960c5 drm/i915: Quick spring clean of intel_prepare_plane_fb()
144db97 drm/i915: Track pinned vma in intel_plane_state
6e62480 drm/i915: Always prepare planes at the start of an atomic commit
34f16e2 drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2996/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb()
  2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
                   ` (4 preceding siblings ...)
  2016-11-15  9:15 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Patchwork
@ 2016-11-15  9:28 ` Daniel Vetter
  5 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15  9:28 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:13AM +0000, Chris Wilson wrote:
> In the next patch, a few rearrangements are made to make these static.
> First, we move them so the changes are not lost in the noise.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Assuming you really just moved (didn't spot anything else):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 255 ++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  2 files changed, 130 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cb9377de456e..8e04f31bf12e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14144,6 +14144,134 @@ static int intel_atomic_check(struct drm_device *dev,
>  	return calc_watermark_data(state);
>  }
>  
> +/**
> + * 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.
> + *
> + * Must be called with struct_mutex held.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int
> +intel_prepare_plane_fb(struct drm_plane *plane,
> +		       struct drm_plane_state *new_state)
> +{
> +	struct intel_atomic_state *intel_state =
> +		to_intel_atomic_state(new_state->state);
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
> +	int ret;
> +
> +	if (!obj && !old_obj)
> +		return 0;
> +
> +	if (old_obj) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(new_state->state,
> +							   plane->state->crtc);
> +
> +		/* Big Hammer, we also need to ensure that any pending
> +		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> +		 * current scanout is retired before unpinning the old
> +		 * framebuffer. Note that we rely on userspace rendering
> +		 * into the buffer attached to the pipe they are waiting
> +		 * on. If not, userspace generates a GPU hang with IPEHR
> +		 * point to the MI_WAIT_FOR_EVENT.
> +		 *
> +		 * This should only fail upon a hung GPU, in which case we
> +		 * can safely continue.
> +		 */
> +		if (needs_modeset(crtc_state)) {
> +			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> +							      old_obj->resv, NULL,
> +							      false, 0,
> +							      GFP_KERNEL);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	if (new_state->fence) { /* explicit fencing */
> +		ret = i915_sw_fence_await_dma_fence(&intel_state->commit_ready,
> +						    new_state->fence,
> +						    I915_FENCE_TIMEOUT,
> +						    GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (!obj)
> +		return 0;
> +
> +	if (!new_state->fence) { /* implicit fencing */
> +		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> +						      obj->resv, NULL,
> +						      false, I915_FENCE_TIMEOUT,
> +						      GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;
> +
> +		i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> +	}
> +
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
> +		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
> +		ret = i915_gem_object_attach_phys(obj, align);
> +		if (ret) {
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +			return ret;
> +		}
> +	} else {
> +		struct i915_vma *vma;
> +
> +		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> +		if (IS_ERR(vma)) {
> +			DRM_DEBUG_KMS("failed to pin object\n");
> +			return PTR_ERR(vma);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * 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.
> + *
> + * Must be called with struct_mutex held.
> + */
> +void
> +intel_cleanup_plane_fb(struct drm_plane *plane,
> +		       struct drm_plane_state *old_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	struct intel_plane_state *old_intel_state;
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
> +
> +	old_intel_state = to_intel_plane_state(old_state);
> +
> +	if (!obj && !old_obj)
> +		return;
> +
> +	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
> +	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
> +		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
> +}
> +
> +
>  static int intel_atomic_prepare_commit(struct drm_device *dev,
>  				       struct drm_atomic_state *state)
>  {
> @@ -14716,133 +14844,6 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.atomic_destroy_state = intel_crtc_destroy_state,
>  };
>  
> -/**
> - * 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.
> - *
> - * Must be called with struct_mutex held.
> - *
> - * Returns 0 on success, negative error code on failure.
> - */
> -int
> -intel_prepare_plane_fb(struct drm_plane *plane,
> -		       struct drm_plane_state *new_state)
> -{
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(new_state->state);
> -	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -	struct drm_framebuffer *fb = new_state->fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
> -	int ret;
> -
> -	if (!obj && !old_obj)
> -		return 0;
> -
> -	if (old_obj) {
> -		struct drm_crtc_state *crtc_state =
> -			drm_atomic_get_existing_crtc_state(new_state->state,
> -							   plane->state->crtc);
> -
> -		/* Big Hammer, we also need to ensure that any pending
> -		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> -		 * current scanout is retired before unpinning the old
> -		 * framebuffer. Note that we rely on userspace rendering
> -		 * into the buffer attached to the pipe they are waiting
> -		 * on. If not, userspace generates a GPU hang with IPEHR
> -		 * point to the MI_WAIT_FOR_EVENT.
> -		 *
> -		 * This should only fail upon a hung GPU, in which case we
> -		 * can safely continue.
> -		 */
> -		if (needs_modeset(crtc_state)) {
> -			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> -							      old_obj->resv, NULL,
> -							      false, 0,
> -							      GFP_KERNEL);
> -			if (ret < 0)
> -				return ret;
> -		}
> -	}
> -
> -	if (new_state->fence) { /* explicit fencing */
> -		ret = i915_sw_fence_await_dma_fence(&intel_state->commit_ready,
> -						    new_state->fence,
> -						    I915_FENCE_TIMEOUT,
> -						    GFP_KERNEL);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (!obj)
> -		return 0;
> -
> -	if (!new_state->fence) { /* implicit fencing */
> -		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> -						      obj->resv, NULL,
> -						      false, I915_FENCE_TIMEOUT,
> -						      GFP_KERNEL);
> -		if (ret < 0)
> -			return ret;
> -
> -		i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> -	}
> -
> -	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> -	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
> -		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
> -		ret = i915_gem_object_attach_phys(obj, align);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to attach phys object\n");
> -			return ret;
> -		}
> -	} else {
> -		struct i915_vma *vma;
> -
> -		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> -		if (IS_ERR(vma)) {
> -			DRM_DEBUG_KMS("failed to pin object\n");
> -			return PTR_ERR(vma);
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * 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.
> - *
> - * Must be called with struct_mutex held.
> - */
> -void
> -intel_cleanup_plane_fb(struct drm_plane *plane,
> -		       struct drm_plane_state *old_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -	struct intel_plane_state *old_intel_state;
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
> -	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
> -
> -	old_intel_state = to_intel_plane_state(old_state);
> -
> -	if (!obj && !old_obj)
> -		return;
> -
> -	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
> -	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
> -		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
> -}
> -
>  int
>  skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 75252ecaa613..4cb8254c66dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -772,6 +772,8 @@ struct intel_plane {
>  	int max_downscale;
>  	uint32_t frontbuffer_bit;
>  
> +	struct i915_sw_fence *last_commit;
> +
>  	/* Since we need to change the watermarks before/after
>  	 * enabling/disabling the planes, we need to store the parameters here
>  	 * as the other pieces of the struct may not reflect the values we want
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15  8:58 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson
@ 2016-11-15  9:37   ` Daniel Vetter
  2016-11-15  9:47     ` Tvrtko Ursulin
  2016-11-15  9:57     ` Chris Wilson
  0 siblings, 2 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15  9:37 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
> The generic atomic helper likes to skip a prepare_plane_fb() if it
> decides that the plane->fb is unchanged. This is wrong for us for a
> couple of reasons:
> 
>  - if the pipe is reconfigured (i.e. a size change) but the framebuffer
>    is untouched, we still have to flush any rendering prior to the
>    reconfiguration to prevent wait-for-scanline GPU hangs
> 
>  - if the framebuffer is rotated, it remains the same but has a
>    different view and a different address.
> 
> Finally, even if the framebuffer is unchanged the flip/modeset should be
> ordered with respect to rendering to the frontbuffer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

This is directly against

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

and I'm pretty sure that was tested on i915. Do we need to instead revert
the core change? Iirc the idea was to make cursors block less, but that
didn't really pan out fully.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 --
>  drivers/gpu/drm/i915/intel_display.c      | 60 ++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h          |  4 ---
>  3 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index ff821649486e..89a34350a35d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -201,8 +201,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
>  }
>  
>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> -	.prepare_fb = intel_prepare_plane_fb,
> -	.cleanup_fb = intel_cleanup_plane_fb,
>  	.atomic_check = intel_plane_atomic_check,
>  	.atomic_update = intel_plane_atomic_update,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e04f31bf12e..dca1e0b512e5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14158,7 +14158,7 @@ static int intel_atomic_check(struct drm_device *dev,
>   *
>   * Returns 0 on success, negative error code on failure.
>   */
> -int
> +static int
>  intel_prepare_plane_fb(struct drm_plane *plane,
>  		       struct drm_plane_state *new_state)
>  {
> @@ -14252,7 +14252,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   *
>   * Must be called with struct_mutex held.
>   */
> -void
> +static void
>  intel_cleanup_plane_fb(struct drm_plane *plane,
>  		       struct drm_plane_state *old_state)
>  {
> @@ -14271,6 +14271,49 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
>  }
>  
> +static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	int i, j, ret;
> +
> +	ret = mutex_lock_interruptible(&state->dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		ret = intel_prepare_plane_fb(plane, plane_state);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret) {
> +		for_each_plane_in_state(state, plane, plane_state, j) {
> +			if (j >= i)
> +				break;
> +
> +			intel_cleanup_plane_fb(plane, plane_state);
> +		}
> +	}
> +
> +	mutex_unlock(&state->dev->struct_mutex);
> +
> +	return ret;
> +}
> +
> +static void intel_atomic_commit_cleanup_planes(struct drm_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	int i;
> +
> +	mutex_lock(&state->dev->struct_mutex);
> +
> +	for_each_plane_in_state(state, plane, plane_state, i)
> +		intel_cleanup_plane_fb(plane, plane_state);
> +
> +	mutex_unlock(&state->dev->struct_mutex);
> +}
>  
>  static int intel_atomic_prepare_commit(struct drm_device *dev,
>  				       struct drm_atomic_state *state)
> @@ -14292,14 +14335,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  			flush_workqueue(dev_priv->wq);
>  	}
>  
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return ret;
> +	return intel_atomic_commit_prepare_planes(state);
>  }
>  
>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> @@ -14622,9 +14658,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	if (intel_state->modeset)
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>  
> -	mutex_lock(&dev->struct_mutex);
> -	drm_atomic_helper_cleanup_planes(dev, state);
> -	mutex_unlock(&dev->struct_mutex);
> +	intel_atomic_commit_cleanup_planes(state);
>  
>  	drm_atomic_helper_commit_cleanup_done(state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4cb8254c66dd..fff55d93ca73 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1281,10 +1281,6 @@ __intel_framebuffer_create(struct drm_device *dev,
>  void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe);
>  void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe);
>  void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe);
> -int intel_prepare_plane_fb(struct drm_plane *plane,
> -			   struct drm_plane_state *new_state);
> -void intel_cleanup_plane_fb(struct drm_plane *plane,
> -			    struct drm_plane_state *old_state);
>  int intel_plane_atomic_get_property(struct drm_plane *plane,
>  				    const struct drm_plane_state *state,
>  				    struct drm_property *property,
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15  9:37   ` Daniel Vetter
@ 2016-11-15  9:47     ` Tvrtko Ursulin
  2016-11-15  9:57     ` Chris Wilson
  1 sibling, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2016-11-15  9:47 UTC (permalink / raw)
  To: Daniel Vetter, Chris Wilson; +Cc: intel-gfx


On 15/11/2016 09:37, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
>> The generic atomic helper likes to skip a prepare_plane_fb() if it
>> decides that the plane->fb is unchanged. This is wrong for us for a
>> couple of reasons:
>>
>>  - if the pipe is reconfigured (i.e. a size change) but the framebuffer
>>    is untouched, we still have to flush any rendering prior to the
>>    reconfiguration to prevent wait-for-scanline GPU hangs
>>
>>  - if the framebuffer is rotated, it remains the same but has a
>>    different view and a different address.
>>
>> Finally, even if the framebuffer is unchanged the flip/modeset should be
>> ordered with respect to rendering to the frontbuffer.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> This is directly against
>
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
>
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>
> and I'm pretty sure that was tested on i915. Do we need to instead revert
> the core change? Iirc the idea was to make cursors block less, but that
> didn't really pan out fully.

As Chris wrote in the commit, and I experienced myself, it indeed blows 
up when rotation is used since running prepare is crucial to set up the 
rotated view. In other words, I've been running with the revert of the 
core change to be able to use rotation.

Just my 2c that it is indeed broken, I don't know this way or the other 
is the way to fix it.

Regards,

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state
  2016-11-15  8:58 ` [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state Chris Wilson
@ 2016-11-15  9:52   ` Daniel Vetter
  2016-11-15 10:02     ` Chris Wilson
  2016-11-15 10:24   ` Daniel Vetter
  1 sibling, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15  9:52 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote:
> With atomic plane states we are able to track an allocation right from
> preparation, during use and through to the final free after being
> swapped out for a new plane. We can couple the VMA we pin for the
> framebuffer (and its rotation) to this lifetime and avoid all the clumsy
> lookups in between.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

A few questions and comments below. Of course review assumes some form of
patch 2 lands first (but leaning towards just reverting the core one for
that).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h           | 16 ++---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
>  drivers/gpu/drm/i915/intel_display.c      | 97 +++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h          |  9 ++-
>  drivers/gpu/drm/i915/intel_fbc.c          | 52 +++++++----------
>  drivers/gpu/drm/i915/intel_fbdev.c        |  4 +-
>  drivers/gpu/drm/i915/intel_sprite.c       |  8 +--
>  7 files changed, 78 insertions(+), 110 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e7148a3ee8b..2bc285e2ff82 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -989,6 +989,8 @@ struct intel_fbc {
>  	struct work_struct underrun_work;
>  
>  	struct intel_fbc_state_cache {
> +		struct i915_vma *vma;
> +
>  		struct {
>  			unsigned int mode_flags;
>  			uint32_t hsw_bdw_pixel_rate;
> @@ -1002,15 +1004,14 @@ struct intel_fbc {
>  		} plane;
>  
>  		struct {
> -			u64 ilk_ggtt_offset;
>  			uint32_t pixel_format;
>  			unsigned int stride;
> -			int fence_reg;
> -			unsigned int tiling_mode;
>  		} fb;
>  	} state_cache;
>  
>  	struct intel_fbc_reg_params {
> +		struct i915_vma *vma;
> +
>  		struct {
>  			enum pipe pipe;
>  			enum plane plane;
> @@ -1018,10 +1019,8 @@ struct intel_fbc {
>  		} crtc;
>  
>  		struct {
> -			u64 ggtt_offset;
>  			uint32_t pixel_format;
>  			unsigned int stride;
> -			int fence_reg;
>  		} fb;
>  
>  		int cfb_size;
> @@ -3150,13 +3149,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
>  	return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
>  }
>  
> -static inline unsigned long
> -i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
> -			    const struct i915_ggtt_view *view)
> -{
> -	return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
> -}
> -
>  /* i915_gem_fence_reg.c */
>  int __must_check i915_vma_get_fence(struct i915_vma *vma);
>  int __must_check i915_vma_put_fence(struct i915_vma *vma);
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 89a34350a35d..5a86e0d52409 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
>  
>  	__drm_atomic_helper_plane_duplicate_state(plane, state);
>  
> +	intel_state->vma = NULL;
> +
>  	return state;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dca1e0b512e5..cb52116f8577 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2241,24 +2241,19 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>  			i915_vma_pin_fence(vma);
>  	}
>  
> +	i915_vma_get(vma);

I'm not entirely clear on why we no need a refcount for this? Vestige code
from when you wanted to refcount plane_state->fb like other refcounted
state pointers? If we restrict the lifetime to just in between
prepare_plane/cleanup_plane like you do I don't think this is needed ...

>  err:
>  	intel_runtime_pm_put(dev_priv);
>  	return vma;
>  }
>  
> -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> +void intel_unpin_fb_vma(struct i915_vma *vma)
>  {
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct i915_ggtt_view view;
> -	struct i915_vma *vma;
> -
> -	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
> -
> -	intel_fill_fb_ggtt_view(&view, fb, rotation);
> -	vma = i915_gem_object_to_ggtt(obj, &view);
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
>  
>  	i915_vma_unpin_fence(vma);
>  	i915_gem_object_unpin_from_display_plane(vma);
> +	i915_vma_put(vma);
>  }
>  
>  static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane,
> @@ -2752,7 +2747,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *c;
> -	struct intel_crtc *i;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_plane *primary = intel_crtc->base.primary;
>  	struct drm_plane_state *plane_state = primary->state;
> @@ -2777,20 +2771,20 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	 * an fb with another CRTC instead
>  	 */
>  	for_each_crtc(dev, c) {
> -		i = to_intel_crtc(c);
> +		struct intel_plane_state *state;
>  
>  		if (c == &intel_crtc->base)
>  			continue;
>  
> -		if (!i->active)
> +		if (!to_intel_crtc(c)->active)
>  			continue;
>  
> -		fb = c->primary->fb;
> -		if (!fb)
> +		state = to_intel_plane_state(c->primary->state);
> +		if (!state->vma)
>  			continue;
>  
> -		obj = intel_fb_obj(fb);
> -		if (i915_gem_object_ggtt_offset(obj, NULL) == plane_config->base) {
> +		if (i915_ggtt_offset(state->vma) == plane_config->base) {
> +			fb = c->primary->fb;
>  			drm_framebuffer_reference(fb);
>  			goto valid_fb;
>  		}
> @@ -2830,6 +2824,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  
>  	drm_framebuffer_reference(fb);
>  	primary->fb = primary->state->fb = fb;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	intel_state->vma =
> +		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	primary->crtc = primary->state->crtc = &intel_crtc->base;
>  	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
>  	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
> @@ -3104,13 +3104,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
>  	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		I915_WRITE(DSPSURF(plane),
> -			   intel_fb_gtt_offset(fb, rotation) +
> +			   intel_plane_ggtt_offset(plane_state) +
>  			   intel_crtc->dspaddr_offset);
>  		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
>  		I915_WRITE(DSPLINOFF(plane), linear_offset);
>  	} else {
>  		I915_WRITE(DSPADDR(plane),
> -			   intel_fb_gtt_offset(fb, rotation) +
> +			   intel_plane_ggtt_offset(plane_state) +
>  			   intel_crtc->dspaddr_offset);
>  	}
>  	POSTING_READ(reg);
> @@ -3207,7 +3207,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
>  
>  	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
>  	I915_WRITE(DSPSURF(plane),
> -		   intel_fb_gtt_offset(fb, rotation) +
> +		   intel_plane_ggtt_offset(plane_state) +
>  		   intel_crtc->dspaddr_offset);
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		I915_WRITE(DSPOFFSET(plane), (y << 16) | x);
> @@ -3230,23 +3230,6 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb,
> -			unsigned int rotation)
> -{
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct i915_ggtt_view view;
> -	struct i915_vma *vma;
> -
> -	intel_fill_fb_ggtt_view(&view, fb, rotation);
> -
> -	vma = i915_gem_object_to_ggtt(obj, &view);
> -	if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n",
> -		 view.type))
> -		return -1;
> -
> -	return i915_ggtt_offset(vma);
> -}
> -
>  static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> @@ -3447,7 +3430,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	}
>  
>  	I915_WRITE(PLANE_SURF(pipe, 0),
> -		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
> +		   intel_plane_ggtt_offset(plane_state) + surf_addr);
>  
>  	POSTING_READ(PLANE_SURF(pipe, 0));
>  }
> @@ -11576,7 +11559,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  		flush_work(&work->mmio_work);
>  
>  	mutex_lock(&dev->struct_mutex);
> -	intel_unpin_fb_obj(work->old_fb, primary->state->rotation);
> +	intel_unpin_fb_vma(work->old_vma);
>  	i915_gem_object_put(work->pending_flip_obj);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -12286,8 +12269,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		goto cleanup_pending;
>  	}
>  
> -	work->gtt_offset = intel_fb_gtt_offset(fb, primary->state->rotation);
> -	work->gtt_offset += intel_crtc->dspaddr_offset;
> +	work->old_vma = to_intel_plane_state(primary->state)->vma;
> +	to_intel_plane_state(primary->state)->vma = vma;
> +
> +	work->gtt_offset = i915_ggtt_offset(vma) + intel_crtc->dspaddr_offset;
>  	work->rotation = crtc->primary->state->rotation;
>  
>  	/*
> @@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  cleanup_request:
>  	i915_add_request_no_flush(request);
>  cleanup_unpin:
> -	intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
> +	to_intel_plane_state(primary->state)->vma = work->old_vma;
> +	intel_unpin_fb_vma(vma);
>  cleanup_pending:
>  	atomic_dec(&intel_crtc->unpin_work_count);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		struct i915_vma *vma;
>  
>  		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> -		if (IS_ERR(vma)) {
> -			DRM_DEBUG_KMS("failed to pin object\n");
> -			return PTR_ERR(vma);
> -		}
> +		if (IS_ERR(vma))
> +			ret = PTR_ERR(vma);
> +
> +		to_intel_plane_state(new_state)->vma = vma;
>  	}
>  
>  	return 0;
> @@ -14256,19 +14242,12 @@ static void
>  intel_cleanup_plane_fb(struct drm_plane *plane,
>  		       struct drm_plane_state *old_state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -	struct intel_plane_state *old_intel_state;
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
> -	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
> -
> -	old_intel_state = to_intel_plane_state(old_state);
> -
> -	if (!obj && !old_obj)
> -		return;
> +	struct i915_vma *vma;
>  
> -	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
> -	    !INTEL_INFO(dev_priv)->cursor_needs_physical))
> -		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
> +	/* Should only called after a successful intel_prepare_plane_fb()! */
> +	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
> +	if (vma)

WARN_ON(!vma); maybe even instead of the comment?

> +		intel_unpin_fb_vma(vma);
>  }
>  
>  static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
> @@ -15215,7 +15194,7 @@ intel_update_cursor_plane(struct drm_plane *plane,
>  	if (!obj)
>  		addr = 0;
>  	else if (!INTEL_INFO(dev_priv)->cursor_needs_physical)
> -		addr = i915_gem_object_ggtt_offset(obj, NULL);
> +		addr = i915_ggtt_offset(state->vma);
>  	else
>  		addr = obj->phys_handle->busaddr;
>  
> @@ -17135,6 +17114,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  			update_state_fb(c->primary);
>  			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
>  		}
> +
> +		to_intel_plane_state(c->primary->state)->vma = vma;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fff55d93ca73..d3d26d69930a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -372,6 +372,7 @@ struct intel_atomic_state {
>  struct intel_plane_state {
>  	struct drm_plane_state base;
>  	struct drm_rect clip;

I think a comment here about the special refcounting rules for this would
be good, e.g.

"Note that despite that vmas are refcounted using i915_vma_get() and
i915_vma_put() we don't refcount them as part of the state like other
objects. Instead their lifetime is only between prepare_planes and
cleanup_planes, and hence vma must be set to NULL when duplicating the
plane state."

> +	struct i915_vma *vma;
>  
>  	struct {
>  		u32 offset;
> @@ -1046,6 +1047,7 @@ struct intel_flip_work {
>  	struct work_struct mmio_work;
>  
>  	struct drm_crtc *crtc;
> +	struct i915_vma *old_vma;
>  	struct drm_framebuffer *old_fb;
>  	struct drm_i915_gem_object *pending_flip_obj;
>  	struct drm_pending_vblank_event *event;
> @@ -1273,7 +1275,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct drm_modeset_acquire_ctx *ctx);
>  struct i915_vma *
>  intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> +void intel_unpin_fb_vma(struct i915_vma *vma);
>  struct drm_framebuffer *
>  __intel_framebuffer_create(struct drm_device *dev,
>  			   struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -1358,7 +1360,10 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
> -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, unsigned int rotation);
> +static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> +{
> +	return i915_ggtt_offset(state->vma);
> +}
>  
>  u32 skl_plane_ctl_format(uint32_t pixel_format);
>  u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 62f215b12eb5..f3a1d6a5cabe 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -173,7 +173,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv)
>  	if (IS_I945GM(dev_priv))
>  		fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */
>  	fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
> -	fbc_ctl |= params->fb.fence_reg;
> +	fbc_ctl |= params->vma->fence->id;
>  	I915_WRITE(FBC_CONTROL, fbc_ctl);
>  }
>  
> @@ -193,8 +193,8 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
>  	else
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
>  
> -	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> -		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
> +	if (params->vma->fence) {
> +		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id;
>  		I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
>  	} else {
>  		I915_WRITE(DPFC_FENCE_YOFF, 0);
> @@ -251,13 +251,14 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> +	if (params->vma->fence) {
>  		dpfc_ctl |= DPFC_CTL_FENCE_EN;
>  		if (IS_GEN5(dev_priv))
> -			dpfc_ctl |= params->fb.fence_reg;
> +			dpfc_ctl |= params->vma->fence->id;
>  		if (IS_GEN6(dev_priv)) {
>  			I915_WRITE(SNB_DPFC_CTL_SA,
> -				   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
> +				   SNB_CPU_FENCE_ENABLE |
> +				   params->vma->fence->id);
>  			I915_WRITE(DPFC_CPU_FENCE_OFFSET,
>  				   params->crtc.fence_y_offset);
>  		}
> @@ -269,7 +270,8 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
>  	}
>  
>  	I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
> -	I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID);
> +	I915_WRITE(ILK_FBC_RT_BASE,
> +		   i915_ggtt_offset(params->vma) | ILK_FBC_RT_VALID);
>  	/* enable it... */
>  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
>  
> @@ -319,10 +321,11 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> +	if (params->vma->fence) {
>  		dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
>  		I915_WRITE(SNB_DPFC_CTL_SA,
> -			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
> +			   SNB_CPU_FENCE_ENABLE |
> +			   params->vma->fence->id);
>  		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
>  	} else {
>  		I915_WRITE(SNB_DPFC_CTL_SA,0);
> @@ -727,14 +730,6 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  	return effective_w <= max_w && effective_h <= max_h;
>  }
>  
> -/* XXX replace me when we have VMA tracking for intel_plane_state */
> -static int get_fence_id(struct drm_framebuffer *fb)
> -{
> -	struct i915_vma *vma = i915_gem_object_to_ggtt(intel_fb_obj(fb), NULL);
> -
> -	return vma && vma->fence ? vma->fence->id : I915_FENCE_REG_NONE;
> -}
> -
>  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  					 struct intel_crtc_state *crtc_state,
>  					 struct intel_plane_state *plane_state)
> @@ -743,7 +738,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	struct intel_fbc *fbc = &dev_priv->fbc;
>  	struct intel_fbc_state_cache *cache = &fbc->state_cache;
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	struct drm_i915_gem_object *obj;
> +
> +	cache->vma = NULL;
>  
>  	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> @@ -758,16 +754,10 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	if (!cache->plane.visible)
>  		return;
>  
> -	obj = intel_fb_obj(fb);
> -
> -	/* FIXME: We lack the proper locking here, so only run this on the
> -	 * platforms that need. */
> -	if (IS_GEN(dev_priv, 5, 6))
> -		cache->fb.ilk_ggtt_offset = i915_gem_object_ggtt_offset(obj, NULL);
>  	cache->fb.pixel_format = fb->pixel_format;
>  	cache->fb.stride = fb->pitches[0];
> -	cache->fb.fence_reg = get_fence_id(fb);
> -	cache->fb.tiling_mode = i915_gem_object_get_tiling(obj);
> +
> +	cache->vma = plane_state->vma;
>  }
>  
>  static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> @@ -784,7 +774,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	if (!cache->plane.visible) {
> +	if (!cache->vma) {
>  		fbc->no_fbc_reason = "primary plane not visible";
>  		return false;
>  	}
> @@ -807,8 +797,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  	 * so have no fence associated with it) due to aperture constaints
>  	 * at the time of pinning.
>  	 */
> -	if (cache->fb.tiling_mode != I915_TILING_X ||
> -	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> +	if (!cache->vma->fence) {
>  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
>  		return false;
>  	}
> @@ -888,17 +877,16 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
>  	 * zero. */
>  	memset(params, 0, sizeof(*params));
>  
> +	params->vma = cache->vma;
> +
>  	params->crtc.pipe = crtc->pipe;
>  	params->crtc.plane = crtc->plane;
>  	params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc);
>  
>  	params->fb.pixel_format = cache->fb.pixel_format;
>  	params->fb.stride = cache->fb.stride;
> -	params->fb.fence_reg = cache->fb.fence_reg;
>  
>  	params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache);
> -
> -	params->fb.ggtt_offset = cache->fb.ilk_ggtt_offset;
>  }
>  
>  static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fc958d5ed0dc..ca29a5314f0e 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -284,7 +284,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> -	intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0);
> +	intel_unpin_fb_vma(vma);
>  out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
> @@ -549,7 +549,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>  
>  	if (ifbdev->fb) {
>  		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> -		intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0);
> +		intel_unpin_fb_vma(ifbdev->vma);
>  		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
>  
>  		drm_framebuffer_remove(&ifbdev->fb->base);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b2fc67acbba..ae8ce621e3fc 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -281,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>  	I915_WRITE(PLANE_SURF(pipe, plane),
> -		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
> +		   intel_plane_ggtt_offset(plane_state) + surf_addr);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
>  }
>  
> @@ -476,7 +476,7 @@ vlv_update_plane(struct drm_plane *dplane,
>  	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>  	I915_WRITE(SPCNTR(pipe, plane), sprctl);
>  	I915_WRITE(SPSURF(pipe, plane),
> -		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
> +		   intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
>  	POSTING_READ(SPSURF(pipe, plane));
>  }
>  
> @@ -612,7 +612,7 @@ ivb_update_plane(struct drm_plane *plane,
>  		I915_WRITE(SPRSCALE(pipe), sprscale);
>  	I915_WRITE(SPRCTL(pipe), sprctl);
>  	I915_WRITE(SPRSURF(pipe),
> -		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
> +		   intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
>  }
>  
> @@ -739,7 +739,7 @@ ilk_update_plane(struct drm_plane *plane,
>  	I915_WRITE(DVSSCALE(pipe), dvsscale);
>  	I915_WRITE(DVSCNTR(pipe), dvscntr);
>  	I915_WRITE(DVSSURF(pipe),
> -		   intel_fb_gtt_offset(fb, rotation) + dvssurf_offset);
> +		   intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
>  	POSTING_READ(DVSSURF(pipe));
>  }
>  
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15  9:37   ` Daniel Vetter
  2016-11-15  9:47     ` Tvrtko Ursulin
@ 2016-11-15  9:57     ` Chris Wilson
  2016-11-15 10:10       ` Daniel Vetter
  1 sibling, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2016-11-15  9:57 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 10:37:09AM +0100, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
> > The generic atomic helper likes to skip a prepare_plane_fb() if it
> > decides that the plane->fb is unchanged. This is wrong for us for a
> > couple of reasons:
> > 
> >  - if the pipe is reconfigured (i.e. a size change) but the framebuffer
> >    is untouched, we still have to flush any rendering prior to the
> >    reconfiguration to prevent wait-for-scanline GPU hangs
> > 
> >  - if the framebuffer is rotated, it remains the same but has a
> >    different view and a different address.
> > 
> > Finally, even if the framebuffer is unchanged the flip/modeset should be
> > ordered with respect to rendering to the frontbuffer.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This is directly against
> 
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
> 
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> 
> and I'm pretty sure that was tested on i915.

That patch is bogus. But if we want plane granularity, the helpers are
not sufficient either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state
  2016-11-15  9:52   ` Daniel Vetter
@ 2016-11-15 10:02     ` Chris Wilson
  2016-11-15 10:13       ` Daniel Vetter
  0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2016-11-15 10:02 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 10:52:00AM +0100, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote:
> > With atomic plane states we are able to track an allocation right from
> > preparation, during use and through to the final free after being
> > swapped out for a new plane. We can couple the VMA we pin for the
> > framebuffer (and its rotation) to this lifetime and avoid all the clumsy
> > lookups in between.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A few questions and comments below. Of course review assumes some form of
> patch 2 lands first (but leaning towards just reverting the core one for
> that).
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h           | 16 ++---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
> >  drivers/gpu/drm/i915/intel_display.c      | 97 +++++++++++++------------------
> >  drivers/gpu/drm/i915/intel_drv.h          |  9 ++-
> >  drivers/gpu/drm/i915/intel_fbc.c          | 52 +++++++----------
> >  drivers/gpu/drm/i915/intel_fbdev.c        |  4 +-
> >  drivers/gpu/drm/i915/intel_sprite.c       |  8 +--
> >  7 files changed, 78 insertions(+), 110 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4e7148a3ee8b..2bc285e2ff82 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -989,6 +989,8 @@ struct intel_fbc {
> >  	struct work_struct underrun_work;
> >  
> >  	struct intel_fbc_state_cache {
> > +		struct i915_vma *vma;
> > +
> >  		struct {
> >  			unsigned int mode_flags;
> >  			uint32_t hsw_bdw_pixel_rate;
> > @@ -1002,15 +1004,14 @@ struct intel_fbc {
> >  		} plane;
> >  
> >  		struct {
> > -			u64 ilk_ggtt_offset;
> >  			uint32_t pixel_format;
> >  			unsigned int stride;
> > -			int fence_reg;
> > -			unsigned int tiling_mode;
> >  		} fb;
> >  	} state_cache;
> >  
> >  	struct intel_fbc_reg_params {
> > +		struct i915_vma *vma;
> > +
> >  		struct {
> >  			enum pipe pipe;
> >  			enum plane plane;
> > @@ -1018,10 +1019,8 @@ struct intel_fbc {
> >  		} crtc;
> >  
> >  		struct {
> > -			u64 ggtt_offset;
> >  			uint32_t pixel_format;
> >  			unsigned int stride;
> > -			int fence_reg;
> >  		} fb;
> >  
> >  		int cfb_size;
> > @@ -3150,13 +3149,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
> >  	return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
> >  }
> >  
> > -static inline unsigned long
> > -i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
> > -			    const struct i915_ggtt_view *view)
> > -{
> > -	return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
> > -}
> > -
> >  /* i915_gem_fence_reg.c */
> >  int __must_check i915_vma_get_fence(struct i915_vma *vma);
> >  int __must_check i915_vma_put_fence(struct i915_vma *vma);
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 89a34350a35d..5a86e0d52409 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
> >  
> >  	__drm_atomic_helper_plane_duplicate_state(plane, state);
> >  
> > +	intel_state->vma = NULL;
> > +
> >  	return state;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index dca1e0b512e5..cb52116f8577 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2241,24 +2241,19 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >  			i915_vma_pin_fence(vma);
> >  	}
> >  
> > +	i915_vma_get(vma);
> 
> I'm not entirely clear on why we no need a refcount for this? Vestige code
> from when you wanted to refcount plane_state->fb like other refcounted
> state pointers? If we restrict the lifetime to just in between
> prepare_plane/cleanup_plane like you do I don't think this is needed ...

The lifetime is quite broad since we aquire it on takeover and it is not
finally released until plane state destruction. It's not as simple as
saying they only exist between two atomic commits :| Since the vma is
being tracked outside of the parent fb (and it is independent since it
is a view of that fb), it is safer to hold a reference.

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fff55d93ca73..d3d26d69930a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -372,6 +372,7 @@ struct intel_atomic_state {
> >  struct intel_plane_state {
> >  	struct drm_plane_state base;
> >  	struct drm_rect clip;
> 
> I think a comment here about the special refcounting rules for this would
> be good, e.g.
> 
> "Note that despite that vmas are refcounted using i915_vma_get() and
> i915_vma_put() we don't refcount them as part of the state like other
> objects. Instead their lifetime is only between prepare_planes and
> cleanup_planes, and hence vma must be set to NULL when duplicating the
> plane state."

That would be false :-p
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15  9:57     ` Chris Wilson
@ 2016-11-15 10:10       ` Daniel Vetter
  2016-11-15 10:18         ` Chris Wilson
  0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15 10:10 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx

On Tue, Nov 15, 2016 at 09:57:37AM +0000, Chris Wilson wrote:
> On Tue, Nov 15, 2016 at 10:37:09AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
> > > The generic atomic helper likes to skip a prepare_plane_fb() if it
> > > decides that the plane->fb is unchanged. This is wrong for us for a
> > > couple of reasons:
> > > 
> > >  - if the pipe is reconfigured (i.e. a size change) but the framebuffer
> > >    is untouched, we still have to flush any rendering prior to the
> > >    reconfiguration to prevent wait-for-scanline GPU hangs
> > > 
> > >  - if the framebuffer is rotated, it remains the same but has a
> > >    different view and a different address.
> > > 
> > > Finally, even if the framebuffer is unchanged the flip/modeset should be
> > > ordered with respect to rendering to the frontbuffer.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > This is directly against
> > 
> > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> > Author: Keith Packard <keithp@keithp.com>
> > Date:   Sat Jun 4 01:16:22 2016 -0700
> > 
> >     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> > 
> > and I'm pretty sure that was tested on i915.
> 
> That patch is bogus. But if we want plane granularity, the helpers are
> not sufficient either.

Hm, what do you mean with plane granularity? If you just mean async
commits, then yes the current helper machinery won't work. But I don't
think extending them is a good idea either, per-plane async updates
probably need something entirely different on the back-end. We could still
use the atomic ioctl just as the transport, but that's about it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state
  2016-11-15 10:02     ` Chris Wilson
@ 2016-11-15 10:13       ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15 10:13 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx

On Tue, Nov 15, 2016 at 10:02:23AM +0000, Chris Wilson wrote:
> On Tue, Nov 15, 2016 at 10:52:00AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote:
> > > With atomic plane states we are able to track an allocation right from
> > > preparation, during use and through to the final free after being
> > > swapped out for a new plane. We can couple the VMA we pin for the
> > > framebuffer (and its rotation) to this lifetime and avoid all the clumsy
> > > lookups in between.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > A few questions and comments below. Of course review assumes some form of
> > patch 2 lands first (but leaning towards just reverting the core one for
> > that).
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h           | 16 ++---
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c |  2 +
> > >  drivers/gpu/drm/i915/intel_display.c      | 97 +++++++++++++------------------
> > >  drivers/gpu/drm/i915/intel_drv.h          |  9 ++-
> > >  drivers/gpu/drm/i915/intel_fbc.c          | 52 +++++++----------
> > >  drivers/gpu/drm/i915/intel_fbdev.c        |  4 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c       |  8 +--
> > >  7 files changed, 78 insertions(+), 110 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4e7148a3ee8b..2bc285e2ff82 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -989,6 +989,8 @@ struct intel_fbc {
> > >  	struct work_struct underrun_work;
> > >  
> > >  	struct intel_fbc_state_cache {
> > > +		struct i915_vma *vma;
> > > +
> > >  		struct {
> > >  			unsigned int mode_flags;
> > >  			uint32_t hsw_bdw_pixel_rate;
> > > @@ -1002,15 +1004,14 @@ struct intel_fbc {
> > >  		} plane;
> > >  
> > >  		struct {
> > > -			u64 ilk_ggtt_offset;
> > >  			uint32_t pixel_format;
> > >  			unsigned int stride;
> > > -			int fence_reg;
> > > -			unsigned int tiling_mode;
> > >  		} fb;
> > >  	} state_cache;
> > >  
> > >  	struct intel_fbc_reg_params {
> > > +		struct i915_vma *vma;
> > > +
> > >  		struct {
> > >  			enum pipe pipe;
> > >  			enum plane plane;
> > > @@ -1018,10 +1019,8 @@ struct intel_fbc {
> > >  		} crtc;
> > >  
> > >  		struct {
> > > -			u64 ggtt_offset;
> > >  			uint32_t pixel_format;
> > >  			unsigned int stride;
> > > -			int fence_reg;
> > >  		} fb;
> > >  
> > >  		int cfb_size;
> > > @@ -3150,13 +3149,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
> > >  	return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
> > >  }
> > >  
> > > -static inline unsigned long
> > > -i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
> > > -			    const struct i915_ggtt_view *view)
> > > -{
> > > -	return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
> > > -}
> > > -
> > >  /* i915_gem_fence_reg.c */
> > >  int __must_check i915_vma_get_fence(struct i915_vma *vma);
> > >  int __must_check i915_vma_put_fence(struct i915_vma *vma);
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > index 89a34350a35d..5a86e0d52409 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > @@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
> > >  
> > >  	__drm_atomic_helper_plane_duplicate_state(plane, state);
> > >  
> > > +	intel_state->vma = NULL;
> > > +
> > >  	return state;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index dca1e0b512e5..cb52116f8577 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2241,24 +2241,19 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > >  			i915_vma_pin_fence(vma);
> > >  	}
> > >  
> > > +	i915_vma_get(vma);
> > 
> > I'm not entirely clear on why we no need a refcount for this? Vestige code
> > from when you wanted to refcount plane_state->fb like other refcounted
> > state pointers? If we restrict the lifetime to just in between
> > prepare_plane/cleanup_plane like you do I don't think this is needed ...
> 
> The lifetime is quite broad since we aquire it on takeover and it is not
> finally released until plane state destruction. It's not as simple as
> saying they only exist between two atomic commits :| Since the vma is
> being tracked outside of the parent fb (and it is independent since it
> is a view of that fb), it is safer to hold a reference.

Takeover = prepare_planes done by someone else. And assuming our current
refcounting is correct we shouldn't need this additional refcount either.

> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index fff55d93ca73..d3d26d69930a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -372,6 +372,7 @@ struct intel_atomic_state {
> > >  struct intel_plane_state {
> > >  	struct drm_plane_state base;
> > >  	struct drm_rect clip;
> > 
> > I think a comment here about the special refcounting rules for this would
> > be good, e.g.
> > 
> > "Note that despite that vmas are refcounted using i915_vma_get() and
> > i915_vma_put() we don't refcount them as part of the state like other
> > objects. Instead their lifetime is only between prepare_planes and
> > cleanup_planes, and hence vma must be set to NULL when duplicating the
> > plane state."
> 
> That would be false :-p

What's the false part? Ignoring the takeover corner case, where we have
lots of other cases where we need to adjust refcounts and pointers and
make sure it all looks like boot-up never happened, and it matches state
that a normal atomic commit could have produced.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed
  2016-11-15  8:58 ` [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed Chris Wilson
@ 2016-11-15 10:15   ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15 10:15 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:17AM +0000, Chris Wilson wrote:
> Since an fb may have multiple VMA (due to rotations etc), we need to
> wait a vblank and unpin the old VMA not if the fb itself is changed, but
> if the underlying VMA is changed.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Imo we should move this into core state, iirc we're not the only ones
needing it. This = fb_changed. Probably also needs to be on the
plane_state.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d578dc6d23f..a92adce033ab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12476,9 +12476,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	if (!was_visible && !visible)
>  		return 0;
>  
> -	if (fb != old_plane_state->base.fb)
> -		pipe_config->fb_changed = true;
> -
>  	turn_off = was_visible && (!visible || mode_changed);
>  	turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -14228,6 +14225,13 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  			return PTR_ERR(vma);
>  
>  		to_intel_plane_state(new_state)->vma = vma;
> +		if (to_intel_plane_state(plane->state)->vma != vma) {
> +			struct intel_crtc_state *crtc_state;
> +
> +			crtc_state = intel_atomic_get_crtc_state(new_state->state,
> +								 to_intel_crtc(new_state->crtc));
> +			crtc_state->fb_changed = true;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-11-15 10:10       ` Daniel Vetter
@ 2016-11-15 10:18         ` Chris Wilson
  0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2016-11-15 10:18 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 11:10:26AM +0100, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 09:57:37AM +0000, Chris Wilson wrote:
> > On Tue, Nov 15, 2016 at 10:37:09AM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
> > > > The generic atomic helper likes to skip a prepare_plane_fb() if it
> > > > decides that the plane->fb is unchanged. This is wrong for us for a
> > > > couple of reasons:
> > > > 
> > > >  - if the pipe is reconfigured (i.e. a size change) but the framebuffer
> > > >    is untouched, we still have to flush any rendering prior to the
> > > >    reconfiguration to prevent wait-for-scanline GPU hangs
> > > > 
> > > >  - if the framebuffer is rotated, it remains the same but has a
> > > >    different view and a different address.
> > > > 
> > > > Finally, even if the framebuffer is unchanged the flip/modeset should be
> > > > ordered with respect to rendering to the frontbuffer.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > This is directly against
> > > 
> > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> > > Author: Keith Packard <keithp@keithp.com>
> > > Date:   Sat Jun 4 01:16:22 2016 -0700
> > > 
> > >     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> > > 
> > > and I'm pretty sure that was tested on i915.
> > 
> > That patch is bogus. But if we want plane granularity, the helpers are
> > not sufficient either.
> 
> Hm, what do you mean with plane granularity? If you just mean async
> commits, then yes the current helper machinery won't work. But I don't
> think extending them is a good idea either, per-plane async updates
> probably need something entirely different on the back-end. We could still
> use the atomic ioctl just as the transport, but that's about it.

I mean an atomic commit that only modifies the cursor plane (no wm, no
other crtc or global state updates) does not impede an atomic commit that
only modifies the sprite/primary plane (again no wm, no other crtc or
global state updates).

crtc granularity (i.e. a single timeline per crtc) simply doesn't work
when the existing ABI is plane granularity.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb()
  2016-11-15  8:58 ` [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb() Chris Wilson
@ 2016-11-15 10:23   ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15 10:23 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:16AM +0000, Chris Wilson wrote:
> Just a quick tidy now to make the next patch neater.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cb52116f8577..4d578dc6d23f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14130,6 +14130,17 @@ static int intel_atomic_check(struct drm_device *dev,
>  	return calc_watermark_data(state);
>  }
>  
> +static bool old_plane_needs_modeset(struct drm_plane *plane,
> +				    struct drm_plane_state *new_state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	crtc_state = drm_atomic_get_existing_crtc_state(new_state->state,
> +							plane->state->crtc);
> +
> +	return needs_modeset(crtc_state);
> +}
> +
>  /**
>   * intel_prepare_plane_fb - Prepare fb for usage on plane
>   * @plane: drm plane to prepare for
> @@ -14153,16 +14164,11 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(plane->dev);
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>  	int ret;
>  
> -	if (!obj && !old_obj)
> -		return 0;
> -
> -	if (old_obj) {
> -		struct drm_crtc_state *crtc_state =
> -			drm_atomic_get_existing_crtc_state(new_state->state,
> -							   plane->state->crtc);
> +	if (plane->state->fb && old_plane_needs_modeset(plane, new_state)) {
> +		struct drm_i915_gem_object *old_obj =
> +			intel_fb_obj(plane->state->fb);
>  
>  		/* Big Hammer, we also need to ensure that any pending
>  		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> @@ -14175,14 +14181,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		 * This should only fail upon a hung GPU, in which case we
>  		 * can safely continue.
>  		 */
> -		if (needs_modeset(crtc_state)) {
> -			ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> -							      old_obj->resv, NULL,
> -							      false, 0,
> -							      GFP_KERNEL);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		ret = i915_sw_fence_await_reservation(&intel_state->commit_ready,
> +						      old_obj->resv, NULL,
> +						      false, 0,
> +						      GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	if (new_state->fence) { /* explicit fencing */
> @@ -14221,7 +14225,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  
>  		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
>  		if (IS_ERR(vma))
> -			ret = PTR_ERR(vma);
> +			return PTR_ERR(vma);

I don't have this one here, I guess you have some different baseline.
Current code already has the direct return (but also a debug output on
top). Looking closer that's a mistake in the preceeding patch, which also
drops the debug output. I didn't spot that, so please fix that patch by
dropping the hunk and then you can drop this one here.

With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  		to_intel_plane_state(new_state)->vma = vma;
>  	}
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state
  2016-11-15  8:58 ` [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state Chris Wilson
  2016-11-15  9:52   ` Daniel Vetter
@ 2016-11-15 10:24   ` Daniel Vetter
  1 sibling, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2016-11-15 10:24 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote:
> @@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  cleanup_request:
>  	i915_add_request_no_flush(request);
>  cleanup_unpin:
> -	intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
> +	to_intel_plane_state(primary->state)->vma = work->old_vma;
> +	intel_unpin_fb_vma(vma);
>  cleanup_pending:
>  	atomic_dec(&intel_crtc->unpin_work_count);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		struct i915_vma *vma;
>  
>  		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> -		if (IS_ERR(vma)) {
> -			DRM_DEBUG_KMS("failed to pin object\n");
> -			return PTR_ERR(vma);
> -		}
> +		if (IS_ERR(vma))
> +			ret = PTR_ERR(vma);

Uncessary change above that you then go ahead and partially undo in the
next patch. Please remove.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit
  2016-10-29 12:04 Chris Wilson
@ 2016-10-29 12:04 ` Chris Wilson
  0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2016-10-29 12:04 UTC (permalink / raw)
  To: intel-gfx

The generic atomic helper likes to skip a prepare_plane_fb() if it
decides that the plane->fb is unchanged. This is wrong for us for a
couple of reasons:

 - if the pipe is reconfigured (i.e. a size change) but the framebuffer
   is untouched, we still have to flush any rendering prior to the
   reconfiguration to prevent wait-for-scanline GPU hangs

 - if the framebuffer is rotated, it remains the same but has a
   different view and a different address.

Finally, even if the framebuffer is unchanged the flip/modeset should be
ordered with respect to rendering to the frontbuffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  2 -
 drivers/gpu/drm/i915/intel_display.c      | 61 ++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h          |  4 --
 3 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index cb5594411bb6..4ed5b2e49691 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -192,8 +192,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 }
 
 const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
-	.prepare_fb = intel_prepare_plane_fb,
-	.cleanup_fb = intel_cleanup_plane_fb,
 	.atomic_check = intel_plane_atomic_check,
 	.atomic_update = intel_plane_atomic_update,
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c817215f61a..69720a126c67 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14146,7 +14146,7 @@ static int intel_atomic_check(struct drm_device *dev,
  *
  * Returns 0 on success, negative error code on failure.
  */
-int
+static int
 intel_prepare_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *new_state)
 {
@@ -14241,7 +14241,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  *
  * Must be called with struct_mutex held.
  */
-void
+static void
 intel_cleanup_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *old_state)
 {
@@ -14260,6 +14260,50 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
 }
 
+static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i, j, ret;
+
+	ret = mutex_lock_interruptible(&state->dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ret = intel_prepare_plane_fb(plane, plane_state);
+		if (ret)
+			break;
+	}
+
+	if (ret) {
+		for_each_plane_in_state(state, plane, plane_state, j) {
+			if (j >= i)
+				break;
+
+			intel_cleanup_plane_fb(plane, plane_state);
+		}
+	}
+
+	mutex_unlock(&state->dev->struct_mutex);
+
+	return ret;
+}
+
+static void intel_atomic_commit_cleanup_planes(struct drm_atomic_state *state)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i;
+
+	mutex_lock(&state->dev->struct_mutex);
+
+	for_each_plane_in_state(state, plane, plane_state, i)
+		intel_cleanup_plane_fb(plane, plane_state);
+
+	mutex_unlock(&state->dev->struct_mutex);
+}
+
 static int intel_atomic_prepare_commit(struct drm_device *dev,
 				       struct drm_atomic_state *state)
 {
@@ -14280,14 +14324,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 			flush_workqueue(dev_priv->wq);
 	}
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
+	return intel_atomic_commit_prepare_planes(state);
 }
 
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
@@ -14614,9 +14651,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	if (intel_state->modeset)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 
-	mutex_lock(&dev->struct_mutex);
-	drm_atomic_helper_cleanup_planes(dev, state);
-	mutex_unlock(&dev->struct_mutex);
+	intel_atomic_commit_cleanup_planes(state);
 
 	drm_atomic_helper_commit_cleanup_done(state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f5975fe4553d..610f2f969a9a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1292,10 +1292,6 @@ __intel_framebuffer_create(struct drm_device *dev,
 void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe);
 void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe);
 void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe);
-int intel_prepare_plane_fb(struct drm_plane *plane,
-			   struct drm_plane_state *new_state);
-void intel_cleanup_plane_fb(struct drm_plane *plane,
-			    struct drm_plane_state *old_state);
 int intel_plane_atomic_get_property(struct drm_plane *plane,
 				    const struct drm_plane_state *state,
 				    struct drm_property *property,
-- 
2.10.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-11-15 10:24 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-15  8:58 [PATCH 1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Chris Wilson
2016-11-15  8:58 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson
2016-11-15  9:37   ` Daniel Vetter
2016-11-15  9:47     ` Tvrtko Ursulin
2016-11-15  9:57     ` Chris Wilson
2016-11-15 10:10       ` Daniel Vetter
2016-11-15 10:18         ` Chris Wilson
2016-11-15  8:58 ` [PATCH 3/5] drm/i915: Track pinned vma in intel_plane_state Chris Wilson
2016-11-15  9:52   ` Daniel Vetter
2016-11-15 10:02     ` Chris Wilson
2016-11-15 10:13       ` Daniel Vetter
2016-11-15 10:24   ` Daniel Vetter
2016-11-15  8:58 ` [PATCH 4/5] drm/i915: Quick spring clean of intel_prepare_plane_fb() Chris Wilson
2016-11-15 10:23   ` Daniel Vetter
2016-11-15  8:58 ` [PATCH 5/5] drm/i915: Set crtc_state->fb_changed whenever a VMA is changed Chris Wilson
2016-11-15 10:15   ` Daniel Vetter
2016-11-15  9:15 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915: Move intel_prepare_plane_fb() and intel_cleanup_plane_fb() Patchwork
2016-11-15  9:28 ` [PATCH 1/5] " Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2016-10-29 12:04 Chris Wilson
2016-10-29 12:04 ` [PATCH 2/5] drm/i915: Always prepare planes at the start of an atomic commit Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.