Re: [PATCH 0/4] [RFC] use HW watchdog timer

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 16, 2012 at 11:51:55AM -0700, Ben Widawsky wrote:
> This was my pet project for the last few days, but I have to take a
> break from working on it for now to do some real work ;-). The patches
> compile, and pass a basic test, but that's about it. There is still
> quite a bit of work left to make this useful. The easiest thing would be
> to tie this into error state.
> 
> The idea is pretty simple. It uses the HW watchdog to set a timeout per
> batchbuffer, instead of a global software watchdog.
> 
> Pros:
> * Potential for per batch, or ring watchdog values. I believe when/if we
> get to GPGPU workloads, this is particularly interesting.
> * Batch granularity hang detection. This mostly just makes hang
> detection and recovery a bit easier IMO.
> 
> Cons:
> * Blit ring doesn't have an interrupt. This means we still need the
> software watchdog, and it makes hang detection more complex. I've been
> led to believe future HW *may* have this interrupt.
> * Semaphores 
> 
> I'm looking for feedback, mainly for Daniel, and Chris if this is worth
> pursuing further when I have more time. The idea would be to eventually
> use this to implement much of the ARB robustness requirements instead of
> doing a bunch of request list processing.
> 
> Ben Widawsky (4):
>   drm/i915: Use HW watchdog for each batch
>   drm/i915: Turn on watchdog interrupts
>   drm/i915: Add a breadcrumb
>   drm/i915: Display the failing seqno

High-level drive-by review:

I think it's very important to separate hangs in the batch itself from
hangs in the ringbuffers, e.g. semaphore deadlocks. We should only blame
the former on the userspace-submitted batchbuffer. So on that ground I
think the following speudo-code check in the hangcheck code would be
simpler to implement and more robust:

/* Check whether we're outside of the ring. At worst this check presumes
 * that the hang is in the ring, never the other way round. */
if (ACTHEAD != RING_HEAD)
	/* Check whether ACTHEAD lies in any active ring. We can't check
	 * the object's gpu_domain, that might have been changed again
	 * already. */
	for_each_active_buffer(buffer)
		if (buffer->gtt_offset + buffer->size < ACTHEAD &&
		    buffer->gtt_offset >= ACTHEAD)
			goto found;

Otoh I don't think your patches here are a completely lost cause. This
render watchdog could be used rather well for a per-patch short timeout to
kill one specific batchbuffer I guess. But right now I don't see an
immediate use-case ...

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48