From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers Date: Wed, 7 Feb 2018 10:40:46 +0000 Message-ID: References: <20180206094633.30181-1-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180206094633.30181-1-chris@chris-wilson.co.uk> Content-Language: en-GB Sender: stable-owner@vger.kernel.org To: Chris Wilson , intel-gfx@lists.freedesktop.org Cc: Joonas Lahtinen , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On 06/02/2018 09:46, 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. 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 > Cc: Tvrtko Ursulin > Cc: Joonas Lahtinen > Cc: # v4.12+ > --- > 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); Okay so this can return true for unsubmitted requests since rb node will be empty and global_seqno == wait.seqno == 0. I just panic when thinking about races and ordering, since these checks used to run unlocked. So even better that they are gone. > - > - /* 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 */ > } > > Looks OK. But I can't say it's straightforward to understand it. Reviewed-by: Tvrtko Ursulin Regards, Tvrtko