Op 08-09-2021 om 20:57 schreef Sebastian Andrzej Siewior:
> execlists_dequeue() is invoked from a function which uses
> local_irq_disable() to disable interrupts so the spin_lock() behaves
> like spin_lock_irq().
> This breaks PREEMPT_RT because local_irq_disable() + spin_lock() is not
> the same as spin_lock_irq().
>
> execlists_dequeue_irq() and execlists_dequeue() has each one caller
> only. If intel_engine_cs::active::lock is acquired and released with the
> _irq suffix then it behaves almost as if execlists_dequeue() would be
> invoked with disabled interrupts. The difference is the last part of the
> function which is then invoked with enabled interrupts.
> I can't tell if this makes a difference. From looking at it, it might
> work to move the last unlock at the end of the function as I didn't find
> anything that would acquire the lock again.
>
> Reported-by: Clark Williams <willi...@redhat.com>
> Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
> ---
> .../drm/i915/gt/intel_execlists_submission.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index fc77592d88a96..2ec1dd352960b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -1265,7 +1265,7 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
> * and context switches) submission.
> */
>
> - spin_lock(&engine->active.lock);
> + spin_lock_irq(&engine->active.lock);
>
> /*
> * If the queue is higher priority than the last
> @@ -1365,7 +1365,7 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
> * Even if ELSP[1] is occupied and not worthy
> * of timeslices, our queue might be.
> */
> - spin_unlock(&engine->active.lock);
> + spin_unlock_irq(&engine->active.lock);
> return;
> }
> }
> @@ -1391,7 +1391,7 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
>
> if (last && !can_merge_rq(last, rq)) {
> spin_unlock(&ve->base.active.lock);
> - spin_unlock(&engine->active.lock);
> + spin_unlock_irq(&engine->active.lock);
> return; /* leave this for another sibling */
> }
>
> @@ -1552,7 +1552,7 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
> * interrupt for secondary ports).
> */
> execlists->queue_priority_hint = queue_prio(execlists);
> - spin_unlock(&engine->active.lock);
> + spin_unlock_irq(&engine->active.lock);
>
> /*
> * We can skip poking the HW if we ended up with exactly the same set
> @@ -1578,13 +1578,6 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
> }
> }
>
> -static void execlists_dequeue_irq(struct intel_engine_cs *engine)
> -{
> - local_irq_disable(); /* Suspend interrupts across request submission */
> - execlists_dequeue(engine);
> - local_irq_enable(); /* flush irq_work (e.g. breadcrumb enabling) */
> -}
> -
> static void clear_ports(struct i915_request **ports, int count)
> {
> memset_p((void **)ports, NULL, count);
> @@ -2377,7 +2370,7 @@ static void execlists_submission_tasklet(struct
> tasklet_struct *t)
> }
>
> if (!engine->execlists.pending[0]) {
> - execlists_dequeue_irq(engine);
> + execlists_dequeue(engine);
> start_timeslice(engine);
> }
>
Patches look good.
For both patches:
Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
I've been looking at running i915 with the -rt patch series, and noticed
i915_request_submit fails with GEM_BUG_ON(!irqs_disabled()); presumably same
failure exists for i915_request_unsubmit().
Might be worth removing those checks as well? Seems double with
lockdep_assert_held on an irq lock anyway.
I've also noticed the local_irq_disable/enable is removed from
intel_pipe_update_(start/end) in the rt series. It might make sense from a -rt
point of view, but that code needs to run without interruptions, or i915 may
show visual glitches or even locks up the system.
It should just be a set of registers hammered in, but the code might needs to
be fixed to take the mmio lock as outer lock, and become a strict set of
register read/writes only.
~Maarten
