Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
Date: Tue, 13 Feb 2018 17:00:07 -0800	[thread overview]
Message-ID: <20180214010006.mjldahs3jwtuaw6z@intel.com> (raw)
In-Reply-To: <20180213090154.17373-1-chris@chris-wilson.co.uk>

On Tue, Feb 13, 2018 at 09:01:54AM +0000, Chris Wilson wrote:
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno, or it may match the cleared seqno with the
> now zero rq->global_seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20180206094633.30181-1-chris@chris-wilson.co.uk
> (cherry picked from commit fd10e2ce9905030d922e179a8047a4d50daffd8e)

applied to fixes. Thanks

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index bd40fea16b4f..f54ddda9fdad 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -594,29 +594,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	spin_unlock_irq(&b->rb_lock);
>  }
>  
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> -	return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
>  static bool signal_complete(const struct drm_i915_gem_request *request)
>  {
>  	if (!request)
>  		return false;
>  
> -	/* If another process served as the bottom-half it may have already
> -	 * signalled that this wait is already completed.
> -	 */
> -	if (intel_wait_complete(&request->signaling.wait))
> -		return signal_valid(request);
> -
> -	/* Carefully check if the request is complete, giving time for the
> +	/*
> +	 * Carefully check if the request is complete, giving time for the
>  	 * seqno to be visible or if the GPU hung.
>  	 */
> -	if (__i915_request_irq_complete(request))
> -		return true;
> -
> -	return false;
> +	return __i915_request_irq_complete(request);
>  }
>  
>  static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -659,9 +646,13 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			request = i915_gem_request_get_rcu(request);
>  		rcu_read_unlock();
>  		if (signal_complete(request)) {
> -			local_bh_disable();
> -			dma_fence_signal(&request->fence);
> -			local_bh_enable(); /* kick start the tasklets */
> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				      &request->fence.flags)) {
> +				local_bh_disable();
> +				dma_fence_signal(&request->fence);
> +				GEM_BUG_ON(!i915_gem_request_completed(request));
> +				local_bh_enable(); /* kick start the tasklets */
> +			}
>  
>  			spin_lock_irq(&b->rb_lock);
>  
> -- 
> 2.16.1
> 