| Message ID | 20170228084746.21128-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/02/2017 08:47, Chris Wilson wrote: > Following the use of dma_fence_signal() from within our interrupt > handler, we need to make guc->wq_lock also irq-safe. This was done > previously as part of the guc scheduler patch (which also started > mixing our fences with the interrupt handler), but is now required to > fix the current guc submission backend. Doh! :) > Fixes: 67b807a89230 ("drm/i915: Delay disabling the user interrupt for > breadcrumbs") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index beec88a30347..c49cb5c8e975 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -348,7 +348,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)) { > @@ -358,7 +358,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; > } > @@ -370,9 +370,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); > } __i915_guc_submit would need irqsave as well since it is called from the signal callback. > /* Construct a Work Item and append it to the GuC's Work Queue */ > @@ -946,15 +946,21 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > /* Take over from manual control of ELSP (execlists) */ > for_each_engine(engine, dev_priv, id) { > struct drm_i915_gem_request *rq; > + unsigned long flags; > > engine->submit_request = i915_guc_submit; > engine->schedule = NULL; > > /* Replay the current set of previously submitted requests */ > + spin_lock_irqsave(&engine->timeline->lock, flags); This one can be normal spin_lock_irq. > list_for_each_entry(rq, &engine->timeline->requests, link) { > + spin_lock(&client->wq_lock); > client->wq_rsvd += sizeof(struct guc_wq_item); > + spin_unlock(&client->wq_lock); > + > __i915_guc_submit(rq); > } > + spin_unlock_irqrestore(&engine->timeline->lock, flags); > } > > return 0; > Regards, Tvrtko
On Tue, Feb 28, 2017 at 08:58:18AM +0000, Tvrtko Ursulin wrote: > > On 28/02/2017 08:47, Chris Wilson wrote: > >Following the use of dma_fence_signal() from within our interrupt > >handler, we need to make guc->wq_lock also irq-safe. This was done > >previously as part of the guc scheduler patch (which also started > >mixing our fences with the interrupt handler), but is now required to > >fix the current guc submission backend. > > Doh! :) > > >Fixes: 67b807a89230 ("drm/i915: Delay disabling the user interrupt for > >breadcrumbs") > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >--- > > drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > >index beec88a30347..c49cb5c8e975 100644 > >--- a/drivers/gpu/drm/i915/i915_guc_submission.c > >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c > >@@ -348,7 +348,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)) { > >@@ -358,7 +358,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; > > } > >@@ -370,9 +370,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); > > } > > __i915_guc_submit would need irqsave as well since it is called from > the signal callback. Hmm, this was guc unreserve, and can be just spin_lock_irq. __i915_guc_submit is always called with irq disabled. -Chris
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index beec88a30347..c49cb5c8e975 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -348,7 +348,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)) { @@ -358,7 +358,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; } @@ -370,9 +370,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 */ @@ -946,15 +946,21 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) /* Take over from manual control of ELSP (execlists) */ for_each_engine(engine, dev_priv, id) { struct drm_i915_gem_request *rq; + unsigned long flags; engine->submit_request = i915_guc_submit; engine->schedule = NULL; /* Replay the current set of previously submitted requests */ + spin_lock_irqsave(&engine->timeline->lock, flags); list_for_each_entry(rq, &engine->timeline->requests, link) { + spin_lock(&client->wq_lock); client->wq_rsvd += sizeof(struct guc_wq_item); + spin_unlock(&client->wq_lock); + __i915_guc_submit(rq); } + spin_unlock_irqrestore(&engine->timeline->lock, flags); } return 0;
Following the use of dma_fence_signal() from within our interrupt handler, we need to make guc->wq_lock also irq-safe. This was done previously as part of the guc scheduler patch (which also started mixing our fences with the interrupt handler), but is now required to fix the current guc submission backend. Fixes: 67b807a89230 ("drm/i915: Delay disabling the user interrupt for breadcrumbs") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)