[PATCH 1/4] drm/i915: Teach hangcheck about long operations on rings

[PATCH 1/4] drm/i915: Teach hangcheck about long operations on rings

[Mika Kuoppala]

Some operations that happen in ringbuffer, like flushing,
can take significant amounts of time. After some intense
shader tests, the PIPE_CONTROL with flush can apparently last
longer time than what is our hangcheck tick (1500ms). If
this happens twice in a row, even with subsequent batches,
the hangcheck score decaying mechanism can't cope and
hang is declared.

Strip out actual head checking to a separate function and if
actual head has not moved, check if it is lingering inside the
ringbuffer as opposed to batch. If so, treat it as if it would be
inside loop to only slightly increment the hangcheck score.

References: https://bugs.freedesktop.org/show_bug.cgi?id=93029
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e88d692..6ed6571 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2914,11 +2914,11 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
 }
 
 static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_engine_cs *ring, u64 acthd)
+head_stuck(struct intel_engine_cs *ring, u64 acthd)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 tmp;
+	u32 head;
 
 	if (acthd != ring->hangcheck.acthd) {
 		if (acthd > ring->hangcheck.max_acthd) {
@@ -2929,6 +2929,30 @@ ring_stuck(struct intel_engine_cs *ring, u64 acthd)
 		return HANGCHECK_ACTIVE_LOOP;
 	}
 
+	head = I915_READ_HEAD(ring) & HEAD_ADDR;
+
+	/* Some operations, like pipe flush, can take a long time.
+	 * Detect if we are inside ringbuffer and treat these as if
+	 * the ring would be busy.
+	 */
+	if (lower_32_bits(acthd) == head)
+		return HANGCHECK_ACTIVE_LOOP;
+
+	return HANGCHECK_HUNG;
+}
+
+static enum intel_ring_hangcheck_action
+ring_stuck(struct intel_engine_cs *ring, u64 acthd)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum intel_ring_hangcheck_action ha;
+	u32 tmp;
+
+	ha = head_stuck(ring, acthd);
+	if (ha != HANGCHECK_HUNG)
+		return ha;
+
 	if (IS_GEN2(dev))
 		return HANGCHECK_HUNG;
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Let hangcheck score decay faster than loop increment

[Mika Kuoppala]

We decay hangcheck score, instead of setting it to zero,
when seqno moves forward. This was added as mechanism to
detect batches full of invalid waits. But with multiple runs of
very intensive batches (shader tests), the score accumulates
even without wait/kick pairs only by engine being active inside
shader loops multiple times in succession.

Prevent this mechanism to falsely trigger on loops by
decaying faster than we accumulate during active looping.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6ed6571..3507269 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3009,6 +3009,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 #define BUSY 1
 #define KICK 5
 #define HUNG 20
+#define BUSY_DECAY (2*BUSY)
 
 	if (!i915.enable_hangcheck)
 		return;
@@ -3084,8 +3085,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 			/* Gradually reduce the count so that we catch DoS
 			 * attempts across multiple batches.
 			 */
-			if (ring->hangcheck.score > 0)
-				ring->hangcheck.score--;
+			if (ring->hangcheck.score > BUSY_DECAY)
+				ring->hangcheck.score -= BUSY_DECAY;
 
 			ring->hangcheck.acthd = ring->hangcheck.max_acthd = 0;
 		}
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Clear hangcheck score if engine is idle

[Mika Kuoppala]

If we have accumulated hangcheck score before reaching
engine idle, that score will remain and is transferred to
the next set of batches. This is wrong as idle is a quite clear
boundary to prevent hangcheck score manipulation across batches.

Fix this by clearing the score if engine reaches idle.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3507269..c1d1400 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3091,6 +3091,9 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 			ring->hangcheck.acthd = ring->hangcheck.max_acthd = 0;
 		}
 
+		if (!busy)
+			ring->hangcheck.score = 0;
+
 		ring->hangcheck.seqno = seqno;
 		ring->hangcheck.acthd = acthd;
 		busy_count += busy;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Detect small loops in hangcheck

[Mika Kuoppala]

If there is very small loop in batch, the chances are quite high
that we sample the same head value twice in a row leading the
hangcheck score to be incremented with hung engine status, instead of
active loop which would have been more correct.

Try to resample the actual head few times to detect small loops
instead of jumping into conclusions.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c1d1400..7c1168b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2914,12 +2914,8 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
 }
 
 static enum intel_ring_hangcheck_action
-head_stuck(struct intel_engine_cs *ring, u64 acthd)
+head_action(struct intel_engine_cs *ring, u64 acthd)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 head;
-
 	if (acthd != ring->hangcheck.acthd) {
 		if (acthd > ring->hangcheck.max_acthd) {
 			ring->hangcheck.max_acthd = acthd;
@@ -2929,6 +2925,21 @@ head_stuck(struct intel_engine_cs *ring, u64 acthd)
 		return HANGCHECK_ACTIVE_LOOP;
 	}
 
+	return HANGCHECK_HUNG;
+}
+
+static enum intel_ring_hangcheck_action
+head_stuck(struct intel_engine_cs *ring, u64 acthd)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	static enum intel_ring_hangcheck_action ha;
+	u32 head, retries = 5;
+
+	ha = head_action(ring, acthd);
+	if (ha != HANGCHECK_HUNG)
+		return ha;
+
 	head = I915_READ_HEAD(ring) & HEAD_ADDR;
 
 	/* Some operations, like pipe flush, can take a long time.
@@ -2938,6 +2949,17 @@ head_stuck(struct intel_engine_cs *ring, u64 acthd)
 	if (lower_32_bits(acthd) == head)
 		return HANGCHECK_ACTIVE_LOOP;
 
+	do {
+		msleep(20);
+
+		ring->hangcheck.acthd = acthd;
+		acthd = intel_ring_get_active_head(ring);
+
+		ha = head_action(ring, acthd);
+		if (ha != HANGCHECK_HUNG)
+			return ha;
+	} while (retries--);
+
 	return HANGCHECK_HUNG;
 }
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Clear hangcheck score if engine is idle

[Chris Wilson]

On Mon, Nov 30, 2015 at 06:53:08PM +0200, Mika Kuoppala wrote:
> If we have accumulated hangcheck score before reaching
> engine idle, that score will remain and is transferred to
> the next set of batches. This is wrong as idle is a quite clear
> boundary to prevent hangcheck score manipulation across batches.
> 
> Fix this by clearing the score if engine reaches idle.

The hangcheck accumulation is to detect denial of service by a
sequence of batches that each take more than a hangcheck interval, being
idle doesn't mean anything with respect to the slow dos. The only
question is whether trying to do that at all is sensible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Teach hangcheck about long operations on rings

[Chris Wilson]

On Mon, Nov 30, 2015 at 06:53:06PM +0200, Mika Kuoppala wrote:
> Some operations that happen in ringbuffer, like flushing,
> can take significant amounts of time. After some intense
> shader tests, the PIPE_CONTROL with flush can apparently last
> longer time than what is our hangcheck tick (1500ms). If
> this happens twice in a row, even with subsequent batches,
> the hangcheck score decaying mechanism can't cope and
> hang is declared.
> 
> Strip out actual head checking to a separate function and if
> actual head has not moved, check if it is lingering inside the
> ringbuffer as opposed to batch. If so, treat it as if it would be
> inside loop to only slightly increment the hangcheck score.

The PIPE_CONTROL in the ring after the batch, is equivalent to the batch
performing its own PIPE_CONTROL as the last instruction. It does not
make sense to distinguish the two.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Let hangcheck score decay faster than loop increment

[Chris Wilson]

On Mon, Nov 30, 2015 at 06:53:07PM +0200, Mika Kuoppala wrote:
> We decay hangcheck score, instead of setting it to zero,
> when seqno moves forward. This was added as mechanism to
> detect batches full of invalid waits.

And slow DoS. So no.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Teach hangcheck about long operations on rings

[Dave Gordon]

On 30/11/15 17:11, Chris Wilson wrote:
> On Mon, Nov 30, 2015 at 06:53:06PM +0200, Mika Kuoppala wrote:
>> Some operations that happen in ringbuffer, like flushing,
>> can take significant amounts of time. After some intense
>> shader tests, the PIPE_CONTROL with flush can apparently last
>> longer time than what is our hangcheck tick (1500ms). If
>> this happens twice in a row, even with subsequent batches,
>> the hangcheck score decaying mechanism can't cope and
>> hang is declared.
>>
>> Strip out actual head checking to a separate function and if
>> actual head has not moved, check if it is lingering inside the
>> ringbuffer as opposed to batch. If so, treat it as if it would be
>> inside loop to only slightly increment the hangcheck score.
>
> The PIPE_CONTROL in the ring after the batch, is equivalent to the batch
> performing its own PIPE_CONTROL as the last instruction. It does not
> make sense to distinguish the two.
> -Chris

It's equivalent in terms of outcome, but not when checking what's 
happening. The driver controls insertion of PIPE_CONTROLs in the ring, 
but not in batches. If execution is at the ring level, we know it's 
running instructions that the driver put there, and we know that it 
*will* then progress to the next batch (assuming the hardware's not 
stuck). OTOH if execution is inside a batch then we don't know what 
sequence of instructions it's running, and we can't guarantee that the 
batch will ever terminate. So, a reduced penalty if executing 
driver-supplied code makes sense.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Teach hangcheck about long operations on rings

[Chris Wilson]

On Mon, Nov 30, 2015 at 06:04:54PM +0000, Dave Gordon wrote:
> On 30/11/15 17:11, Chris Wilson wrote:
> >On Mon, Nov 30, 2015 at 06:53:06PM +0200, Mika Kuoppala wrote:
> >>Some operations that happen in ringbuffer, like flushing,
> >>can take significant amounts of time. After some intense
> >>shader tests, the PIPE_CONTROL with flush can apparently last
> >>longer time than what is our hangcheck tick (1500ms). If
> >>this happens twice in a row, even with subsequent batches,
> >>the hangcheck score decaying mechanism can't cope and
> >>hang is declared.
> >>
> >>Strip out actual head checking to a separate function and if
> >>actual head has not moved, check if it is lingering inside the
> >>ringbuffer as opposed to batch. If so, treat it as if it would be
> >>inside loop to only slightly increment the hangcheck score.
> >
> >The PIPE_CONTROL in the ring after the batch, is equivalent to the batch
> >performing its own PIPE_CONTROL as the last instruction. It does not
> >make sense to distinguish the two.
> >-Chris
> 
> It's equivalent in terms of outcome, but not when checking what's
> happening. The driver controls insertion of PIPE_CONTROLs in the
> ring, but not in batches. If execution is at the ring level, we know
> it's running instructions that the driver put there, and we know
> that it *will* then progress to the next batch (assuming the
> hardware's not stuck). OTOH if execution is inside a batch then we
> don't know what sequence of instructions it's running, and we can't
> guarantee that the batch will ever terminate. So, a reduced penalty
> if executing driver-supplied code makes sense.

Not exactly. If it is executing an infinite loop in the shader, it will
hang indefinitely at whatever pipecontrol comes next. The pipecontrol
following the batch is a forced operation in the user context to ensure
correctness between batches.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Let hangcheck score decay faster than loop increment

[Daniel Vetter]

On Mon, Nov 30, 2015 at 05:18:43PM +0000, Chris Wilson wrote:
> On Mon, Nov 30, 2015 at 06:53:07PM +0200, Mika Kuoppala wrote:
> > We decay hangcheck score, instead of setting it to zero,
> > when seqno moves forward. This was added as mechanism to
> > detect batches full of invalid waits.
> 
> And slow DoS. So no.

Yeah we need to fix the deqp CI issues with tunables. Running a shader for
10s and monopolizing the gpu might be ok in a stress tests, but it's
definitely not ok by default on a normal system.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Let hangcheck score decay faster than loop increment

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Nov 30, 2015 at 06:53:07PM +0200, Mika Kuoppala wrote:
>> We decay hangcheck score, instead of setting it to zero,
>> when seqno moves forward. This was added as mechanism to
>> detect batches full of invalid waits.
>
> And slow DoS. So no.

Point was to decay faster than 'loop detection' (-2) but slower
than kicking (+5) to keep the score decaying even if there is
one 'loop detection' per batch.

The criticism about pipe control (patch 1/4) is warranted as it just
happens to help now when the synchronization happens on ring, and thus
only helps with batches that follow this pattern wrt pipe control.

I will post a patch that tries to achieve more generic way
by inspecting subunit states if head seems to be stuck, so
that substitutes 1/4. And also makes 4/4 even more moot.

There is not much to salvage in this series so please ignore.

Thanks,
-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx