| Message ID | 20180503195416.22498-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/3/2018 12:54 PM, Chris Wilson wrote: > Limit the arbitration (where preemption may occur) to inside the batch, > and prevent it from happening on the pipecontrols/flushes we use to > write the breadcrumb seqno. Once the user batch is complete, we have > nothing left to do but serialise and emit the breadcrumb; switching > contexts at this point is futile so don't. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > > Michał and Michel, > please take a look and see if you can think of any objections. No objections, I was only thinking what would happen if we arm the watchdog (and we decide timeout != reset the engine) and forgot to write a reply. For the record, I think it'd be ok anyway. Reviewed-by: Michel Thierry <michel.thierry@intel.com> > -Chris > > --- > drivers/gpu/drm/i915/intel_lrc.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 3d747d1c3d4d..9f3cce022b2d 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1933,7 +1933,7 @@ static int gen8_emit_bb_start(struct i915_request *rq, > rq->ctx->ppgtt->pd_dirty_rings &= ~intel_engine_flag(rq->engine); > } > > - cs = intel_ring_begin(rq, 4); > + cs = intel_ring_begin(rq, 6); > if (IS_ERR(cs)) > return PTR_ERR(cs); > > @@ -1962,6 +1962,9 @@ static int gen8_emit_bb_start(struct i915_request *rq, > (flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0); > *cs++ = lower_32_bits(offset); > *cs++ = upper_32_bits(offset); > + > + *cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE; > + *cs++ = MI_NOOP; > intel_ring_advance(rq, cs); > > return 0; > @@ -2104,7 +2107,7 @@ static void gen8_emit_breadcrumb(struct i915_request *request, u32 *cs) > cs = gen8_emit_ggtt_write(cs, request->global_seqno, > intel_hws_seqno_address(request->engine)); > *cs++ = MI_USER_INTERRUPT; > - *cs++ = MI_NOOP; > + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > request->tail = intel_ring_offset(request, cs); > assert_ring_tail_valid(request->ring, request->tail); > > @@ -2120,7 +2123,7 @@ static void gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs) > cs = gen8_emit_ggtt_write_rcs(cs, request->global_seqno, > intel_hws_seqno_address(request->engine)); > *cs++ = MI_USER_INTERRUPT; > - *cs++ = MI_NOOP; > + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > request->tail = intel_ring_offset(request, cs); > assert_ring_tail_valid(request->ring, request->tail); > >
On 03/05/18 20:54, Chris Wilson wrote: > Limit the arbitration (where preemption may occur) to inside the batch, > and prevent it from happening on the pipecontrols/flushes we use to > write the breadcrumb seqno. Once the user batch is complete, we have > nothing left to do but serialise and emit the breadcrumb; switching > contexts at this point is futile so don't. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > > Michał and Michel, > please take a look and see if you can think of any objections. > -Chris > > --- > drivers/gpu/drm/i915/intel_lrc.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 3d747d1c3d4d..9f3cce022b2d 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1933,7 +1933,7 @@ static int gen8_emit_bb_start(struct i915_request *rq, > rq->ctx->ppgtt->pd_dirty_rings &= ~intel_engine_flag(rq->engine); > } > > - cs = intel_ring_begin(rq, 4); > + cs = intel_ring_begin(rq, 6); > if (IS_ERR(cs)) > return PTR_ERR(cs); > > @@ -1962,6 +1962,9 @@ static int gen8_emit_bb_start(struct i915_request *rq, > (flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0); > *cs++ = lower_32_bits(offset); > *cs++ = upper_32_bits(offset); > + > + *cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE; > + *cs++ = MI_NOOP; > intel_ring_advance(rq, cs); > > return 0; > @@ -2104,7 +2107,7 @@ static void gen8_emit_breadcrumb(struct i915_request *request, u32 *cs) > cs = gen8_emit_ggtt_write(cs, request->global_seqno, > intel_hws_seqno_address(request->engine)); > *cs++ = MI_USER_INTERRUPT; > - *cs++ = MI_NOOP; > + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > request->tail = intel_ring_offset(request, cs); > assert_ring_tail_valid(request->ring, request->tail); > > @@ -2120,7 +2123,7 @@ static void gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs) > cs = gen8_emit_ggtt_write_rcs(cs, request->global_seqno, > intel_hws_seqno_address(request->engine)); > *cs++ = MI_USER_INTERRUPT; > - *cs++ = MI_NOOP; > + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > request->tail = intel_ring_offset(request, cs); > assert_ring_tail_valid(request->ring, request->tail); >
Quoting Patchwork (2018-05-04 01:31:49) > == Series Details == > > Series: drm/i915/execlists: Drop preemption arbitrations points along the ring > URL : https://patchwork.freedesktop.org/series/42660/ > State : success > > == Summary == > > = CI Bug Log - changes from CI_DRM_4135_full -> Patchwork_8903_full = > > == Summary - WARNING == > > Minor unknown changes coming with Patchwork_8903_full need to be verified > manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_8903_full, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/42660/revisions/1/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_8903_full: > > === IGT changes === > > ==== Warnings ==== > > igt@kms_color@pipe-c-ctm-blue-to-red: > shard-kbl: PASS -> SKIP +29 > > > == Known issues == > > Here are the changes found in Patchwork_8903_full that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@kms_cursor_crc@cursor-256x256-suspend: > shard-kbl: PASS -> DMESG-FAIL (fdo#103232, fdo#103841, fdo#103558) > > igt@kms_flip@dpms-vs-vblank-race-interruptible: > shard-hsw: PASS -> FAIL (fdo#103060) > > igt@kms_flip@plain-flip-fb-recreate-interruptible: > shard-hsw: PASS -> FAIL (fdo#100368) > > igt@kms_flip@plain-flip-ts-check-interruptible: > shard-glk: PASS -> FAIL (fdo#100368) > > igt@kms_rmfb@rmfb-ioctl: > shard-kbl: PASS -> DMESG-WARN (fdo#105602, fdo#103558) +29 > > igt@kms_setmode@basic: > shard-glk: PASS -> FAIL (fdo#99912) It succumbed to link training, no surreptitious GPU hangs. Thanks to the review, -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3d747d1c3d4d..9f3cce022b2d 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1933,7 +1933,7 @@ static int gen8_emit_bb_start(struct i915_request *rq, rq->ctx->ppgtt->pd_dirty_rings &= ~intel_engine_flag(rq->engine); } - cs = intel_ring_begin(rq, 4); + cs = intel_ring_begin(rq, 6); if (IS_ERR(cs)) return PTR_ERR(cs); @@ -1962,6 +1962,9 @@ static int gen8_emit_bb_start(struct i915_request *rq, (flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0); *cs++ = lower_32_bits(offset); *cs++ = upper_32_bits(offset); + + *cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE; + *cs++ = MI_NOOP; intel_ring_advance(rq, cs); return 0; @@ -2104,7 +2107,7 @@ static void gen8_emit_breadcrumb(struct i915_request *request, u32 *cs) cs = gen8_emit_ggtt_write(cs, request->global_seqno, intel_hws_seqno_address(request->engine)); *cs++ = MI_USER_INTERRUPT; - *cs++ = MI_NOOP; + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; request->tail = intel_ring_offset(request, cs); assert_ring_tail_valid(request->ring, request->tail); @@ -2120,7 +2123,7 @@ static void gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs) cs = gen8_emit_ggtt_write_rcs(cs, request->global_seqno, intel_hws_seqno_address(request->engine)); *cs++ = MI_USER_INTERRUPT; - *cs++ = MI_NOOP; + *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; request->tail = intel_ring_offset(request, cs); assert_ring_tail_valid(request->ring, request->tail);