| Message ID | 20210819061639.21051-4-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up GuC CI failures, simplify locking, and kernel DOC | expand |
On Thu, Aug 19, 2021 at 04:54:00PM -0700, Daniele Ceraolo Spurio wrote: > > > On 8/18/2021 11:16 PM, Matthew Brost wrote: > > When unwinding requests on a reset context, if other requests in the > > context are in the priority list the requests could be resubmitted out > > of seqno order. Traverse the list of active requests in reverse and > > append to the head of the priority list to fix this. > > > > Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > Cc: <stable@vger.kernel.org> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index 32c414aa9009..9ca0ba4ea85a 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -805,9 +805,9 @@ __unwind_incomplete_requests(struct intel_context *ce) > > spin_lock_irqsave(&sched_engine->lock, flags); > > spin_lock(&ce->guc_active.lock); > > - list_for_each_entry_safe(rq, rn, > > - &ce->guc_active.requests, > > - sched.link) { > > + list_for_each_entry_safe_reverse(rq, rn, > > + &ce->guc_active.requests, > > + sched.link) { > > if (i915_request_completed(rq)) > > The execlists unwind function has a list_del if the request is completed. > Any reason not to do that here? > Def isn't needed here as this is done in remove_from_context(), probably not needed in execlists mode either. > > continue; > > @@ -824,7 +824,7 @@ __unwind_incomplete_requests(struct intel_context *ce) > > } > > GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); > > - list_add_tail(&rq->sched.link, pl); > > + list_add(&rq->sched.link, pl); > > Since you always do both list_del and list_add and it doesn't look like you > use the fact that the list is empty between the 2 calls, you can merge them > in a list_move. > Can't use a list move here because we drop spin_lock(&ce->guc_active.lock), that gets fixed later in the series and at that point we likely can use a list_move. Matt > Apart from these nits, the change to navigate the list in reverse and append > here at the top LGTM. > > Daniele > > > set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); > > spin_lock(&ce->guc_active.lock); >
On 8/18/2021 11:16 PM, Matthew Brost wrote: > When unwinding requests on a reset context, if other requests in the > context are in the priority list the requests could be resubmitted out > of seqno order. Traverse the list of active requests in reverse and > append to the head of the priority list to fix this. > > Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > Cc: <stable@vger.kernel.org> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index 32c414aa9009..9ca0ba4ea85a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -805,9 +805,9 @@ __unwind_incomplete_requests(struct intel_context *ce) > > spin_lock_irqsave(&sched_engine->lock, flags); > spin_lock(&ce->guc_active.lock); > - list_for_each_entry_safe(rq, rn, > - &ce->guc_active.requests, > - sched.link) { > + list_for_each_entry_safe_reverse(rq, rn, > + &ce->guc_active.requests, > + sched.link) { > if (i915_request_completed(rq)) The execlists unwind function has a list_del if the request is completed. Any reason not to do that here? > continue; > > @@ -824,7 +824,7 @@ __unwind_incomplete_requests(struct intel_context *ce) > } > GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); > > - list_add_tail(&rq->sched.link, pl); > + list_add(&rq->sched.link, pl); Since you always do both list_del and list_add and it doesn't look like you use the fact that the list is empty between the 2 calls, you can merge them in a list_move. Apart from these nits, the change to navigate the list in reverse and append here at the top LGTM. Daniele > set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); > > spin_lock(&ce->guc_active.lock);
On 8/19/2021 4:53 PM, Matthew Brost wrote: > On Thu, Aug 19, 2021 at 04:54:00PM -0700, Daniele Ceraolo Spurio wrote: >> >> On 8/18/2021 11:16 PM, Matthew Brost wrote: >>> When unwinding requests on a reset context, if other requests in the >>> context are in the priority list the requests could be resubmitted out >>> of seqno order. Traverse the list of active requests in reverse and >>> append to the head of the priority list to fix this. >>> >>> Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") >>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> index 32c414aa9009..9ca0ba4ea85a 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> @@ -805,9 +805,9 @@ __unwind_incomplete_requests(struct intel_context *ce) >>> spin_lock_irqsave(&sched_engine->lock, flags); >>> spin_lock(&ce->guc_active.lock); >>> - list_for_each_entry_safe(rq, rn, >>> - &ce->guc_active.requests, >>> - sched.link) { >>> + list_for_each_entry_safe_reverse(rq, rn, >>> + &ce->guc_active.requests, >>> + sched.link) { >>> if (i915_request_completed(rq)) >> The execlists unwind function has a list_del if the request is completed. >> Any reason not to do that here? >> > Def isn't needed here as this is done in remove_from_context(), probably > not needed in execlists mode either. > > >>> continue; >>> @@ -824,7 +824,7 @@ __unwind_incomplete_requests(struct intel_context *ce) >>> } >>> GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); >>> - list_add_tail(&rq->sched.link, pl); >>> + list_add(&rq->sched.link, pl); >> Since you always do both list_del and list_add and it doesn't look like you >> use the fact that the list is empty between the 2 calls, you can merge them >> in a list_move. >> > Can't use a list move here because we drop > spin_lock(&ce->guc_active.lock), that gets fixed later in the series and > at that point we likely can use a list_move. fair enough. I'll leave it to you to decide if it is worth moving this patch after the next one and using a list_move. With or without that, this is: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > > Matt > >> Apart from these nits, the change to navigate the list in reverse and append >> here at the top LGTM. >> >> Daniele >> >>> set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); >>> spin_lock(&ce->guc_active.lock);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 32c414aa9009..9ca0ba4ea85a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -805,9 +805,9 @@ __unwind_incomplete_requests(struct intel_context *ce) spin_lock_irqsave(&sched_engine->lock, flags); spin_lock(&ce->guc_active.lock); - list_for_each_entry_safe(rq, rn, - &ce->guc_active.requests, - sched.link) { + list_for_each_entry_safe_reverse(rq, rn, + &ce->guc_active.requests, + sched.link) { if (i915_request_completed(rq)) continue; @@ -824,7 +824,7 @@ __unwind_incomplete_requests(struct intel_context *ce) } GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); - list_add_tail(&rq->sched.link, pl); + list_add(&rq->sched.link, pl); set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); spin_lock(&ce->guc_active.lock);
When unwinding requests on a reset context, if other requests in the context are in the priority list the requests could be resubmitted out of seqno order. Traverse the list of active requests in reverse and append to the head of the priority list to fix this. Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") Signed-off-by: Matthew Brost <matthew.brost@intel.com> Cc: <stable@vger.kernel.org> --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)