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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
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@xxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux