| Message ID | 20170426101516.633-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > 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
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 <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > 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
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 <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> 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 <tvrtko.ursulin@intel.com> >> 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 >
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);
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 <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)