| Message ID | 20191010071434.31195-10-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915: Note the addition of timeslicing to the pretend scheduler | expand |
On 10/10/2019 08:14, Chris Wilson wrote: > If we do find ourselves with an idle barrier inside our active while > waiting, attempt to flush it by emitting a pulse using the kernel > context. The point of this one completely escapes me at the moment. Idle barriers are kept in there to be consumed by the engine_pm parking, so if any random waiter finds some (there will always be some, as long as the engine executed some user context, right?), why would it want to handle them? Again just to use the opportunity for some house keeping? But what if the system is otherwise quite busy and a low-priority client just happens to want to wait on something silly? Regards, Tvrtko > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 14 +++++++++++++ > .../gpu/drm/i915/gt/intel_engine_heartbeat.h | 1 + > drivers/gpu/drm/i915/i915_active.c | 21 +++++++++++++++++-- > 3 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > index f68acf9118f3..e27bb7f028bd 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > @@ -169,3 +169,17 @@ int intel_engine_pulse(struct intel_engine_cs *engine) > intel_engine_pm_put(engine); > return err; > } > + > +int intel_engine_flush_barriers(struct intel_engine_cs *engine) > +{ > + struct i915_request *rq; > + > + rq = i915_request_create(engine->kernel_context); > + if (IS_ERR(rq)) > + return PTR_ERR(rq); > + > + idle_pulse(engine, rq); > + i915_request_add(rq); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > index 39391004554d..0c1ad0fc091d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > @@ -15,5 +15,6 @@ void intel_engine_park_heartbeat(struct intel_engine_cs *engine); > void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine); > > int intel_engine_pulse(struct intel_engine_cs *engine); > +int intel_engine_flush_barriers(struct intel_engine_cs *engine); > > #endif /* INTEL_ENGINE_HEARTBEAT_H */ > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > index aa37c07004b9..98d5fe1c7e19 100644 > --- a/drivers/gpu/drm/i915/i915_active.c > +++ b/drivers/gpu/drm/i915/i915_active.c > @@ -6,6 +6,7 @@ > > #include <linux/debugobjects.h> > > +#include "gt/intel_engine_heartbeat.h" > #include "gt/intel_engine_pm.h" > > #include "i915_drv.h" > @@ -435,6 +436,21 @@ static void enable_signaling(struct i915_active_fence *active) > dma_fence_put(fence); > } > > +static int flush_barrier(struct active_node *it) > +{ > + struct intel_engine_cs *engine; > + > + if (!is_barrier(&it->base)) > + return 0; > + > + engine = __barrier_to_engine(it); > + smp_rmb(); /* serialise with add_active_barriers */ > + if (!is_barrier(&it->base)) > + return 0; > + > + return intel_engine_flush_barriers(engine); > +} > + > int i915_active_wait(struct i915_active *ref) > { > struct active_node *it, *n; > @@ -448,8 +464,9 @@ int i915_active_wait(struct i915_active *ref) > /* Flush lazy signals */ > enable_signaling(&ref->excl); > rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) { > - if (is_barrier(&it->base)) /* unconnected idle barrier */ > - continue; > + err = flush_barrier(it); /* unconnected idle barrier? */ > + if (err) > + break; > > enable_signaling(&it->base); > } >
Quoting Tvrtko Ursulin (2019-10-11 15:56:35) > > On 10/10/2019 08:14, Chris Wilson wrote: > > If we do find ourselves with an idle barrier inside our active while > > waiting, attempt to flush it by emitting a pulse using the kernel > > context. > > The point of this one completely escapes me at the moment. Idle barriers > are kept in there to be consumed by the engine_pm parking, so if any > random waiter finds some (there will always be some, as long as the > engine executed some user context, right?), Not any random waiter; the waiter has to be waiting on a context that was active and so setup a barrier. > why would it want to handle > them? Again just to use the opportunity for some house keeping? But what > if the system is otherwise quite busy and a low-priority client just > happens to want to wait on something silly? There's no guarantee that it will ever be flushed. So why wouldn't we use a low priority request to give a semblance of forward progress and give a guarantee that the wait will complete. It's a hypothetical point, there are no waiters that need to wait upon their own barriers at present. We are just completing the picture for idle barrier tracking. -Chris
On 11/10/2019 16:11, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-10-11 15:56:35) >> >> On 10/10/2019 08:14, Chris Wilson wrote: >>> If we do find ourselves with an idle barrier inside our active while >>> waiting, attempt to flush it by emitting a pulse using the kernel >>> context. >> >> The point of this one completely escapes me at the moment. Idle barriers >> are kept in there to be consumed by the engine_pm parking, so if any >> random waiter finds some (there will always be some, as long as the >> engine executed some user context, right?), > > Not any random waiter; the waiter has to be waiting on a context that > was active and so setup a barrier. > >> why would it want to handle >> them? Again just to use the opportunity for some house keeping? But what >> if the system is otherwise quite busy and a low-priority client just >> happens to want to wait on something silly? > > There's no guarantee that it will ever be flushed. So why wouldn't we > use a low priority request to give a semblance of forward progress and > give a guarantee that the wait will complete. > > It's a hypothetical point, there are no waiters that need to wait upon > their own barriers at present. We are just completing the picture for > idle barrier tracking. Hm I was mistakenly remembering things like rpcs reconfiguration would wait on ce->active, but I forgot about your trick with putting kernel context request on an user timeline. I guess it is fine there, but since, and as you have said, it is hypothetical, then this patch is dead code and can wait. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-10-14 14:08:12) > > On 11/10/2019 16:11, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-10-11 15:56:35) > >> > >> On 10/10/2019 08:14, Chris Wilson wrote: > >>> If we do find ourselves with an idle barrier inside our active while > >>> waiting, attempt to flush it by emitting a pulse using the kernel > >>> context. > >> > >> The point of this one completely escapes me at the moment. Idle barriers > >> are kept in there to be consumed by the engine_pm parking, so if any > >> random waiter finds some (there will always be some, as long as the > >> engine executed some user context, right?), > > > > Not any random waiter; the waiter has to be waiting on a context that > > was active and so setup a barrier. > > > >> why would it want to handle > >> them? Again just to use the opportunity for some house keeping? But what > >> if the system is otherwise quite busy and a low-priority client just > >> happens to want to wait on something silly? > > > > There's no guarantee that it will ever be flushed. So why wouldn't we > > use a low priority request to give a semblance of forward progress and > > give a guarantee that the wait will complete. > > > > It's a hypothetical point, there are no waiters that need to wait upon > > their own barriers at present. We are just completing the picture for > > idle barrier tracking. > > Hm I was mistakenly remembering things like rpcs reconfiguration would > wait on ce->active, but I forgot about your trick with putting kernel > context request on an user timeline. > > I guess it is fine there, but since, and as you have said, it is > hypothetical, then this patch is dead code and can wait. Why would we even bother checking against the potential invalid pointer dereference then?... :-p -Chris
Quoting Tvrtko Ursulin (2019-10-14 14:08:12) > > On 11/10/2019 16:11, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-10-11 15:56:35) > >> > >> On 10/10/2019 08:14, Chris Wilson wrote: > >>> If we do find ourselves with an idle barrier inside our active while > >>> waiting, attempt to flush it by emitting a pulse using the kernel > >>> context. > >> > >> The point of this one completely escapes me at the moment. Idle barriers > >> are kept in there to be consumed by the engine_pm parking, so if any > >> random waiter finds some (there will always be some, as long as the > >> engine executed some user context, right?), > > > > Not any random waiter; the waiter has to be waiting on a context that > > was active and so setup a barrier. > > > >> why would it want to handle > >> them? Again just to use the opportunity for some house keeping? But what > >> if the system is otherwise quite busy and a low-priority client just > >> happens to want to wait on something silly? > > > > There's no guarantee that it will ever be flushed. So why wouldn't we > > use a low priority request to give a semblance of forward progress and > > give a guarantee that the wait will complete. > > > > It's a hypothetical point, there are no waiters that need to wait upon > > their own barriers at present. We are just completing the picture for > > idle barrier tracking. > > Hm I was mistakenly remembering things like rpcs reconfiguration would > wait on ce->active, but I forgot about your trick with putting kernel > context request on an user timeline. > > I guess it is fine there, but since, and as you have said, it is > hypothetical, then this patch is dead code and can wait. Ok, I have a use for this now! In "drm/i915: Allow userspace to specify ringsize on construction" we need to wait on the context itself to idle, i.e. i915_active_wait(&ce->active) and so now it is possible for us to be waiting on an idle_barrier() and so the flush be beneficial. static int __apply_ringsize(struct intel_context *ce, void *sz) { int err; err = i915_active_wait(&ce->active); if (err < 0) return err; if (intel_context_lock_pinned(ce)) return -EINTR; if (intel_context_is_pinned(ce)) { err = -EBUSY; /* In active use, come back later! */ goto unlock; } if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) { struct intel_ring *ring; /* Replace the existing ringbuffer */ ring = intel_engine_create_ring(ce->engine, (unsigned long)sz); if (IS_ERR(ring)) { err = PTR_ERR(ring); goto unlock; } intel_ring_put(ce->ring); ce->ring = ring; /* Context image will be updated on next pin */ } else { ce->ring = sz; } unlock: intel_context_unlock_pinned(ce); return err; } -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c index f68acf9118f3..e27bb7f028bd 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -169,3 +169,17 @@ int intel_engine_pulse(struct intel_engine_cs *engine) intel_engine_pm_put(engine); return err; } + +int intel_engine_flush_barriers(struct intel_engine_cs *engine) +{ + struct i915_request *rq; + + rq = i915_request_create(engine->kernel_context); + if (IS_ERR(rq)) + return PTR_ERR(rq); + + idle_pulse(engine, rq); + i915_request_add(rq); + + return 0; +} diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h index 39391004554d..0c1ad0fc091d 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h @@ -15,5 +15,6 @@ void intel_engine_park_heartbeat(struct intel_engine_cs *engine); void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine); int intel_engine_pulse(struct intel_engine_cs *engine); +int intel_engine_flush_barriers(struct intel_engine_cs *engine); #endif /* INTEL_ENGINE_HEARTBEAT_H */ diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index aa37c07004b9..98d5fe1c7e19 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -6,6 +6,7 @@ #include <linux/debugobjects.h> +#include "gt/intel_engine_heartbeat.h" #include "gt/intel_engine_pm.h" #include "i915_drv.h" @@ -435,6 +436,21 @@ static void enable_signaling(struct i915_active_fence *active) dma_fence_put(fence); } +static int flush_barrier(struct active_node *it) +{ + struct intel_engine_cs *engine; + + if (!is_barrier(&it->base)) + return 0; + + engine = __barrier_to_engine(it); + smp_rmb(); /* serialise with add_active_barriers */ + if (!is_barrier(&it->base)) + return 0; + + return intel_engine_flush_barriers(engine); +} + int i915_active_wait(struct i915_active *ref) { struct active_node *it, *n; @@ -448,8 +464,9 @@ int i915_active_wait(struct i915_active *ref) /* Flush lazy signals */ enable_signaling(&ref->excl); rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) { - if (is_barrier(&it->base)) /* unconnected idle barrier */ - continue; + err = flush_barrier(it); /* unconnected idle barrier? */ + if (err) + break; enable_signaling(&it->base); }
If we do find ourselves with an idle barrier inside our active while waiting, attempt to flush it by emitting a pulse using the kernel context. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 14 +++++++++++++ .../gpu/drm/i915/gt/intel_engine_heartbeat.h | 1 + drivers/gpu/drm/i915/i915_active.c | 21 +++++++++++++++++-- 3 files changed, 34 insertions(+), 2 deletions(-)