On Thu, Jan 12, 2017 at 07:02:56AM +0000, Tvrtko Ursulin wrote:
>
> On 11/01/2017 21:24, Chris Wilson wrote:
> >This emulates execlists on top of the GuC in order to defer submission of
> >requests to the hardware. This deferral allows time for high priority
> >requests to gazump their way to the head of the queue, however it nerfs
> >the GuC by converting it back into a simple execlist (where the CPU has
> >to wake up after every request to feed new commands into the GuC).
> >
> >v2: Drop hack status - though iirc there is still a lockdep inversion
> >between fence and engine->timeline->lock (which is impossible as the
> >nesting only occurs on different fences - hopefully just requires some
> >judicious lockdep annotation)
> >v3: Apply lockdep nesting to enabling signaling on the request, using
> >the pattern we already have in __i915_gem_request_submit();
> >v4: Replaying requests after a hang also now needs the timeline
> >spinlock, to disable the interrupts at least
> >
> >Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/i915_guc_submission.c | 95
> > +++++++++++++++++++++++++++---
> > drivers/gpu/drm/i915/i915_irq.c | 4 +-
> > drivers/gpu/drm/i915/intel_lrc.c | 5 +-
> > 3 files changed, 92 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> >b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index 913d87358972..2f0a853f820a 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -350,7 +350,7 @@ int i915_guc_wq_reserve(struct drm_i915_gem_request
> >*request)
> > u32 freespace;
> > int ret;
> >
> >- spin_lock(&client->wq_lock);
> >+ spin_lock_irq(&client->wq_lock);
> > freespace = CIRC_SPACE(client->wq_tail, desc->head, client->wq_size);
> > freespace -= client->wq_rsvd;
> > if (likely(freespace >= wqi_size)) {
> >@@ -360,7 +360,7 @@ int i915_guc_wq_reserve(struct drm_i915_gem_request
> >*request)
> > client->no_wq_space++;
> > ret = -EAGAIN;
> > }
> >- spin_unlock(&client->wq_lock);
> >+ spin_unlock_irq(&client->wq_lock);
> >
> > return ret;
> > }
> >@@ -372,9 +372,9 @@ void i915_guc_wq_unreserve(struct drm_i915_gem_request
> >*request)
> >
> > GEM_BUG_ON(READ_ONCE(client->wq_rsvd) < wqi_size);
> >
> >- spin_lock(&client->wq_lock);
> >+ spin_lock_irq(&client->wq_lock);
> > client->wq_rsvd -= wqi_size;
> >- spin_unlock(&client->wq_lock);
> >+ spin_unlock_irq(&client->wq_lock);
> > }
> >
> > /* Construct a Work Item and append it to the GuC's Work Queue */
> >@@ -534,10 +534,87 @@ static void __i915_guc_submit(struct
> >drm_i915_gem_request *rq)
> >
> > static void i915_guc_submit(struct drm_i915_gem_request *rq)
> > {
> >- i915_gem_request_submit(rq);
> >+ __i915_gem_request_submit(rq);
> > __i915_guc_submit(rq);
> > }
> >
> >+static void nested_enable_signaling(struct drm_i915_gem_request *rq)
> >+{
> >+ if (test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> >+ &rq->fence.flags))
> >+ return;
>
> You didn't like the idea of duplicating the tracepoint from
> dma_fence_enable_sw_signalling here?
No, I was just forgetful and worrying about the S3 hangs.
> > /* Replay the current set of previously submitted requests */
> >+ spin_lock_irqsave(&engine->timeline->lock, flags);
> > list_for_each_entry(rq, &engine->timeline->requests, link) {
> > client->wq_rsvd += sizeof(struct guc_wq_item);
>
> Strictly speaking the guc client wq lock as well so maybe just put a
> comment here saying it is not needed or maybe just take it.
True.
-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
