Re: drm/i915: Watchdog timeout: IRQ handler for gen8+

[Carlos Santa <carlos.santa@intel.com>]

On Mon, 2019-01-07 at 16:58 +0000, 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.

The watchdog timeout gets saved as part of the register state context
so it will still be enabled after coming back from preemption but the
timeout value will be reset back to the original MAX value that it was
programmed. At least that's what I remember from a discussion with
Michel but I can check again...

Regards,
Carlos

> 
> 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.
> 
> 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.
> 
> Regards,
> 
> Tvrtko

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