* [PATCH 0/2] drm/i915/gt: Locking splats PREEMPT_RT @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams Clark Williams reported two issues with the i915 driver running on PREEMPT_RT. While #1 looks simple I have no idea about #2 thus the RFC. Sebastian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-gfx] [PATCH 0/2] drm/i915/gt: Locking splats PREEMPT_RT @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams Clark Williams reported two issues with the i915 driver running on PREEMPT_RT. While #1 looks simple I have no idea about #2 thus the RFC. Sebastian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] drm/i915/gt: Queue and wait for the irq_work item. 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior -1 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams, Sebastian Andrzej Siewior Disabling interrupts and invoking the irq_work function directly breaks on PREEMPT_RT. PREEMPT_RT does not invoke all irq_work from hardirq context because some of the user have spinlock_t locking in the callback function. These locks are then turned into a sleeping locks which can not be acquired with disabled interrupts. Using irq_work_queue() has the benefit that the irqwork will be invoked in the regular context. In general there is "no" delay between enqueuing the callback and its invocation because the interrupt is raised right away on architectures which support it (which includes x86). Use irq_work_queue() + irq_work_sync() instead invoking the callback directly. Reported-by: Clark Williams <williams@redhat.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c index 38cc42783dfb2..594dec2f76954 100644 --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c @@ -318,10 +318,9 @@ void __intel_breadcrumbs_park(struct intel_breadcrumbs *b) /* Kick the work once more to drain the signalers, and disarm the irq */ irq_work_sync(&b->irq_work); while (READ_ONCE(b->irq_armed) && !atomic_read(&b->active)) { - local_irq_disable(); - signal_irq_work(&b->irq_work); - local_irq_enable(); + irq_work_queue(&b->irq_work); cond_resched(); + irq_work_sync(&b->irq_work); } } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Intel-gfx] [PATCH 1/2] drm/i915/gt: Queue and wait for the irq_work item. @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams, Sebastian Andrzej Siewior Disabling interrupts and invoking the irq_work function directly breaks on PREEMPT_RT. PREEMPT_RT does not invoke all irq_work from hardirq context because some of the user have spinlock_t locking in the callback function. These locks are then turned into a sleeping locks which can not be acquired with disabled interrupts. Using irq_work_queue() has the benefit that the irqwork will be invoked in the regular context. In general there is "no" delay between enqueuing the callback and its invocation because the interrupt is raised right away on architectures which support it (which includes x86). Use irq_work_queue() + irq_work_sync() instead invoking the callback directly. Reported-by: Clark Williams <williams@redhat.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c index 38cc42783dfb2..594dec2f76954 100644 --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c @@ -318,10 +318,9 @@ void __intel_breadcrumbs_park(struct intel_breadcrumbs *b) /* Kick the work once more to drain the signalers, and disarm the irq */ irq_work_sync(&b->irq_work); while (READ_ONCE(b->irq_armed) && !atomic_read(&b->active)) { - local_irq_disable(); - signal_irq_work(&b->irq_work); - local_irq_enable(); + irq_work_queue(&b->irq_work); cond_resched(); + irq_work_sync(&b->irq_work); } } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior -1 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams, 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 <williams@redhat.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@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); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Intel-gfx] [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() @ 2021-09-08 18:57 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-09-08 18:57 UTC (permalink / raw) To: dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams, 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 <williams@redhat.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@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); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior (?) @ 2021-09-16 9:38 ` Maarten Lankhorst 2021-10-01 9:29 ` Sebastian Andrzej Siewior -1 siblings, 1 reply; 9+ messages in thread From: Maarten Lankhorst @ 2021-09-16 9:38 UTC (permalink / raw) To: Sebastian Andrzej Siewior, dri-devel, intel-gfx Cc: David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams 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 <williams@redhat.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@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.lankhorst@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() 2021-09-16 9:38 ` Maarten Lankhorst @ 2021-10-01 9:29 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Andrzej Siewior @ 2021-10-01 9:29 UTC (permalink / raw) To: Maarten Lankhorst Cc: dri-devel, intel-gfx, David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Chris Wilson, Peter Zijlstra, Thomas Gleixner, Clark Williams, Anton Lundin On 2021-09-16 11:38:55 [+0200], Maarten Lankhorst wrote: > Patches look good. Thank you for looking. > For both patches: > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@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. yes, let me prepare something in a few. > 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. Let me see. So Anton Lundin (Cc:) reported glitches due to _this_ patch on -RT. I have just a Sandybridge around with a i915 and it does not get near that code here. > ~Maarten Sebastian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gt: Locking splats PREEMPT_RT 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior ` (2 preceding siblings ...) (?) @ 2021-09-08 19:25 ` Patchwork -1 siblings, 0 replies; 9+ messages in thread From: Patchwork @ 2021-09-08 19:25 UTC (permalink / raw) To: Sebastian Andrzej Siewior; +Cc: intel-gfx == Series Details == Series: drm/i915/gt: Locking splats PREEMPT_RT URL : https://patchwork.freedesktop.org/series/94480/ State : failure == Summary == Applying: drm/i915/gt: Queue and wait for the irq_work item. Applying: drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/gt/intel_execlists_submission.c Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/gt/intel_execlists_submission.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gt/intel_execlists_submission.c error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0002 drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-01 9:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-08 18:57 [PATCH 0/2] drm/i915/gt: Locking splats PREEMPT_RT Sebastian Andrzej Siewior 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior 2021-09-08 18:57 ` [PATCH 1/2] drm/i915/gt: Queue and wait for the irq_work item Sebastian Andrzej Siewior 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior 2021-09-08 18:57 ` [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock() Sebastian Andrzej Siewior 2021-09-08 18:57 ` [Intel-gfx] " Sebastian Andrzej Siewior 2021-09-16 9:38 ` Maarten Lankhorst 2021-10-01 9:29 ` Sebastian Andrzej Siewior 2021-09-08 19:25 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gt: Locking splats PREEMPT_RT Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.