| Message ID | 20170912124726.19689-1-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michał Winiarski (2017-09-12 13:47:23) > Originally removed in: > c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs") > f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs") > > Were accidentaly restored in: > 925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next") > > We can also remove unused variable and replace it with a WARN. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 3 +-- > drivers/gpu/drm/i915/intel_uc.h | 4 ---- > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 48a1e9349a2c..8a550785b257 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > struct intel_guc *guc = &rq->i915->guc; > struct i915_guc_client *client = guc->execbuf_client; > unsigned long flags; > - int b_ret; > > /* WA to flush out the pending GMADR writes to ring buffer. */ > if (i915_vma_is_map_and_fenceable(rq->ring->vma)) > @@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > spin_lock_irqsave(&client->wq_lock, flags); > > guc_wq_item_append(client, rq); > - b_ret = guc_ring_doorbell(client); > + WARN_ON(guc_ring_doorbell(client)); Hmm: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5658/fi-skl-6700k/igt@gem_exec_parallel@basic.html Ok, time to dig. -Chris >
Quoting Chris Wilson (2017-09-12 15:17:56) > Quoting Michał Winiarski (2017-09-12 13:47:23) > > Originally removed in: > > c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs") > > f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs") > > > > Were accidentaly restored in: > > 925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next") > > > > We can also remove unused variable and replace it with a WARN. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_guc_submission.c | 3 +-- > > drivers/gpu/drm/i915/intel_uc.h | 4 ---- > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > > index 48a1e9349a2c..8a550785b257 100644 > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > > @@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > > struct intel_guc *guc = &rq->i915->guc; > > struct i915_guc_client *client = guc->execbuf_client; > > unsigned long flags; > > - int b_ret; > > > > /* WA to flush out the pending GMADR writes to ring buffer. */ > > if (i915_vma_is_map_and_fenceable(rq->ring->vma)) > > @@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > > spin_lock_irqsave(&client->wq_lock, flags); > > > > guc_wq_item_append(client, rq); > > - b_ret = guc_ring_doorbell(client); > > + WARN_ON(guc_ring_doorbell(client)); > > Hmm: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5658/fi-skl-6700k/igt@gem_exec_parallel@basic.html > > Ok, time to dig. Actually that's a compliment to your lockless implementation. We have multiple threads competing to ring the doorbell. So one more patch required... -Chris
Quoting Chris Wilson (2017-09-12 15:21:06) > Quoting Chris Wilson (2017-09-12 15:17:56) > > Quoting Michał Winiarski (2017-09-12 13:47:23) > > > Originally removed in: > > > c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs") > > > f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs") > > > > > > Were accidentaly restored in: > > > 925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next") > > > > > > We can also remove unused variable and replace it with a WARN. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_guc_submission.c | 3 +-- > > > drivers/gpu/drm/i915/intel_uc.h | 4 ---- > > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > > > index 48a1e9349a2c..8a550785b257 100644 > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > > > @@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > > > struct intel_guc *guc = &rq->i915->guc; > > > struct i915_guc_client *client = guc->execbuf_client; > > > unsigned long flags; > > > - int b_ret; > > > > > > /* WA to flush out the pending GMADR writes to ring buffer. */ > > > if (i915_vma_is_map_and_fenceable(rq->ring->vma)) > > > @@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) > > > spin_lock_irqsave(&client->wq_lock, flags); > > > > > > guc_wq_item_append(client, rq); > > > - b_ret = guc_ring_doorbell(client); > > > + WARN_ON(guc_ring_doorbell(client)); > > > > Hmm: > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5658/fi-skl-6700k/igt@gem_exec_parallel@basic.html > > > > Ok, time to dig. > > Actually that's a compliment to your lockless implementation. We have > multiple threads competing to ring the doorbell. So one more patch > required... I suggest another cmpxchg loop: static void guc_ring_doorbell(struct i915_guc_client *client) { struct guc_doorbell_info *db = __get_doorbell(client); u32 cookie; do { cookie = READ_ONCE(db->cookie); } while (cmpxchg(&db->cookie, cookie, cookie + 1) != cookie); GEM_BUG_ON(db->db_status != GUC_DOORBELL_ENABLED); } with associated cleanup. -Chris
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 48a1e9349a2c..8a550785b257 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) struct intel_guc *guc = &rq->i915->guc; struct i915_guc_client *client = guc->execbuf_client; unsigned long flags; - int b_ret; /* WA to flush out the pending GMADR writes to ring buffer. */ if (i915_vma_is_map_and_fenceable(rq->ring->vma)) @@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq) spin_lock_irqsave(&client->wq_lock, flags); guc_wq_item_append(client, rq); - b_ret = guc_ring_doorbell(client); + WARN_ON(guc_ring_doorbell(client)); client->submissions[engine_id] += 1; diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 22ae52b17b0f..69daf4c01cd0 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -59,10 +59,6 @@ struct drm_i915_gem_request; * available in the work queue (note, the queue is shared, * not per-engine). It is OK for this to be nonzero, but * it should not be huge! - * b_fail: failed to ring the doorbell. This should never happen, unless - * somehow the hardware misbehaves, or maybe if the GuC firmware - * crashes? We probably need to reset the GPU to recover. - * retcode: errno from last guc_submit() */ struct i915_guc_client { struct i915_vma *vma;
Originally removed in: c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs") f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs") Were accidentaly restored in: 925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next") We can also remove unused variable and replace it with a WARN. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 +-- drivers/gpu/drm/i915/intel_uc.h | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-)