Re: [PATCH v7 26/26] drm/i915/gt: Add a spinlock to prevent starvation of irq_work.

[Maarten Lankhorst <dev@lankhorst.se>]

Hey,

Den 2026-03-10 kl. 18:04, skrev Sebastian Andrzej Siewior:
> On 2026-03-10 12:57:08 [+0100], Maarten Lankhorst wrote:
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>
>> IRQ-Work (FIFO-1) will be preempted by the threaded-interrupt (FIFO-50)
>> and the interrupt will poll on signaler_active while the irq-work can't
>> make progress.
> 
> The threaded-interrupt is the interrupt.
> 
> | On PREEMPT_RT the irq_work can be preempted by threaded-interrupt which
> | will be poll for completion but the irq_work routine can't make
> | progress.
> 
>> Solve this by adding a spinlock to prevent starvation and force
>> completion.
> 
> | Solve this by adding a spinlock_t to prevent starvation by forcing a
> | context switch if lock is held based on `signaler_active'. On
> | !PREEMPT_RT `signaler_active' can only be non-zero if multiple CPUs are
> | involved and spinning on the lock leds to the same result.
> 
>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> 
> If I am the From: then I should have the Signed-off-by, too. Let me do
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> so it can be picked up.
> 
> You did suggest the following:
> 
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -209,7 +209,7 @@ static void signal_irq_work(struct irq_work *work)
>  		intel_breadcrumbs_disarm_irq(b);
>  
>  	rcu_read_lock();
> -	atomic_inc(&b->signaler_active);
> +	spin_lock(&b->signaler_active_sync);
>  	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
>  		struct i915_request *rq;
>  
> @@ -245,7 +245,7 @@ static void signal_irq_work(struct irq_work *work)
>  				i915_request_put(rq);
>  		}
>  	}
> -	atomic_dec(&b->signaler_active);
> +	spin_unlock(&b->signaler_active_sync);
>  	rcu_read_unlock();
>  
>  	llist_for_each_safe(signal, sn, signal) {
> @@ -290,6 +290,7 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>  	init_llist_head(&b->signaled_requests);
>  
>  	spin_lock_init(&b->irq_lock);
> +	spin_lock_init(&b->signaler_active_sync);
>  	init_irq_work(&b->irq_work, signal_irq_work);
>  
>  	b->irq_engine = irq_engine;
> @@ -487,8 +488,10 @@ void intel_context_remove_breadcrumbs(struct intel_context *ce,
>  	if (release)
>  		intel_context_put(ce);
>  
> -	while (atomic_read(&b->signaler_active))
> -		cpu_relax();
> +	while (spin_is_locked(&b->signaler_active_sync)) {
> +		spin_lock_irqsave(&b->signaler_active_sync, flags);
> +		spin_unlock_irqrestore(&b->signaler_active_sync, flags);
> 
> And this does not work because spin_is_locked() returns true and spins
> forever. This fails because there is a "corner case" where
> spin_is_locked() returns but the lock has no lock owner as in locked.
> This happens if there is a waiter which did not yet acquire the lock. 
> 
> So if you happy with this, we could keep it ;)

It seems CI is a lot happier too now.

Xe:
https://patchwork.freedesktop.org/series/159034/#rev14

BAT passes, the full run has some minor issues but only kms_vblank systematic.
Likely due to the fundamental changes of the PREEMPT_RT kernel itself, nothing driver specific.

i915:
https://patchwork.freedesktop.org/series/159035/#rev14

A few new warnings, and some noise. The most worrying part is the i915 execlists selftest
failing on nearly all platforms:

<3>[  504.279024] i915/intel_execlists_live_selftests: live_preempt_user failed with error -62
...
<7>[  506.799685] [IGT] i915_selftest: finished subtest execlists, FAIL

I don't know what's going on there yet, likely needs more debugging. Could be the test itself
being written incorrectly or something else entirely.

Otherwise things are looking good! Have you uncovered anything else?

Kind regards,
~Maarten Lankhorst