Re: [Intel-gfx] [PATCH 07/20] drm/i915/gt: Shrink the critical section for irq signaling

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Dec 07, 2020 at 07:38:11PM +0000, Chris Wilson wrote:
> Let's only wait for the list iterator when decoupling the virtual
> breadcrumb, as the signaling of all the requests may take a long time,
> during which we do not want to keep the tasklet spinning.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_breadcrumbs.c       | 2 ++
>  drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h | 1 +
>  drivers/gpu/drm/i915/gt/intel_lrc.c               | 3 ++-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 63900edbde88..ac1e5f6c3c2c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -239,6 +239,7 @@ static void signal_irq_work(struct irq_work *work)
>  		intel_breadcrumbs_disarm_irq(b);
>  
>  	rcu_read_lock();
> +	atomic_inc(&b->signaler_active);
>  	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
>  		struct i915_request *rq;
>  
> @@ -274,6 +275,7 @@ static void signal_irq_work(struct irq_work *work)
>  			}
>  		}
>  	}
> +	atomic_dec(&b->signaler_active);
>  	rcu_read_unlock();
>  
>  	llist_for_each_safe(signal, sn, signal) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> index a74bb3062bd8..f672053d694d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> @@ -35,6 +35,7 @@ struct intel_breadcrumbs {
>  	spinlock_t signalers_lock; /* protects the list of signalers */
>  	struct list_head signalers;
>  	struct llist_head signaled_requests;
> +	atomic_t signaler_active;
>  
>  	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
>  	struct irq_work irq_work; /* for use from inside irq_lock */
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index b3db16b2a5a4..35cded25c6c1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1401,7 +1401,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
>  		 * ce->signal_link.
>  		 */
>  		i915_request_cancel_breadcrumb(rq);
> -		irq_work_sync(&engine->breadcrumbs->irq_work);
> +		while (atomic_read(&engine->breadcrumbs->signaler_active))
> +			cpu_relax();

Would a 'cond_resched' be better here? I trust your opinion on which to
use but thought I'd mention it.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  	}
>  
>  	if (READ_ONCE(ve->request))
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx