Re: [PATCH v2 1/2] drm/i915/guc: Complete all requests of a non-schedulable context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915/guc: out-of-order request completion in
  __guc_add_request()
--

commit ab2fec244e71c5f1800fda0182eb817d04ef826f
Author: Javier MV <javiermv@duck.com>

drm/i915/guc: Complete all requests of a non-schedulable context

When submitting a request for a non-schedulable context, this commit marks
all requests on ce->guc_state.requests complete with -EIO once the context
is off the hardware.

Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15301

> 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 788e59cdfac9b..b2ffc6b4614e4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -709,7 +709,25 @@ static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>  	 * request resubmitted after the context was banned.
>  	 */
>  	if (unlikely(!intel_context_is_schedulable(ce))) {
> -		i915_request_put(i915_request_mark_eio(rq));
> +		struct i915_request *pos;
> +
> +		/*
> +		 * The dequeue only passes the last of the merged requests, and
> +		 * the context won't run again to write the others' breadcrumbs:
> +		 * complete them all (rq is on the list too), once the context
> +		 * is off the HW. Don't touch rq after the unlock: once complete
> +		 * it can be retired and freed.
> +		 */
> +		spin_lock(&ce->guc_state.lock);
> +		if (!context_enabled(ce) && !context_pending_enable(ce) &&
> +		    !context_pending_disable(ce)) {
> +			list_for_each_entry(pos, &ce->guc_state.requests, sched.link)
> +				i915_request_put(i915_request_mark_eio(pos));
> +		} else {
> +			i915_request_put(i915_request_mark_eio(rq));
> +		}

[Severity: High]
Does the else branch complete rq out of order ahead of earlier requests on
the same timeline?

Before calling __guc_add_request(), guc_dequeue_one_context() and
guc_bypass_tasklet_submit() move every submitted request onto
ce->guc_state.requests via __i915_request_submit() and pass only the final
request as rq.  When the context is still on the hardware or has an enable
or disable in flight, marking only rq complete leaves earlier merged or
in-flight requests on ce->guc_state.requests and tl->requests incomplete.

Because __i915_active_fence_set() moves active tracking callbacks to the
newest request on the timeline, completing rq early retires vma->active and
ce->active while the GPU is still executing earlier requests on the context,
and trips the check in i915_request_retire_upto():

drivers/gpu/drm/i915/i915_request.c:i915_request_retire_upto() {
	do {
		tmp = list_first_entry(&tl->requests, typeof(*tmp), link);
		GEM_BUG_ON(!i915_request_completed(tmp));
	} while (i915_request_retire(tmp) && tmp != rq);
}

Also, if context_pending_enable(ce) is set while !context_enabled(ce) &&
!context_pending_disable(ce) (for example after __guc_reset_context() clears
SCHED_STATE_ENABLED while an enable G2H is still in flight),
intel_guc_sched_done_process_msg() only clears SCHED_STATE_PENDING_ENABLE
without calling guc_cancel_context_requests(), so the earlier merged
requests are never completed.

Since rq is already on ce->guc_state.requests, should the else branch leave
rq incomplete so guc_cancel_context_requests() completes the whole list in
order once the context is off the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111135753.11071-1-javiermv@gmail.com?part=1