Ben Widawsky <ben at bwidawsk.net> writes:
> Mostly just me talking outloud...
Much appreciated!
> 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.
I received a claim, which I haven't verified, that oglconform
would have a test with vertex shader loop that manages to bring this
up.
Well I suspect that we have some other problems in recovery for that
test than the actual loop.
>>
>> 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 was poor judgement form my part to mail this patch. It is horrible.
It is not wise to throw garbage at people if you want to discuss.
I have split this up to smaller parts, will post after i get the
commit messages in shape.
> It sounds like a good idea but there are a lot of pitfalls involved.
Idea of tracking the ring progression came as the remedy to the verter
shader loop test problem in oglconform. As the loop keeps executing
the achtd kept changing and no hang was declared. Perhaps hang would
happened eventually if the ring would have filled up? But it would
happen only after lots of batch buffer submissions.
-Mika
>>
>> 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
