Quoting Tvrtko Ursulin (2018-01-12 11:43:11)
>
> On 12/01/2018 10:35, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-01-12 10:30:26)
> >>
> >>
> >> On 12/01/2018 09:51, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-01-12 09:40:40)
> >>>> So submit side doesn't work in either case, unless I am missing
> >>>> something. Would need the pair of port manipulation and context_in to be
> >>>> atomic.
> >>>
> >>> Sure, there is a small window with the result that we either never turn
> >>> off the stats, or turn them off one too early (and then recover if the
> >>> my understanding of the underflow protection holds). The same problem as
> >>> existed before the reconstruction, except now the window is much
> >>> smaller. I'm not that scared by this (it should not explode, nor result
> >>> in hopelessly wrong stats) so it can wait until stats enabling doesn't
> >>> need to peek at execlists. I think you will need to postpone enabling
> >>> until the next context-switch if we wanted to avoid the atomics; except
> >>> that poses a problem with the igt expecting busy-start to be accurate. A
> >>> dilemma for later.
> >>
> >> My analysis was partially incorrect, yes, there is underflow protection
> >> already.
> >>
> >> But I don't see that there is a race window where we end up with
> >> permanent 100% busyness before the reconstruction patch. Where do you
> >> see that?
> >>
> >> The worst I see without the reconstruction is to miss the accounting of
> >> the batch currently in progress when stats get enabled. Which is a much
> >> less serious, totally ignorable event.
> >
> > One is observable via pmu, the other not. If we fail to turn off
> > busy-stats accounting, nothing is lost except for a few wasted cycles.
> > Except on the next pmu, it starts from the previous cs instead of the
> > enabling -- but that is a problem that also exists with the second user.
>
> I don't follow, I am talking about permanent 100%. Let me copy what I
> wrote earlier:
>
> port0 context complete
> context_out - not enabled, no-op
> stats enable - port0 busy, active = 1
> port0 clear
> submit
> context_in - active = 2 !!! BAD
> port0 set
>
> That leads to permanent 100% until busy stats are disabled. I think that
> is hugely less desirable than just failing to account for the currently
> running batch, which was the case before reconstruction on enable, and
> is 99.9% only a problem for IGTs.
>
> Do you think there is a flaw in this analysis or something else?
No, I don't think it's a huge problem, an improbable race for which the
quick-and-dirty cure may worse than the disease:
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 25360ce0353f..62d9ee9d45a6 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1980,16 +1980,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);
@@ -2004,14 +2010,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)
using the tasklet control as a spinlock. Whereas the previous race did
not take much effort to observe.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
