diff mbox series

[19/57] drm/i915: Fix the iterative dfs for defering requests

Message ID 20210201085715.27435-19-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/57] drm/i915/gt: Restrict the GT clock override to just Icelake | expand

Commit Message

Chris Wilson Feb. 1, 2021, 8:56 a.m. UTC 
The current implementation of walking the children of a deferred
requests lacks the backtracking required to reduce the dfs to linear.
Having pulled it from execlists into the common layer, we can reuse the
dfs code for priority inheritance.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 56 +++++++++++++++++++--------
 1 file changed, 40 insertions(+), 16 deletions(-)

Comments

Tvrtko Ursulin Feb. 2, 2021, 2:10 p.m. UTC | #1 
On 01/02/2021 08:56, Chris Wilson wrote:
> The current implementation of walking the children of a deferred
> requests lacks the backtracking required to reduce the dfs to linear.
> Having pulled it from execlists into the common layer, we can reuse the
> dfs code for priority inheritance.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_scheduler.c | 56 +++++++++++++++++++--------
>   1 file changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index bfd37ee801fd..694ca3a3b563 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -466,8 +466,10 @@ void i915_request_set_priority(struct i915_request *rq, int prio)
>   void __i915_sched_defer_request(struct intel_engine_cs *engine,
>   				struct i915_request *rq)
>   {
> -	struct list_head *pl;
> -	LIST_HEAD(list);
> +	struct list_head *pos = &rq->sched.waiters_list;
> +	const int prio = rq_prio(rq);
> +	struct i915_request *rn;
> +	LIST_HEAD(dfs);
>   
>   	lockdep_assert_held(&engine->active.lock);
>   	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags));
> @@ -477,14 +479,11 @@ void __i915_sched_defer_request(struct intel_engine_cs *engine,
>   	 * to those that are waiting upon it. So we traverse its chain of
>   	 * waiters and move any that are earlier than the request to after it.
>   	 */
> -	pl = lookup_priolist(engine, rq_prio(rq));
> +	rq->sched.dfs.prev = NULL;
>   	do {
> -		struct i915_dependency *p;
> -
> -		GEM_BUG_ON(i915_request_is_active(rq));
> -		list_move_tail(&rq->sched.link, pl);
> -
> -		for_each_waiter(p, rq) {
> +		list_for_each_continue(pos, &rq->sched.waiters_list) {
> +			struct i915_dependency *p =
> +				list_entry(pos, typeof(*p), wait_link);
>   			struct i915_request *w =
>   				container_of(p->waiter, typeof(*w), sched);
>   
> @@ -500,19 +499,44 @@ void __i915_sched_defer_request(struct intel_engine_cs *engine,
>   				   __i915_request_has_started(w) &&
>   				   !__i915_request_is_complete(rq));
>   
> -			if (!i915_request_is_ready(w))
> +			if (!i915_request_in_priority_queue(w))
>   				continue;
>   
> -			if (rq_prio(w) < rq_prio(rq))
> +			/*
> +			 * We also need to reorder within the same priority.
> +			 *
> +			 * This is unlike priority-inheritance, where if the
> +			 * signaler already has a higher priority [earlier
> +			 * deadline] than us, we can ignore as it will be
> +			 * scheduled first. If a waiter already has the
> +			 * same priority, we still have to push it to the end
> +			 * of the list. This unfortunately means we cannot
> +			 * use the rq_deadline() itself as a 'visited' bit.

rq_deadline only appears later but never mind.

> +			 */
> +			if (rq_prio(w) < prio)
>   				continue;
>   
> -			GEM_BUG_ON(rq_prio(w) > rq_prio(rq));
> -			GEM_BUG_ON(i915_request_is_active(w));
> -			list_move_tail(&w->sched.link, &list);
> +			GEM_BUG_ON(rq_prio(w) != prio);
> +
> +			/* Remember our position along this branch */
> +			rq = stack_push(w, rq, pos);
> +			pos = &rq->sched.waiters_list;
>   		}
>   
> -		rq = list_first_entry_or_null(&list, typeof(*rq), sched.link);
> -	} while (rq);
> +		/* Note list is reversed for waiters wrt signal hierarchy */
> +		GEM_BUG_ON(rq->engine != engine);
> +		GEM_BUG_ON(!i915_request_in_priority_queue(rq));
> +		list_move(&rq->sched.link, &dfs);
> +
> +		/* Track our visit, and prevent duplicate processing */
> +		clear_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> +	} while ((rq = stack_pop(rq, &pos)));
> +
> +	pos = lookup_priolist(engine, prio);
> +	list_for_each_entry_safe(rq, rn, &dfs, sched.link) {
> +		set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> +		list_add_tail(&rq->sched.link, pos);
> +	}
>   }
>   
>   static void queue_request(struct intel_engine_cs *engine,
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index bfd37ee801fd..694ca3a3b563 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -466,8 +466,10 @@  void i915_request_set_priority(struct i915_request *rq, int prio)
 void __i915_sched_defer_request(struct intel_engine_cs *engine,
 				struct i915_request *rq)
 {
-	struct list_head *pl;
-	LIST_HEAD(list);
+	struct list_head *pos = &rq->sched.waiters_list;
+	const int prio = rq_prio(rq);
+	struct i915_request *rn;
+	LIST_HEAD(dfs);
 
 	lockdep_assert_held(&engine->active.lock);
 	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags));
