Re: [Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first
On 26/04/2017 11:48, Chris Wilson wrote: On Wed, Apr 26, 2017 at 11:43:03AM +0100, Tvrtko Ursulin wrote: On 26/04/2017 11:15, Chris Wilson wrote: Now that we try to signal the fence from inside the interrupt handler, when we reach the signaler thread, the fence is most likely already signaled. Skip manipulating the bottom-half locks if this is so. Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c index 8f52fd5f6102..5f79c8135b3f 100644 --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c @@ -616,9 +616,12 @@ 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); + local_bh_enable(); /* kick start the tasklet */ + } spin_lock_irq(&b->rb_lock); What are you referring to by "bottom-half locks" in the commit msg? AFAICS it would only skip kicking the tasklets with this change. That may be worth it, I haven't measured, but I don't see a difference wrt any locks. It's just the preempt enable/disable pair plus the function call Okay then, with a corrected commit message: Reviewed-by: Tvrtko Ursulin In fact we could change this to: if (!dma_fence_signal(...)) { local_bh_disable(); local_bh_enable(); } If we wanted to avoid touching the flags directly, but then would have a function call.. Yeah, plus that looks ridiculous ;) Hey, we can always hide it in a wrapper! :)) Regards, Tvrtko The biggest cost in the signaler thread is going to sleep. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first
On Wed, Apr 26, 2017 at 11:43:03AM +0100, Tvrtko Ursulin wrote: > > On 26/04/2017 11:15, Chris Wilson wrote: > >Now that we try to signal the fence from inside the interrupt handler, > >when we reach the signaler thread, the fence is most likely already > >signaled. Skip manipulating the bottom-half locks if this is so. > > > >Signed-off-by: Chris Wilson > >Cc: Tvrtko Ursulin > >--- > > drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 ++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c > >b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >index 8f52fd5f6102..5f79c8135b3f 100644 > >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c > >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >@@ -616,9 +616,12 @@ 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); > >+local_bh_enable(); /* kick start the tasklet */ > >+} > > > > spin_lock_irq(&b->rb_lock); > > > > > > What are you referring to by "bottom-half locks" in the commit msg? > AFAICS it would only skip kicking the tasklets with this change. > That may be worth it, I haven't measured, but I don't see a > difference wrt any locks. It's just the preempt enable/disable pair plus the function call > In fact we could change this to: > > if (!dma_fence_signal(...)) { > local_bh_disable(); > local_bh_enable(); > } > > If we wanted to avoid touching the flags directly, but then would > have a function call.. Yeah, plus that looks ridiculous ;) The biggest cost in the signaler thread is going to sleep. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first
On 26/04/2017 11:15, Chris Wilson wrote: Now that we try to signal the fence from inside the interrupt handler, when we reach the signaler thread, the fence is most likely already signaled. Skip manipulating the bottom-half locks if this is so. Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c index 8f52fd5f6102..5f79c8135b3f 100644 --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c @@ -616,9 +616,12 @@ 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); + local_bh_enable(); /* kick start the tasklet */ + } spin_lock_irq(&b->rb_lock); What are you referring to by "bottom-half locks" in the commit msg? AFAICS it would only skip kicking the tasklets with this change. That may be worth it, I haven't measured, but I don't see a difference wrt any locks. In fact we could change this to: if (!dma_fence_signal(...)) { local_bh_disable(); local_bh_enable(); } If we wanted to avoid touching the flags directly, but then would have a function call.. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx