[Intel-gfx] [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Tvrtko Ursulin]

On 08/12/2017 12:10, Chris Wilson wrote:
> The intent here was that we would be listening to
> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> release the reference on the request. Cancelling the signaler is done
> directly via intel_engine_cancel_signaling (called from unsubmit), but
> that does not directly wake up the signaling thread, and neither does
> setting the request->global_seqno back to zero wake up listeners to the
> request->execute waitqueue. So the only time that listening to the
> request->execute waitqueue would wake up the signaling kthread would be
> on the request resubmission, during which time we would already receive
> wake ups from rejoining the global breadcrumbs wait rbtree.
> 
> Trying to wake up to release the request remains an issue. If the
> signaling was cancelled and no other request required signaling, then it
> is possible for us to shutdown with the reference on the request still
> held. To ensure that we do not try to shutdown, leaking that request, we
> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> parking the engine when idle.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 5ae2d276f7f3..b826725989b1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	struct intel_wait *wait, *n, *first;
>   
>   	if (!b->irq_armed)
> -		return;
> +		goto wakeup_signaler;
>   
>   	/* We only disarm the irq when we are idle (all requests completed),
>   	 * so if the bottom-half remains asleep, it missed the request
> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	b->waiters = RB_ROOT;
>   
>   	spin_unlock_irq(&b->rb_lock);
> +
> +	/*
> +	 * The signaling thread may be asleep holding a reference to a request,
> +	 * that had its signaling cancelled prior to being preempted. We need
> +	 * to kick the signaler, just in case, to release any such reference.
> +	 */
> +wakeup_signaler:
> +	wake_up_process(b->signaler);
>   }
>   
>   static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
>   		}
>   
>   		if (unlikely(do_schedule)) {
> -			DEFINE_WAIT(exec);
> -
>   			if (kthread_should_park())
>   				kthread_parkme();
>   
> @@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>   				break;
>   			}
>   
> -			if (request)
> -				add_wait_queue(&request->execute, &exec);
> -
>   			schedule();
> -
> -			if (request)
> -				remove_wait_queue(&request->execute, &exec);
>   		}
>   		i915_gem_request_put(request);
>   	} while (1);
> 

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

Regards,

Tvrtko