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

[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@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@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

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

[Chris Wilson]

On Tue, May 20, 2014 at 08:47:41AM +0100, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@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)

Sigh. I forgot this is now exported.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Tue, May 20, 2014 at 08:47:41AM +0100, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Hm, I think we should tackle the larger issue here and have a bit of a
plane config checker, to make sure reality matches up with what we expect
it to be. But I'm not sure that effort is worth it.

Wrt the assert I actually prefer we keep those in the low-level callbacks.
At least they often encode platform specifics, which will be more obvious
once we have atomic modesets support and also need to deal with sprites
and cursors here.
-Daniel
> ---
>  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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Tue, May 20, 2014 at 10:16:54AM +0200, Daniel Vetter wrote:
> On Tue, May 20, 2014 at 08:47:41AM +0100, Chris Wilson wrote:
> > 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@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Hm, I think we should tackle the larger issue here and have a bit of a
> plane config checker, to make sure reality matches up with what we expect
> it to be. But I'm not sure that effort is worth it.
> 
> Wrt the assert I actually prefer we keep those in the low-level callbacks.
> At least they often encode platform specifics, which will be more obvious
> once we have atomic modesets support and also need to deal with sprites
> and cursors here.

Long-term I want to share more of this prep code with the pageflip code
anyway, i.e. the overall code structure of the full-blown magic modeset
would be:

1. Compute config and figure out what needs to be done. Bail out if it's
not possible.
2. Pin buffers for everything (so including cursors and stuff).
3. Nuclear pageflip to solid black on all pipes we want to disable
(currently embedded in the crtc_disable hooks still)
4. Shut down pipes.
5. Commit sw state from staging areas to actual state pointers.
6. Fire up all the pipes again.
7. Nuclear pageflip from solid black to the desired output configuration.

If step 1 notices that nothing needs to be done for steps 4-6 then we'll
fuse steps 3&7 together and just do one nuclear pageflip. There's also
going to be a fun complication due to DP retraining.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Chris Wilson]

On Tue, May 20, 2014 at 10:16:54AM +0200, Daniel Vetter wrote:
> On Tue, May 20, 2014 at 08:47:41AM +0100, Chris Wilson wrote:
> > 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@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Hm, I think we should tackle the larger issue here and have a bit of a
> plane config checker, to make sure reality matches up with what we expect
> it to be. But I'm not sure that effort is worth it.
> 
> Wrt the assert I actually prefer we keep those in the low-level callbacks.
> At least they often encode platform specifics, which will be more obvious
> once we have atomic modesets support and also need to deal with sprites
> and cursors here.

Here they are testing the preconditions you stated for this code to be
safe. I think they are exactly where they need to be as they serve a
similar but different purpose to the lowlevel checks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre