On Fri, Feb 24, 2017 at 10:04:23AM +0000, Tvrtko Ursulin wrote:
>
> On 23/02/2017 15:42, Chris Wilson wrote:
> >diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> >index 5f73d8c0a38a..0efee879df23 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_request.h
> >+++ b/drivers/gpu/drm/i915/i915_gem_request.h
> >@@ -32,10 +32,12 @@
> >
> > struct drm_file;
> > struct drm_i915_gem_object;
> >+struct drm_i915_gem_request;
> >
> > struct intel_wait {
> > struct rb_node node;
> > struct task_struct *tsk;
> >+ struct drm_i915_gem_request *request;
> > u32 seqno;
>
> Hm, do we now have a situation where both waiters and signallers
> hold a reference to the request so could drop the seqno field from
> here?
Keeping the seqno field helps with the lowlevel testcase that works
without requests. Not scientific, but it was one less pointer for the
irq handler.
> > };
> >
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >index bc70e2c451b2..3c79753edf0e 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -1033,12 +1033,41 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
> >
> > static void notify_ring(struct intel_engine_cs *engine)
> > {
> >- bool waiters;
> >+ struct drm_i915_gem_request *rq = NULL;
> >+ struct intel_wait *wait;
> >+
> >+ if (!intel_engine_has_waiter(engine))
> >+ return;
>
> I think we need the tracepoint before the bail out.
>
> >
> > atomic_inc(&engine->irq_count);
>
> And the increment as well.
>
> > set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
> >- waiters = intel_engine_wakeup(engine);
> >- trace_intel_engine_notify(engine, waiters);
> >+
> >+ spin_lock(&engine->breadcrumbs.lock);
> >+ wait = engine->breadcrumbs.first_wait;
> >+ if (wait) {
> >+ /* We use a callback from the dma-fence to submit
> >+ * requests after waiting on our own requests. To
> >+ * ensure minimum delay in queuing the next request to
> >+ * hardware, signal the fence now rather than wait for
> >+ * the signaler to be woken up. We still wake up the
> >+ * waiter in order to handle the irq-seqno coherency
> >+ * issues (we may receive the interrupt before the
> >+ * seqno is written, see __i915_request_irq_complete())
> >+ * and to handle coalescing of multiple seqno updates
> >+ * and many waiters.
> >+ */
> >+ if (i915_seqno_passed(intel_engine_get_seqno(engine),
> >+ wait->seqno))
> >+ rq = wait->request;
> >+
> >+ wake_up_process(wait->tsk);
> >+ }
> >+ spin_unlock(&engine->breadcrumbs.lock);
> >+
> >+ if (rq) /* rq is protected by RCU, so safe from hard-irq context */
> >+ dma_fence_signal(&rq->fence);
> >+
> >+ trace_intel_engine_notify(engine, wait);
Hmm, I put the trace here for access to wait. I guess we only need a
READ_ONCE(engine->breadcrumbs.first_wait) for tracking?
Note in the next patch, it's different again. And yeah I should massage
it such that it doesn't introduce a fluctuation.
> > }
> >
> > static void vlv_c0_read(struct drm_i915_private *dev_priv,
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index 027c93e34c97..5f2665aa817c 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -81,8 +81,11 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
> > * every jiffie in order to kick the oldest waiter to do the
> > * coherent seqno check.
> > */
> >- if (intel_engine_wakeup(engine))
> >- mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> >+ if (!intel_engine_has_waiter(engine))
> >+ return;
> >+
> >+ intel_engine_wakeup(engine);
> >+ mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
>
> Not sure it is worth splitting this in two instead of just leaving
> it as it was.
The problem with the previous code was the timer stopped if the waiter
was running at that moment. I am still playing to find something that
doesn't look horrible. Just going back to the bool intel_engine_wakeup,
which is now far too large to be an inline for an unimportant function.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
