From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mika Kuoppala Subject: Re: [PATCH 1/2] drm/i915: fix up semaphore_waits_for Date: Fri, 21 Mar 2014 19:33:59 +0200 Message-ID: <87vbv7v4l4.fsf@gaia.fi.intel.com> References: <1394838536-5372-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 6404A6E3D2 for ; Fri, 21 Mar 2014 10:34:48 -0700 (PDT) In-Reply-To: <1394838536-5372-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Intel Graphics Development Cc: Daniel Vetter , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org Hi, Daniel Vetter writes: > There's an entire pile of issues in here: > > - Use the main RING_HEAD register, not ACTHD. ACTHD points at the gtt > offset of the batch buffer when a batch is executed. Semaphores are > always emitted to the main ring, so we always want to look at that. The ipehr check should make sure that we are at the ring and acthd should not be at batch. Regardless I agree that RING_HEAD is much more safer. When I have tried to get bottom of the snb reset hang, I have noticed that after reset the acthd is sometimes 0x0c even tho head is 0x00, on snb. > - Mask the obtained HEAD pointer with the actual ring size, which is > much smaller. Together with the above issue this resulted us in > trying to dereference a pointer way outside of the ring mmio > mapping. The resulting invalid access in interrupt context > (hangcheck is executed from timers) lead to a full blown kernel > panic. The fbcon panic handler then tried to frob our driver harder, > resulting in a full machine hang at least on my snb here where I've > stumbled over this. > > - Handle ring wrapping correctly and be a bit more explicit about how > many dwords we're scanning. We probably should also scan more than > just 4 ... ipehr dont update on MI_NOOPS and the head should point to the extra MI_NOOP after semaphore sequence. So 4 should be enough. Perhaps revisit this when bdw gains semaphores. > - Space out some of teh computations for readability. > > This prevents hard-hangs on my snb here. Verdict from QA is still > pending, but the symptoms match. > > Cc: Mika Kuoppala > Cc: Ben Widawsky > Cc: Chris Wilson > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74100 The hard hangs are not so frequent with this patch but they are still there. This patch should take care of the oops we have been seeing, but there is still something else to be found until #74100 can be marked as fixed. Very often after reset, when igt has pushed the quietence batches into rings, blitter and video ring heads gets moved properly but all updates to hardware status page and to the sync registers are missing. And result is hang by hangcheck. Repeat enough times and it is a hard hang. > Signed-off-by: Daniel Vetter Please remove the blowup comment and also update the bugzilla tag. Patches 1-2 and the followup one are Reviewed-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/i915_irq.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index be2713f12e08..473372a6c97d 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2529,29 +2529,43 @@ static struct intel_ring_buffer * > semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno) > { > struct drm_i915_private *dev_priv = ring->dev->dev_private; > - u32 cmd, ipehr, acthd, acthd_min; > + u32 cmd, ipehr, head; > + int i; > > ipehr = I915_READ(RING_IPEHR(ring->mmio_base)); > if ((ipehr & ~(0x3 << 16)) != > (MI_SEMAPHORE_MBOX | MI_SEMAPHORE_COMPARE | MI_SEMAPHORE_REGISTER)) > return NULL; > > - /* ACTHD is likely pointing to the dword after the actual command, > - * so scan backwards until we find the MBOX. > + /* > + * HEAD is likely pointing to the dword after the actual command, > + * so scan backwards until we find the MBOX. But limit it to just 3 > + * dwords. Note that we don't care about ACTHD here since that might > + * point at at batch, and semaphores are always emitted into the > + * ringbuffer itself. > */ > - acthd = intel_ring_get_active_head(ring) & HEAD_ADDR; > - acthd_min = max((int)acthd - 3 * 4, 0); > - do { > - cmd = ioread32(ring->virtual_start + acthd); > + head = I915_READ_HEAD(ring) & HEAD_ADDR; > + > + for (i = 4; i; --i) { > + /* > + * Be paranoid and presume the hw has gone off into the wild - > + * our ring is smaller than what the hardware (and hence > + * HEAD_ADDR) allows. Also handles wrap-around. > + */ > + head &= ring->size - 1; > + > + /* This here seems to blow up */ > + cmd = ioread32(ring->virtual_start + head); > if (cmd == ipehr) > break; > > - acthd -= 4; > - if (acthd < acthd_min) > - return NULL; > - } while (1); > + head -= 4; > + } > + > + if (!i) > + return NULL; > > - *seqno = ioread32(ring->virtual_start+acthd+4)+1; > + *seqno = ioread32(ring->virtual_start + head + 4) + 1; > return &dev_priv->ring[(ring->id + (((ipehr >> 17) & 1) + 1)) % 3]; > } > > -- > 1.8.1.4