[Intel-gfx] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Tvrtko Ursulin]

On 11/01/2019 00:47, Antonio Argenziano wrote:
> On 07/01/19 08:58, Tvrtko Ursulin wrote:
>> On 07/01/2019 13:57, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-01-07 13:43:29)
>>>>
>>>> On 07/01/2019 11:58, Tvrtko Ursulin wrote:
>>>>
>>>> [snip]
>>>>
>>>>>> Note about future interaction with preemption: Preemption could 
>>>>>> happen
>>>>>> in a command sequence prior to watchdog counter getting disabled,
>>>>>> resulting in watchdog being triggered following preemption (e.g. when
>>>>>> watchdog had been enabled in the low priority batch). The driver will
>>>>>> need to explicitly disable the watchdog counter as part of the
>>>>>> preemption sequence.
>>>>>
>>>>> Does the series take care of preemption?
>>>>
>>>> I did not find that it does.
>>>
>>> Oh. I hoped that the watchdog was saved as part of the context... Then
>>> despite preemption, the timeout would resume from where we left off as
>>> soon as it was back on the gpu.
>>>
>>> If the timeout remaining was context saved it would be much simpler (at
>>> least on first glance), please say it is.
>>
>> I made my comments going only by the text from the commit message and 
>> the absence of any preemption special handling.
>>
>> Having read the spec, the situation seems like this:
>>
>>   * Watchdog control and threshold register are context saved and 
>> restored.
>>
>>   * On a context switch watchdog counter is reset to zero and 
>> automatically disabled until enabled by a context restore or explicitly.
>>
>> So it sounds the commit message could be wrong that special handling 
>> is needed from this direction. But read till the end on the 
>> restriction listed.
>>
>>   * Watchdog counter is reset to zero and is not accumulated across 
>> multiple submission of the same context (due preemption).
>>
>> I read this as - after preemption contexts gets a new full timeout 
>> allocation. Or in other words, if a context is preempted N times, it's 
>> cumulative watchdog timeout will be N * set value.
>>
>> This could be theoretically exploitable to bypass the timeout. If a 
>> client sets up two contexts with prio -1 and -2, and keeps submitting 
>> periodical no-op batches against prio -1 context, while prio -2 is 
>> it's own hog, then prio -2 context defeats the watchdog timer. I 
>> think.. would appreciate is someone challenged this conclusion.
> 
> I think you are right that is a possibility but, is that a problem? The 
> client can just not set the threshold to bypass the timeout. Also 
> because you need the hanging batch to be simply preemptible, you cannot 
> disrupt any work from another client that is higher priority. This is 

But I think higher priority client can have the same effect on the lower 
priority purely by accident, no?

As a real world example, user kicks off an background transcoding job, 
which happens to use prio -2, and uses the watchdog timer.

At the same time user watches a video from a player of normal priority. 
This causes periodic, say 24Hz, preemption events, due frame decoding 
activity on the same engine as the transcoding client.

Does this defeat the watchdog timer for the former is the question? Then 
the questions of can we do something about it and whether it really 
isn't a problem?

Maybe it is not disrupting higher priority clients but it is causing an 
time unbound power drain.

> pretty much the same behavior of hangcheck IIRC so something we already 
> accept.

You mean today hangcheck wouldn't notice a hanging batch in the same 
scenario as above? If so it sounds like a huge gap we need to try and fix.

>>
>> And finally there is one programming restriction which says:
>>
>>   * SW must not preempt the workload which has watchdog enabled. 
>> Either it must:
>>
>> a) disable preemption for that workload completely, or
>> b) disable the watchdog via mmio write before any write to ELSP
>>
>> This seems it contradiction with the statement that the counter gets 
>> disabled on context switch and stays disabled.
>>
>> I did not spot anything like this in the series. So it would seem the 
>> commit message is correct after all.
>>
>> It would be good if someone could re-read the bspec text on register 
>> 0x2178 to double check what I wrote.
> 
> The way I read it is that the restriction applies only to some platforms 
> where the 'normal' description doesn't apply.

You are right. Are the listed parts in the field so the series would 
have to handle this or we can ignore it?

Regards,

Tvrtko