From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake Date: Sat, 19 Mar 2011 07:55:01 +0000 Message-ID: <0d30dc$lgdgge@orsmga001.jf.intel.com> References: <1300489968-8574-1-git-send-email-ben@bwidawsk.net> <1300489968-8574-3-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 2DEE89E7A8 for ; Sat, 19 Mar 2011 00:55:05 -0700 (PDT) In-Reply-To: <1300489968-8574-3-git-send-email-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 18 Mar 2011 16:12:46 -0700, Ben Widawsky wrote: > There is a race condition between setting PWRCTXA and executing > MI_SET_CONTEXT. PWRCTXA must not be set until a valid context has been > written (or else the GPU could possible go into rc6, and return to an > invalid context). > > Reported-and-Tested-by: Gu Rui > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=28582 > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++++++++++++++--- > 1 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 790af25..c1e9c30 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6603,14 +6603,27 @@ static int ironlake_setup_rc6(struct drm_device *dev) > return 0; > } > > +static int ironlake_wait_set_context(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_buffer *ring = LP_RING(dev_priv); > + int ret = -EAGAIN; > + > + ret = wait_for((I915_READ_HEAD(ring) == I915_READ_TAIL(ring)), 1000); > + if (ret && atomic_read(&dev_priv->mm.wedged)) { > + ret = -EIO; > + } else if (!ret) { /* head == tail */ > + ret = 0; > + } > + > + return ret; > +} Let's make this a more generic function because it does look useful. static inline int intel_ring_wait_idle(struct intel_ring_buffer *ring) { return intel_wait_ring_buffer(ring, ring->space - 8); } > + > void ironlake_enable_rc6(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - /* rc6 disabled by default due to repeated reports of hanging during > - * boot and resume. > - */ > if (!i915_enable_rc6) > return; > > @@ -6640,6 +6653,19 @@ void ironlake_enable_rc6(struct drm_device *dev) > OUT_RING(MI_FLUSH); > ADVANCE_LP_RING(); > > + /* > + * Wait for the command parser to advance past MI_SET_CONTEXT. The HW > + * does an implicit flush, combined with MI_FLUSH above, it should be > + * safe to assume that renderctx is valid > + */ > + ret = ironlake_wait_set_context(dev); > + if (ret) { > + if (ret == -EAGAIN) > + DRM_ERROR("rc6 switch took too long, freeing the bo"); Just report the failure in all cases and avoid jargon (if possible)! if (intel_ring_wait_idle(LP_RING(dev_priv))) { DRM_ERROR("failed to enable automatic power saving\n"); return; } -- Chris Wilson, Intel Open Source Technology Centre