On 28/06/2018 14:28, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-06-28 14:21:06)
On 28/06/2018 14:11, Chris Wilson wrote:
+/*
+ * Check the unread Context Status Buffers and manage the submission of new
+ * contexts to the ELSP accordingly.
+ */
+static void execlists_submission_tasklet(unsigned long data)
+{
+ struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
+ unsigned long flags;
+
+ GEM_TRACE("%s awake?=%d, active=%x\n",
+ engine->name,
+ engine->i915->gt.awake,
+ engine->execlists.active);
+
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+
+ if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
No races between the check and tasklet call? In other words the code
path which you were describing that can race is taking the timeline lock?
intel_engine_is_idle() is unserialised.
Okay, think I understand. Ship it!
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
+ __execlists_submission_tasklet(engine);
+
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+}
+
static void queue_request(struct intel_engine_cs *engine,
struct i915_sched_node *node,
int prio)
@@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
&lookup_priolist(engine, prio)->requests);
}
-static void __submit_queue(struct intel_engine_cs *engine, int prio)
+static void __update_queue(struct intel_engine_cs *engine, int prio)
{
engine->execlists.queue_priority = prio;
- tasklet_hi_schedule(&engine->execlists.tasklet);
+}
+
+static void __submit_queue_imm(struct intel_engine_cs *engine)
+{
+ struct intel_engine_execlists * const execlists = &engine->execlists;
+
+ if (reset_in_progress(execlists))
+ return; /* defer until we restart the engine following reset */
We have a tasklet kick somewhere in that path?
In execlists_reset_finish()
+ if (execlists->tasklet.func == execlists_submission_tasklet)
+ __execlists_submission_tasklet(engine);
+ else
+ tasklet_hi_schedule(&execlists->tasklet);
}
static void submit_queue(struct intel_engine_cs *engine, int prio)
{
- if (prio > engine->execlists.queue_priority)
- __submit_queue(engine, prio);
+ if (prio > engine->execlists.queue_priority) {
+ __update_queue(engine, prio);
+ __submit_queue_imm(engine);
+ }
}
static void execlists_submit_request(struct i915_request *request)
@@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
spin_lock_irqsave(&engine->timeline.lock, flags);
queue_request(engine, &request->sched, rq_prio(request));
- submit_queue(engine, rq_prio(request));
GEM_BUG_ON(!engine->execlists.first);
GEM_BUG_ON(list_empty(&request->sched.link));
+ submit_queue(engine, rq_prio(request));
+
spin_unlock_irqrestore(&engine->timeline.lock, flags);
}
@@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
}
if (prio > engine->execlists.queue_priority &&
- i915_sw_fence_done(&sched_to_request(node)->submit))
- __submit_queue(engine, prio);
+ i915_sw_fence_done(&sched_to_request(node)->submit)) {
+ /* defer submission until after all of our updates */
+ __update_queue(engine, prio);
+ tasklet_hi_schedule(&engine->execlists.tasklet);
_imm suffix on __submit_queue_imm sounds unused if there is no plain
__submit_queue, which could have been used here. But meh.
I thought of trying to emphasis the immediate nature of this path. It's
not a huge deal, but I didn't particularly like calling it
direct_submit_queue() (or just direct_submit(), too many submits!)
__ prefix to indicate that it's an inner function to submit_queue,
_imm suffix to indicate what's special.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