@@ -477,14 +479,11 @@  void __i915_sched_defer_request(struct intel_engine_cs *engine,
 	 * to those that are waiting upon it. So we traverse its chain of
 	 * waiters and move any that are earlier than the request to after it.
 	 */
-	pl = lookup_priolist(engine, rq_prio(rq));
+	rq->sched.dfs.prev = NULL;
 	do {
-		struct i915_dependency *p;
-
-		GEM_BUG_ON(i915_request_is_active(rq));
-		list_move_tail(&rq->sched.link, pl);
-
-		for_each_waiter(p, rq) {
+		list_for_each_continue(pos, &rq->sched.waiters_list) {
+			struct i915_dependency *p =
+				list_entry(pos, typeof(*p), wait_link);
 			struct i915_request *w =
 				container_of(p->waiter, typeof(*w), sched);
 
@@ -500,19 +499,44 @@  void __i915_sched_defer_request(struct intel_engine_cs *engine,
 				   __i915_request_has_started(w) &&
 				   !__i915_request_is_complete(rq));
 
-			if (!i915_request_is_ready(w))
+			if (!i915_request_in_priority_queue(w))
 				continue;
 
-			if (rq_prio(w) < rq_prio(rq))
+			/*
+			 * We also need to reorder within the same priority.
+			 *
+			 * This is unlike priority-inheritance, where if the
+			 * signaler already has a higher priority [earlier
+			 * deadline] than us, we can ignore as it will be
+			 * scheduled first. If a waiter already has the
+			 * same priority, we still have to push it to the end
+			 * of the list. This unfortunately means we cannot
+			 * use the rq_deadline() itself as a 'visited' bit.
+			 */
+			if (rq_prio(w) < prio)
 				continue;
 
-			GEM_BUG_ON(rq_prio(w) > rq_prio(rq));
-			GEM_BUG_ON(i915_request_is_active(w));
-			list_move_tail(&w->sched.link, &list);
+			GEM_BUG_ON(rq_prio(w) != prio);
+
+			/* Remember our position along this branch */
+			rq = stack_push(w, rq, pos);
+			pos = &rq->sched.waiters_list;
 		}
 
-		rq = list_first_entry_or_null(&list, typeof(*rq), sched.link);
-	} while (rq);
+		/* Note list is reversed for waiters wrt signal hierarchy */
+		GEM_BUG_ON(rq->engine != engine);
+		GEM_BUG_ON(!i915_request_in_priority_queue(rq));
+		list_move(&rq->sched.link, &dfs);
+
+		/* Track our visit, and prevent duplicate processing */
+		clear_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
+	} while ((rq = stack_pop(rq, &pos)));
+
+	pos = lookup_priolist(engine, prio);
+	list_for_each_entry_safe(rq, rn, &dfs, sched.link) {
+		set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
+		list_add_tail(&rq->sched.link, pos);
+	}
 }
 
 static void queue_request(struct intel_engine_cs *engine,