| Message ID | 20180115092041.13509-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/01/2018 09:20, Chris Wilson wrote: > In order to prevent a race condition where we may end up overaccounting > the active state and leaving the busy-stats believing the GPU is 100% > busy, lock out the tasklet while we reconstruct the busy state. There is > no direct spinlock guard for the execlists->port[], so we need to > utilise tasklet_disable() as a synchronous barrier to prevent the only > writer to execlists->port[] from running at the same time as the enable. > > Fixes: 4900727d35bb ("drm/i915/pmu: Reconstruct active state on starting busy-stats") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index d790bdc227ff..b221610f2365 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1943,16 +1943,22 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance) > */ > int intel_enable_engine_stats(struct intel_engine_cs *engine) > { > + struct intel_engine_execlists *execlists = &engine->execlists; > unsigned long flags; > + int err = 0; > > if (!intel_engine_supports_stats(engine)) > return -ENODEV; > > + tasklet_disable(&execlists->tasklet); > spin_lock_irqsave(&engine->stats.lock, flags); > - if (engine->stats.enabled == ~0) > - goto busy; > + > + if (unlikely(engine->stats.enabled == ~0)) { > + err = -EBUSY; > + goto unlock; > + } > + > if (engine->stats.enabled++ == 0) { > - struct intel_engine_execlists *execlists = &engine->execlists; > const struct execlist_port *port = execlists->port; > unsigned int num_ports = execlists_num_ports(execlists); > > @@ -1967,14 +1973,12 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) > if (engine->stats.active) > engine->stats.start = engine->stats.enabled_at; > } > - spin_unlock_irqrestore(&engine->stats.lock, flags); > > - return 0; > - > -busy: > +unlock: > spin_unlock_irqrestore(&engine->stats.lock, flags); > + tasklet_enable(&execlists->tasklet); > > - return -EBUSY; > + return err; > } > > static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine) > Hopefully the end of the immediate trouble, with only beautification remaining. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Patchwork (2018-01-15 12:11:48) > == Series Details == > > Series: drm/i915: Lock out execlist tasklet while peeking inside for busy-stats > URL : https://patchwork.freedesktop.org/series/36473/ > State : success > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7667/shards.html Looks good, pushed. Thanks for the review, hopefully the baseline remains green now and you can tidy up with confidence. -Chris
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index d790bdc227ff..b221610f2365 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1943,16 +1943,22 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance) */ int intel_enable_engine_stats(struct intel_engine_cs *engine) { + struct intel_engine_execlists *execlists = &engine->execlists; unsigned long flags; + int err = 0; if (!intel_engine_supports_stats(engine)) return -ENODEV; + tasklet_disable(&execlists->tasklet); spin_lock_irqsave(&engine->stats.lock, flags); - if (engine->stats.enabled == ~0) - goto busy; + + if (unlikely(engine->stats.enabled == ~0)) { + err = -EBUSY; + goto unlock; + } + if (engine->stats.enabled++ == 0) { - struct intel_engine_execlists *execlists = &engine->execlists; const struct execlist_port *port = execlists->port; unsigned int num_ports = execlists_num_ports(execlists); @@ -1967,14 +1973,12 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) if (engine->stats.active) engine->stats.start = engine->stats.enabled_at; } - spin_unlock_irqrestore(&engine->stats.lock, flags); - return 0; - -busy: +unlock: spin_unlock_irqrestore(&engine->stats.lock, flags); + tasklet_enable(&execlists->tasklet); - return -EBUSY; + return err; } static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine)
In order to prevent a race condition where we may end up overaccounting the active state and leaving the busy-stats believing the GPU is 100% busy, lock out the tasklet while we reconstruct the busy state. There is no direct spinlock guard for the execlists->port[], so we need to utilise tasklet_disable() as a synchronous barrier to prevent the only writer to execlists->port[] from running at the same time as the enable. Fixes: 4900727d35bb ("drm/i915/pmu: Reconstruct active state on starting busy-stats") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)