On Thu, Feb 14, 2013 at 10:14:56PM -0800, Ben Widawsky wrote:
> Mostly just me talking outloud...
>
> On Mon, Feb 04, 2013 at 04:04:37PM +0200, Mika Kuoppala wrote:
> > If there was a batch chaining loop or infinite loop in the batchbuffer,
> > we didn't detect it as acthd and instdone kept changing in those cases
> > and hang was never declared.
>
> I don't think we can infinite loop on any platforms which exists today.
> Can we? I agree chaining issues can occur, but it seems like it'd be
> extremely rare. Furthermore if this did indeed occur, almost certainly
> we'd fill up the ring and fail in some other way anyway.
Infinitely looping chained batches was the simplest trick we could come up
with to get a hang in a batch. And we need such a thing since the current
i-g-t hangman always hangs in the ring. Better ideas highly welcome for
how this could be reliably tested without increased risks of killing the
gpu for good ...
Hm, that reminds me that this _will_ kill the gpu on gen2/3 where we don't
have reset support.
-Daniel
>
> >
> > To detect ring hangs, including infinite loops, keep track of ring
> > seqno progression.
>
> I'm having a lot of trouble reviewing this patch, which leads me to
> believe it should probably be split up a bit more, or I'm stupid
> (both?).
>
> It sounds like a good idea but there are a lot of pitfalls involved.
>
> >
> > Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 3 -
> > drivers/gpu/drm/i915/i915_irq.c | 127 +++++++++++++++----------------
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +
> > 3 files changed, 67 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 984523d..4daab74 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -791,9 +791,6 @@ struct i915_gpu_error {
> > #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> > #define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
> > struct timer_list hangcheck_timer;
> > - int hangcheck_count;
> > - uint32_t last_acthd[I915_NUM_RINGS];
> > - uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
> >
> > /* For reset and error_state handling. */
> > spinlock_t lock;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 5648d84..d0a9e21 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -356,7 +356,7 @@ static void notify_ring(struct drm_device *dev,
> >
> > wake_up_all(&ring->irq_queue);
> > if (i915_enable_hangcheck) {
> > - dev_priv->gpu_error.hangcheck_count = 0;
> > + ring->hangcheck_count = 0;
> > mod_timer(&dev_priv->gpu_error.hangcheck_timer,
> > round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
> > }
> > @@ -1727,11 +1727,11 @@ ring_last_seqno(struct intel_ring_buffer *ring)
> > struct drm_i915_gem_request, list)->seqno;
> > }
> >
> > -static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
> > +static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring,
> > + u32 ring_seqno, bool *err)
> > {
> > if (list_empty(&ring->request_list) ||
> > - i915_seqno_passed(ring->get_seqno(ring, false),
> > - ring_last_seqno(ring))) {
> > + i915_seqno_passed(ring_seqno, ring_last_seqno(ring))) {
> > /* Issue a wake-up to catch stuck h/w. */
> > if (waitqueue_active(&ring->irq_queue)) {
> > DRM_ERROR("Hangcheck timer elapsed... %s idle\n",
> > @@ -1749,39 +1749,32 @@ static bool kick_ring(struct intel_ring_buffer *ring)
> > struct drm_device *dev = ring->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 tmp = I915_READ_CTL(ring);
> > +
> > + ring->hangcheck_waiting = false;
> > +
> > if (tmp & RING_WAIT) {
> > DRM_ERROR("Kicking stuck wait on %s\n",
> > ring->name);
> > I915_WRITE_CTL(ring, tmp);
> > - return true;
> > + ring->hangcheck_waiting = true;
> > }
> > - return false;
> > -}
> > -
> > -static bool i915_hangcheck_hung(struct drm_device *dev)
> > -{
> > - drm_i915_private_t *dev_priv = dev->dev_private;
> >
> > - if (dev_priv->gpu_error.hangcheck_count++ > 1) {
> > - bool hung = true;
> > + if ((INTEL_INFO(dev)->gen >= 6) && (tmp & RING_WAIT_SEMAPHORE))
> > + ring->hangcheck_waiting = true;
> >
> > - DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
> > - i915_handle_error(dev, true);
> > -
> > - if (!IS_GEN2(dev)) {
> > - struct intel_ring_buffer *ring;
> > - int i;
> > -
> > - /* Is the chip hanging on a WAIT_FOR_EVENT?
> > - * If so we can simply poke the RB_WAIT bit
> > - * and break the hang. This should work on
> > - * all but the second generation chipsets.
> > - */
> > - for_each_ring(ring, dev_priv, i)
> > - hung &= !kick_ring(ring);
> > - }
> > + return ring->hangcheck_waiting;
> > +}
> >
> > - return hung;
> > +static bool i915_hangcheck_ring_hung(struct drm_device *dev,
> > + struct intel_ring_buffer *ring)
> > +{
> > + if (!IS_GEN2(dev)) {
> > + /* Is the chip hanging on a WAIT_FOR_EVENT?
> > + * If so we can simply poke the RB_WAIT bit
> > + * and break the hang. This should work on
> > + * all but the second generation chipsets.
> > + */
> > + return !kick_ring(ring);
> > }
> >
> > return false;
> > @@ -1789,62 +1782,68 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
> >
> > /**
> > * This is called when the chip hasn't reported back with completed
> > - * batchbuffers in a long time. The first time this is called we simply record
> > - * ACTHD. If ACTHD hasn't changed by the time the hangcheck timer elapses
> > - * again, we assume the chip is wedged and try to fix it.
> > + * batchbuffers in a long time. We record current seqno for each count and
> > + * in subsequent calls we check if requests have been processed by each ring.
> > + * If there is no progress on specific ring, we declare it as hung.
> > */
> > void i915_hangcheck_elapsed(unsigned long data)
> > {
> > struct drm_device *dev = (struct drm_device *)data;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
> > struct intel_ring_buffer *ring;
> > - bool err = false, idle;
> > + bool ring_hung;
> > int i;
> > + int busy_count = 0;
> >
> > if (!i915_enable_hangcheck)
> > return;
> >
> > - memset(acthd, 0, sizeof(acthd));
> > - idle = true;
> > for_each_ring(ring, dev_priv, i) {
> > - idle &= i915_hangcheck_ring_idle(ring, &err);
> > - acthd[i] = intel_ring_get_active_head(ring);
> > - }
> > + bool err = false, idle;
> > + u32 seqno;
> >
> > - /* If all work is done then ACTHD clearly hasn't advanced. */
> > - if (idle) {
> > - if (err) {
> > - if (i915_hangcheck_hung(dev))
> > - return;
> > + seqno = ring->get_seqno(ring, false);
> > + idle = i915_hangcheck_ring_idle(ring, seqno, &err);
> > +
> > + if (idle) {
> > + if (err)
> > + ring->hangcheck_count++;
> > + else
> > + ring->hangcheck_count = 0;
> > + } else {
> > + busy_count++;
> >
> > - goto repeat;
> > + if (ring->hangcheck_seqno == seqno) {
> > + ring->hangcheck_count++;
> > +
> > + /* If the ring is not waiting, raise the
> > + * hung score */
> > + if (i915_hangcheck_ring_hung(dev, ring))
> > + ring->hangcheck_count++;
> > + } else {
> > + ring->hangcheck_count = 0;
> > + }
> > }
> >
> > - dev_priv->gpu_error.hangcheck_count = 0;
> > - return;
> > + ring->hangcheck_seqno = seqno;
> > }
> >
> > - i915_get_extra_instdone(dev, instdone);
> > - if (memcmp(dev_priv->gpu_error.last_acthd, acthd,
> > - sizeof(acthd)) == 0 &&
> > - memcmp(dev_priv->gpu_error.prev_instdone, instdone,
> > - sizeof(instdone)) == 0) {
> > - if (i915_hangcheck_hung(dev))
> > - return;
> > - } else {
> > - dev_priv->gpu_error.hangcheck_count = 0;
> > -
> > - memcpy(dev_priv->gpu_error.last_acthd, acthd,
> > - sizeof(acthd));
> > - memcpy(dev_priv->gpu_error.prev_instdone, instdone,
> > - sizeof(instdone));
> > + ring_hung = false;
> > + for_each_ring(ring, dev_priv, i) {
> > + if (ring->hangcheck_count > 2) {
> > + ring_hung = true;
> > + DRM_ERROR("%s seems hung\n", ring->name);
> > + }
> > }
> >
> > -repeat:
> > + if (ring_hung)
> > + return i915_handle_error(dev, true);
> > +
> > /* Reset timer case chip hangs without another request being added */
> > - mod_timer(&dev_priv->gpu_error.hangcheck_timer,
> > - round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
> > + if (busy_count)
> > + mod_timer(&dev_priv->gpu_error.hangcheck_timer,
> > + round_jiffies_up(jiffies +
> > + DRM_I915_HANGCHECK_JIFFIES));
> > }
> >
> > /* drm_dma.h hooks
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index d66208c..7257252 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -137,6 +137,10 @@ struct intel_ring_buffer {
> > struct i915_hw_context *default_context;
> > struct drm_i915_gem_object *last_context_obj;
> >
> > + int hangcheck_count;
> > + u32 hangcheck_seqno;
> > + bool hangcheck_waiting;
> > +
> > void *private;
> > };
> >
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
