Re: [PATCH] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

On 2026-08-21 13:45:08 [+0200], Maarten Lankhorst wrote:
> 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.
> 
> Solve this by adding a global spinlock to prevent starvation and force
> completion.
> 
> The existing RCU handling gets in the way on PREEMPT_RT, and would likely
> require conversion to raw spinlock to take them inside a
> rcu_read_lock(), so remove RCU as well.
> 
…

This looks straight forward and removes signaler_active, so look like a
win-win situation.

Acked-by: Sebastian Andrzej Siewior <[email protected]>

…
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index c10ac0ab3bfa8..0ae7759dc74a3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -88,24 +88,25 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
>  				  struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->signal_lock);
> +	lockdep_assert_held(&b->signalers_lock);
>  
> -	spin_lock(&b->signalers_lock);
> -	list_add_rcu(&ce->signal_link, &b->signalers);
> -	spin_unlock(&b->signalers_lock);
> +	if (list_empty(&ce->signals))
> +		return;

This list_empty logic here is inherited. Not sure why this list_empty is
here. But the removal has (had) the reverse check here and now gains a
check against double removal. I guess since the logic is mostly the
same, there is no need to check for double add.

> +	intel_context_get(ce);
> +	list_add(&ce->signal_link, &b->signalers);
>  }
>  
>  static bool remove_signaling_context(struct intel_breadcrumbs *b,
>  				     struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->signal_lock);
> +	lockdep_assert_held(&b->signalers_lock);
>  
> -	if (!list_empty(&ce->signals))
> +	if (!list_empty(&ce->signals) || list_empty(&ce->signal_link))
>  		return false;
>  
> -	spin_lock(&b->signalers_lock);
> -	list_del_rcu(&ce->signal_link);
> -	spin_unlock(&b->signalers_lock);
> -
> +	list_del_init(&ce->signal_link);
>  	return true;
>  }
>  

Sebastian