Re: [Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first

[Tvrtko Ursulin]

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(>fence);
-   local_bh_enable(); /* kick start the tasklets */
+   if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >fence.flags)) {
+   local_bh_disable();
+   dma_fence_signal(>fence);
+   local_bh_enable(); /* kick start the tasklet */
+   }

spin_lock_irq(>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

[Chris Wilson]

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(>fence);
> >-local_bh_enable(); /* kick start the tasklets */
> >+if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> >+  >fence.flags)) {
> >+local_bh_disable();
> >+dma_fence_signal(>fence);
> >+local_bh_enable(); /* kick start the tasklet */
> >+}
> >
> > spin_lock_irq(>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

[Tvrtko Ursulin]

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(>fence);
-   local_bh_enable(); /* kick start the tasklets */
+   if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >fence.flags)) {
+   local_bh_disable();
+   dma_fence_signal(>fence);
+   local_bh_enable(); /* kick start the tasklet */
+   }

spin_lock_irq(>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

[Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first

[Chris Wilson]

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(>fence);
-   local_bh_enable(); /* kick start the tasklets */
+   if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >fence.flags)) {
+   local_bh_disable();
+   dma_fence_signal(>fence);
+   local_bh_enable(); /* kick start the tasklet */
+   }
 
spin_lock_irq(>rb_lock);
 
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx