From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Clarify the safety of the early unpin of old_fb->obj Date: Tue, 20 May 2014 10:16:54 +0200 Message-ID: <20140520081654.GG8790@phenom.ffwll.local> References: <1400572061-9763-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 0B78B6E2E6 for ; Tue, 20 May 2014 01:16:58 -0700 (PDT) Received: by mail-ee0-f49.google.com with SMTP id e53so266359eek.22 for ; Tue, 20 May 2014 01:16:58 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1400572061-9763-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > Cc: Daniel Vetter 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