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@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
