Re: [PATCH v4 2/5] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/03/2019 00:15, Carlos Santa wrote:
> On Mon, 2019-03-11 at 10:39 +0000, Tvrtko Ursulin wrote:
>> On 08/03/2019 03:16, Carlos Santa wrote:
>>> On Fri, 2019-03-01 at 09:36 +0000, Chris Wilson wrote:
>>>>>
>>>>
>>>> Quoting Carlos Santa (2019-02-21 02:58:16)
>>>>> +#define GEN8_WATCHDOG_1000US(dev_priv)
>>>>> watchdog_to_clock_counts(dev_priv, 1000)
>>>>> +static void gen8_watchdog_irq_handler(unsigned long data)
>>>>> +{
>>>>> +       struct intel_engine_cs *engine = (struct
>>>>> intel_engine_cs
>>>>> *)data;
>>>>> +       struct drm_i915_private *dev_priv = engine->i915;
>>>>> +       unsigned int hung = 0;
>>>>> +       u32 current_seqno=0;
>>>>> +       char msg[80];
>>>>> +       unsigned int tmp;
>>>>> +       int len;
>>>>> +
>>>>> +       /* Stop the counter to prevent further timeout
>>>>> interrupts
>>>>> */
>>>>> +       I915_WRITE_FW(RING_CNTR(engine->mmio_base),
>>>>> get_watchdog_disable(engine));
>>>>> +
>>>>> +       /* Read the heartbeat seqno once again to check if we
>>>>> are
>>>>> stuck? */
>>>>> +       current_seqno =
>>>>> intel_engine_get_hangcheck_seqno(engine);
>>>>
>>>> I have said this before, but this doesn't exist either, it's just
>>>> a
>>>> temporary glitch in the matrix.
>>>>
>>>
>>> Chris, Tvrtko, I need some guidance on how to find the quilty seqno
>>> during a hang, can you please advice here what to do?
>>
>> When an interrupt fires you need to ascertain whether the same
>> request
>> which enabled the watchdog is running, correct?
>>
>> So I think you would need this, with a disclaimer that I haven't
>> thought
>> about the details really:
>>
>> 1. Take a reference to timeline hwsp when setting up the watchdog for
>> a
>> request.
>>
>> 2. Store the initial seqno associated with this request.
>>
>> 3. Force enable user interrupts.
>>
>> 4. When timeout fires, inspect the HWSP seqno to see if the request
>> completed or not.
>>
>> 5. Reset the engine if not completed.
>>
>> 6. Put the timeline/hwsp reference.
> 
> 
> static int gen8_emit_bb_start(struct i915_request *rq,
> 							u64 offset, u32
> len,
> 							const unsigned
> int flags)
> {
> 	struct i915_timeline *tl;
> 	u32 seqno;
> 
> 	if (enable_watchdog) {
> 		/* Start watchdog timer */
> 		cs = gen8_emit_start_watchdog(rq, cs);
> 		tl = ce->ring->timeline;
> 		i915_timeline_get_seqno(tl, rq, &seqno);
> 		/*Store initial hwsp seqno associated with this request
> 		engine->watchdog_hwsp_seqno = tl->hwsp_seqno;

You should not need to allocate a new seqno and also having something 
stored per engine does not make clear how will you solve out of order.

Maybe you just set up the timer, then lets see below..

Also, are you not trying to do the software implementation to start with?

> 	}
> 
> }
> 
> static void gen8_watchdog_tasklet(unsigned long data)
> {
> 		struct i915_request *rq;
> 
> 		rq = intel_engine_find_active_request(engine);
> 
> 		/* Inspect the watchdog seqno once again for
> completion? */
> 		if (!i915_seqno_passed(engine->watchdog_hwsp_seqno, rq-
>> fence.seqno)) {
> 			//Reset Engine
> 		}
> }

What happens if you simply reset without checking anything? You know hw 
timer wouldn't have fired if the context wasn't running, correct?

(Ignoring the race condition between interrupt raised -> hw interrupt 
delivered -> serviced -> tasklet scheduled -> tasklet running. Which may 
mean request has completed in the meantime and you reset the engine for 
nothing. But this is probably not 100% solvable.)

Regards,

Tvrtko

> Tvrtko, is the above acceptable to inspect whether the seqno has
> completed?
> 
> I noticed there's a helper function i915_request_completed(struct
> i915_request *rq) but it will require me to modify it in order to pass
> 2 different seqnos.
> 
> Regards,
> Carlos
> 
>>
>> If the user interrupt fires with the request completed cancel the
>> above
>> operations.
>>
>> There could be an inherent race between inspecting the seqno and
>> deciding to reset. Not sure at the moment what to do. Maybe just call
>> it
>> bad luck?
>>
>> I also think for the software implementation you need to force no
>> request coalescing for contexts with timeout set. Because you want
>> to
>> have 100% defined borders for request in and out - since the timeout
>> is
>> defined per request.
>>
>> In this case you don't need the user interrupt for the trailing edge
>> signal but can use context complete. Maybe putting hooks into
>> context_in/out in intel_lrc.c would work under these circumstances.
>>
>> Also if preempted you need to cancel the timer setup and store
>> elapsed
>> execution time.
>>
>> Or it may make sense to just disable preemption for these contexts.
>> Otherwise there is no point in trying to mandate the timeout?
>>
>> But it is also kind of bad since non-privileged contexts can make
>> themselves non-preemptable by setting the watchdog timeout.
>>
>> Maybe as a compromise we need to automatically apply an elevated
>> priority level, but not as high to be completely non-preemptable.
>> Sounds
>> like a hard question.
>>
>> Regards,
>>
>> Tvrtko
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx