Re: [PATCH 4/5] drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 16/07/2020 12:33, Chris Wilson wrote:
> Now that we have serialised the request retirement's decoupling of the
> breadcrumb from the engine with the request signaling, we know that
> there should never be a stale breadcrumb attached to an unbound virtual
> engine. We can now remove the fixup code that had to migrate the
> breadcrumbs along with the virtual engine, from one sibling to the next.

What do you mean by "unbound virtual engine"? Previous siblings[0]? We 
do know that has been completed, at the point the next one is getting 
dequeued, and by the virtue of breadcrumbs doing the signaling it will 
have been removed from the list. But that was true before. Which leaves 
me confused as to why the transfer was needed.. Was it just because 
explicit wait used to be a potential signaler and that's no longer the case?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c  | 29 --------------------
>   drivers/gpu/drm/i915/gt/intel_engine.h       |  3 --
>   drivers/gpu/drm/i915/gt/intel_engine_types.h |  2 --
>   drivers/gpu/drm/i915/gt/intel_lrc.c          | 15 ----------
>   4 files changed, 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index a0f52417238c..164662ae130b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -167,8 +167,6 @@ static void signal_irq_work(struct irq_work *work)
>   	if (b->irq_armed && list_empty(&b->signalers))
>   		__intel_breadcrumbs_disarm_irq(b);
>   
> -	list_splice_init(&b->signaled_requests, &signal);
> -
>   	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
>   		GEM_BUG_ON(list_empty(&ce->signals));
>   
> @@ -258,7 +256,6 @@ void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>   
>   	spin_lock_init(&b->irq_lock);
>   	INIT_LIST_HEAD(&b->signalers);
> -	INIT_LIST_HEAD(&b->signaled_requests);
>   
>   	init_irq_work(&b->irq_work, signal_irq_work);
>   }
> @@ -278,32 +275,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>   	spin_unlock_irqrestore(&b->irq_lock, flags);
>   }
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce)
> -{
> -	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&b->irq_lock, flags);
> -	if (!list_empty(&ce->signals)) {
> -		struct i915_request *rq, *next;
> -
> -		/* Queue for executing the signal callbacks in the irq_work */
> -		list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
> -			GEM_BUG_ON(rq->engine != engine);
> -			GEM_BUG_ON(!__request_completed(rq));
> -
> -			__signal_request(rq, &b->signaled_requests);
> -		}
> -
> -		INIT_LIST_HEAD(&ce->signals);
> -		list_del_init(&ce->signal_link);
> -
> -		irq_work_queue(&b->irq_work);
> -	}
> -	spin_unlock_irqrestore(&b->irq_lock, flags);
> -}
> -
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>   {
>   }
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index a9249a23903a..faf00a353e25 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
>   void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce);
> -
>   void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
>   				    struct drm_printer *p);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 8de92fd7d392..e0a2ceac729f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -393,8 +393,6 @@ struct intel_engine_cs {
>   		spinlock_t irq_lock;
>   		struct list_head signalers;
>   
> -		struct list_head signaled_requests;
> -
>   		struct irq_work irq_work; /* for use from inside irq_lock */
>   
>   		unsigned int irq_enabled;
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 21c16e31c4fe..88a5c155154d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
>   	return true;
>   }
>   
> -static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
> -{
> -	/*
> -	 * All the outstanding signals on ve->siblings[0] must have
> -	 * been completed, just pending the interrupt handler. As those
> -	 * signals still refer to the old sibling (via rq->engine), we must
> -	 * transfer those to the old irq_worker to keep our locking
> -	 * consistent.
> -	 */
> -	intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
> -}
> -
>   #define for_each_waiter(p__, rq__) \
>   	list_for_each_entry_lockless(p__, \
>   				     &(rq__)->sched.waiters_list, \
> @@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   					virtual_update_register_offsets(regs,
>   									engine);
>   
> -				if (!list_empty(&ve->context.signals))
> -					virtual_xfer_breadcrumbs(ve);
> -
>   				/*
>   				 * Move the bound engine to the top of the list
>   				 * for future execution. We then kick this
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx