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