[Intel-gfx] [PATCH] drm/i915: Clarify the safety of the early unpin of old_fb->obj

[Chris Wilson]

Daniel simplified the modesetting code to push the common work performed
by each of the architecture specific routines higher into the caller. This
took me a while to be sure that it was safe in the event of a
modesetting failure - to be sure that the dangling pointer we left in
the registers would never be accessed. As it turns out, it is safe, as
even after the most convoluted error path, we always rewrite the address
registers with the currently pinned object before enabling the planes
and pipes. This patch just adds an assertion that the preconditions
Daniel stated are correct, and a comment to justify the dance.

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 28f31145335d..907ed158c676 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1193,7 +1193,7 @@ static void assert_cursor(struct drm_i915_private *dev_priv,
 #define assert_cursor_enabled(d, p) assert_cursor(d, p, true)
 #define assert_cursor_disabled(d, p) assert_cursor(d, p, false)
 
-void assert_pipe(struct drm_i915_private *dev_priv,
+bool assert_pipe(struct drm_i915_private *dev_priv,
 		 enum pipe pipe, bool state)
 {
 	int reg;
@@ -1215,12 +1215,12 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 		cur_state = !!(val & PIPECONF_ENABLE);
 	}
 
-	WARN(cur_state != state,
-	     "pipe %c assertion failure (expected %s, current %s)\n",
-	     pipe_name(pipe), state_string(state), state_string(cur_state));
+	return WARN(cur_state != state,
+		    "pipe %c assertion failure (expected %s, current %s)\n",
+		    pipe_name(pipe), state_string(state), state_string(cur_state));
 }
 
-static void assert_plane(struct drm_i915_private *dev_priv,
+static bool assert_plane(struct drm_i915_private *dev_priv,
 			 enum plane plane, bool state)
 {
 	int reg;
@@ -1230,9 +1230,9 @@ static void assert_plane(struct drm_i915_private *dev_priv,
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
 	cur_state = !!(val & DISPLAY_PLANE_ENABLE);
-	WARN(cur_state != state,
-	     "plane %c assertion failure (expected %s, current %s)\n",
-	     plane_name(plane), state_string(state), state_string(cur_state));
+	return WARN(cur_state != state,
+		    "plane %c assertion failure (expected %s, current %s)\n",
+		    plane_name(plane), state_string(state), state_string(cur_state));
 }
 
 #define assert_plane_enabled(d, p) assert_plane(d, p, true)
@@ -10308,6 +10308,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		struct drm_framebuffer *old_fb;
 
+		if (!assert_pipe_disabled(dev_priv, intel_crtc->pipe) ||
+		    !assert_plane_disabled(dev_priv, intel_crtc->plane)) {
+			ret = -EIO;
+			goto done;
+		}
+
+		/* The display engine is disabled. We can safely remove the
+		 * current object pointed to by hardware registers as before
+		 * we enable the pipe again, we will always update those
+		 * registers to point to the currently pinned object. Even
+		 * if we fail, though the hardware points to a stale address,
+		 * that address is never read.
+		 */
+
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(dev,
 						 to_intel_framebuffer(fb)->obj,
-- 
2.0.0.rc2