Quoting Mika Kuoppala (2019-06-24 10:03:48) > Chris Wilson <ch...@chris-wilson.co.uk> writes: > > > In the unlikely case (thank you CI!), we may find ourselves wanting to > > issue a preemption but having no runnable requests left. In this case, > > we set the semaphore before computing the preemption and so must unset > > it before forgetting (or else we leave the machine busywaiting until the > > next request comes along and so likely hang). > > > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > > b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index c8a0c9b32764..efccc31887de 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -233,13 +233,18 @@ static inline u32 intel_hws_preempt_address(struct > > intel_engine_cs *engine) > > static inline void > > ring_set_paused(const struct intel_engine_cs *engine, int state) > > { > > + u32 *sema = &engine->status_page.addr[I915_GEM_HWS_PREEMPT]; > > + > > + if (*sema == state) > > + return; > > + > > So you want to avoid useless wmb, as I don't see other > benefit. Makes this look suspiciously racy but seems > to be just my usual paranoia.
It's always set under the execlists spinlock. > > /* > > * We inspect HWS_PREEMPT with a semaphore inside > > * engine->emit_fini_breadcrumb. If the dword is true, > > * the ring is paused as the semaphore will busywait > > * until the dword is false. > > */ > > - engine->status_page.addr[I915_GEM_HWS_PREEMPT] = state; > > + *sema = state; > > wmb(); > > } > > > > @@ -1243,6 +1248,8 @@ static void execlists_dequeue(struct intel_engine_cs > > *engine) > > *port = execlists_schedule_in(last, port - > > execlists->pending); > > memset(port + 1, 0, (last_port - port) * sizeof(*port)); > > execlists_submit_ports(engine); > > + } else { > > + ring_set_paused(engine, 0); > > This looks like a right thing to do. But why did we end up > figuring things out wrong in need_preempt()? It's because we didn't find anything else that needed the preemption after checking what came next in the queue -- it has already been completed by earlier submission. > One would think that if there were nothing to preempt into, > we would never set the pause in the first place. I hear you -- we try very hard to not even look for preemption. False preemption cycles show up as bad scheduling behaviour for saturated transcode jobs. > Also the preempt to idle cycle mention in effective_prio() > seems to be off. Could be that someone forgot to > point that out when he did review preempt-to-busy. Preempt-to-busy still has an effective idle point :-p -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx