drm/i915: Make the semaphore saturation mask global

The idea behind keeping the saturation mask local to a context backfired
spectacularly. The premise with the local mask was that we would be more
proactive in attempting to use semaphores after each time the context
idled, and that all new contexts would attempt to use semaphores
ignoring the current state of the system. This turns out to be horribly
optimistic. If the system state is still oversaturated and the existing
workloads have all stopped using semaphores, the new workloads would
attempt to use semaphores and be deprioritised behind real work. The
new contexts would not switch off using semaphores until their initial
batch of low priority work had completed. Given sufficient backload load
of equal user priority, this would completely starve the new work of any
GPU time.

To compensate, remove the local tracking in favour of keeping it as
global state on the engine -- once the system is saturated and
semaphores are disabled, everyone stops attempting to use semaphores
until the system is idle again. One of the reason for preferring local
context tracking was that it worked with virtual engines, so for
switching to global state we could either do a complete check of all the
virtual siblings or simply disable semaphores for those requests. This
takes the simpler approach of disabling semaphores on virtual engines.

The downside is that the decision that the engine is saturated is a
local measure -- we are only checking whether or not this context was
scheduled in a timely fashion, it may be legitimately delayed due to user
priorities. We still have the same dilemma though, that we do not want
to employ the semaphore poll unless it will be used.

v2: Explain why we need to assume the worst wrt virtual engines.

Fixes: 13edfd29d566 ("drm/i915: Disable semaphore busywaits on saturated systems")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Dmitry Ermilov <dmitry.ermilov@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190618074153.16055-8-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 42f45744d8595cc373681779a9e2fca175e79267..2c454f227c2e47cf94d05e8b2333e998fff5f6f0 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -142,7 +142,6 @@ intel_context_init(struct intel_context *ce,
        ce->engine = engine;
        ce->ops = engine->cops;
        ce->sseu = engine->sseu;
-       ce->saturated = 0;
 
        INIT_LIST_HEAD(&ce->signal_link);
        INIT_LIST_HEAD(&ce->signals);
@@ -223,7 +222,6 @@ void intel_context_enter_engine(struct intel_context *ce)
 
 void intel_context_exit_engine(struct intel_context *ce)
 {
-       ce->saturated = 0;
        intel_engine_pm_put(ce->engine);
 }
 

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index e95be4be9612f2f1c8308ae732bb15689cd7d70c..08049ee91ceeac210222941a64327d1bed3f9f03 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -53,8 +53,6 @@ struct intel_context {
        atomic_t pin_count;
        struct mutex pin_mutex; /* guards pinning and associated on-gpuing */
 
-       intel_engine_mask_t saturated; /* submitting semaphores too late? */
-
        /**
         * active: Active tracker for the rq activity (inc. external) on this
         * intel_context object.

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index d14e352b0b1720652ac099b62fe4122385b26616..2ce00d3dc42a1a5fa17b2175b78cd3657f88abe1 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -100,6 +100,8 @@ static int __engine_park(struct intel_wakeref *wf)
        struct intel_engine_cs *engine =
                container_of(wf, typeof(*engine), wakeref);
 
+       engine->saturated = 0;
+
        /*
         * If one and only one request is completed between pm events,
         * we know that we are inside the kernel context and it is

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index b2faca8e5decefa51bb21f5563c1978037ce0c64..868b220214f81b23087d7d8641345fea137b6640 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -298,6 +298,8 @@ struct intel_engine_cs {
        struct intel_context *kernel_context; /* pinned */
        struct intel_context *preempt_context; /* pinned; optional */
 
+       intel_engine_mask_t saturated; /* submitting semaphores too late? */
+
        unsigned long serial;
 
        unsigned long wakeref_serial;

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index c400c66d0ee5a2c990567beb41ea25e7033a1b07..df58d1d838f7445fdf77146c02e24f875913fe3f 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -3102,7 +3102,6 @@ static void virtual_context_exit(struct intel_context *ce)
        struct virtual_engine *ve = container_of(ce, typeof(*ve), context);
        unsigned int n;
 
-       ce->saturated = 0;
        for (n = 0; n < ve->num_siblings; n++)
                intel_engine_pm_put(ve->siblings[n]);
 }
@@ -3302,6 +3301,21 @@ intel_execlists_create_virtual(struct i915_gem_context *ctx,
        ve->base.instance = I915_ENGINE_CLASS_INVALID_VIRTUAL;
        ve->base.flags = I915_ENGINE_IS_VIRTUAL;
 
+       /*
+        * The decision on whether to submit a request using semaphores
+        * depends on the saturated state of the engine. We only compute
+        * this during HW submission of the request, and we need for this
+        * state to be globally applied to all requests being submitted
+        * to this engine. Virtual engines encompass more than one physical
+        * engine and so we cannot accurately tell in advance if one of those
+        * engines is already saturated and so cannot afford to use a semaphore
+        * and be pessimized in priority for doing so -- if we are the only
+        * context using semaphores after all other clients have stopped, we
+        * will be starved on the saturated system. Such a global switch for
+        * semaphores is less than ideal, but alas is the current compromise.
+        */
+       ve->base.saturated = ALL_ENGINES;
+
        snprintf(ve->base.name, sizeof(ve->base.name), "virtual");
 
        intel_engine_init_active(&ve->base, ENGINE_VIRTUAL);

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 7418e8f1ed6a21ca8b41733777d22c7a8bc20ccd..0c2b53b8a3d1c7e450884f1d0ffad769b2b4822c 100644 (file)
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -407,7 +407,7 @@ void __i915_request_submit(struct i915_request *request)
         */
        if (request->sched.semaphores &&
            i915_sw_fence_signaled(&request->semaphore))
-               request->hw_context->saturated |= request->sched.semaphores;
+               engine->saturated |= request->sched.semaphores;
 
        /* We may be recursing from the signal callback of another i915 fence */
        spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
@@ -787,7 +787,7 @@ already_busywaiting(struct i915_request *rq)
         *
         * See the are-we-too-late? check in __i915_request_submit().
         */
-       return rq->sched.semaphores | rq->hw_context->saturated;
+       return rq->sched.semaphores | rq->engine->saturated;
 }
 
 static int