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

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Chris Wilson (2018-02-06 09:46:33)
> 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. 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+

Any takers for this brown paper bug?

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index efbc627a2a25..b955f7d7bd0f 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -588,29 +588,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)
> @@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>                                       &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 */
>                         }
>  
> -- 
> 2.15.1
>