On 09/03/2018 17:33, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-03-09 17:24:33)
On 08/03/2018 14:07, Chris Wilson wrote:
There is some redundancy between dma_fence->ops->enable_signaling (via
i915_fence_enable_signaling) and our backend,
intel_engine_enable_signaling() in that both levels recheck the fence
status multiple times. If we convert intel_engine_enable_signaling() to
return the information desired by dma_fence->ops->enable_signaling, we
can reduce i915_fence_enable_signaling to a simple stub and avoid
trying to reinterpret the same information.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_request.c | 6 +-----
drivers/gpu/drm/i915/intel_breadcrumbs.c | 21 +++++++++++++--------
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index d437beac3969..2a841800d4cf 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -59,11 +59,7 @@ static bool i915_fence_signaled(struct dma_fence *fence)
static bool i915_fence_enable_signaling(struct dma_fence *fence)
{
- if (i915_fence_signaled(fence))
- return false;
This was based on hws seqno check...
-
- intel_engine_enable_signaling(to_request(fence), true);
- return !i915_fence_signaled(fence);
+ return intel_engine_enable_signaling(to_request(fence), true);
}
@@ -768,11 +769,15 @@ void intel_engine_enable_signaling(struct i915_request *request, bool wakeup)
*/
spin_lock(&b->rb_lock);
insert_signal(b, request, seqno);
- wakeup &= __intel_engine_add_wait(engine, &request->signaling.wait);
+ wakeup &= __intel_engine_add_wait(engine, wait);
spin_unlock(&b->rb_lock);
- if (wakeup)
+ if (wakeup) {
wake_up_process(b->signaler);
+ return !intel_wait_complete(wait);
... and would now be based on breadcrumb waiter waking up and removing
itself from the tree.
And don't forget the same HWS check before the waiter is inserted. So we
have the same guards as before, just inside yet another spinlock.
True, did not think of this. So it may proceed a bit deeper in the call
chain but there is no actual behaviour change as I described it.
So some potential latency where it wasn't before, well, enable-disable
signalling cycles actually.
The extra steps would be the insert_signal(). Fwiw, we could reorder the
insert_signal() but frankly this, dma_fence enable signaling of an
inflight request, is not a fast path. More commonly we will be enabling
signaling on the request as it is submitted (where wakeup is false and
we know that the HWS cannot be complete).
Yes, no complaints after realizing the above.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
