On 16/02/2017 11:21, Chris Wilson wrote:
On Thu, Feb 16, 2017 at 10:45:56AM +0000, Tvrtko Ursulin wrote:
On 16/02/2017 10:36, Chris Wilson wrote:
On Thu, Feb 16, 2017 at 10:21:17AM +0000, Tvrtko Ursulin wrote:
On 16/02/2017 09:29, Chris Wilson wrote:
If an interrupt arrives whilst we are performing the irq-seqno barrier,
recheck the seqno again before returning.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c1b400f1ede4..ecb8b414bdd2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4102,6 +4102,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
if (__i915_gem_request_completed(req, seqno))
return true;
+ if (!engine->irq_seqno_barrier)
+ return false;
+
/* Ensure our read of the seqno is coherent so that we
* do not "miss an interrupt" (i.e. if this is the last
* request and the seqno write from the GPU is not visible
@@ -4113,8 +4116,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
* but it is easier and safer to do it every time the waiter
* is woken.
*/
- if (engine->irq_seqno_barrier &&
- test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
+ while (test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
unsigned long flags;
/* The ordering of irq_posted versus applying the barrier
Hmmm.. if this helps it feels that there is a race somewhere.
Because we have processed one interrupt, but it wasn't for us.
There may be many interrupts between now and the one we actually want.
The caller of this function is (or was at the time) has the first seqno
to be signaled, it is just not necessarily the next interrupt.
That
means there will be another one coming. Why handle that at this
level? Why check twice and not three, four, five times? :)
Because they are all for us! This only saves a trip through schedule. If
we don't loop here, we just loop at the next level.
Yes, which looks more correct to me. Because the caller then do what
they want. Signaller just sleeps and i915_wait_request potentially
busy waits for a bit.
A persuasive counter argument would be:
Interrupts can be arriving frequently for an indefinite period before we
see our seqno that breaks the loop. We should not allow this to spin
indefinitely and at the very least need to be checking for pending
interrupts or timeslice exhaustion. Both of which are handled in the
callers.
I got lost in this branch of the thread. Are you making an argument for
me doubting the patch, or for you supporting the patch? :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
