* [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking @ 2017-10-23 20:06 Chris Wilson 2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson 2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork 0 siblings, 2 replies; 11+ messages in thread From: Chris Wilson @ 2017-10-23 20:06 UTC (permalink / raw) To: intel-gfx In the idle worker we drop the prolonged GT wakeref used to cover such essentials as interrupt delivery. (When a CS interrupt arrives, we also assert that the GT is awake.) However, it turns out that 10ms is not long enough to be assured that the last CS interrupt has been delivered, so bump that to 200ms, and move the entirety of that wait to before we take the struct_mutex to avoid blocking. As this is now a potentially long wait, restore the earlier behaviour of bailing out early when a new request arrives. v2: Break out the repeated check for new requests into its own little helper to try and improve the self-commentary. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 026cb52ece0b..bb0e85043e01 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3276,13 +3276,20 @@ i915_gem_retire_work_handler(struct work_struct *work) } } +static inline bool +new_requests_since_last_retire(const struct drm_i915_private *i915) +{ + return (READ_ONCE(i915->gt.active_requests) || + work_pending(&i915->gt.idle_work.work)); +} + static void i915_gem_idle_work_handler(struct work_struct *work) { struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), gt.idle_work.work); - struct drm_device *dev = &dev_priv->drm; bool rearm_hangcheck; + ktime_t end; if (!READ_ONCE(dev_priv->gt.awake)) return; @@ -3291,14 +3298,21 @@ i915_gem_idle_work_handler(struct work_struct *work) * Wait for last execlists context complete, but bail out in case a * new request is submitted. */ - wait_for(intel_engines_are_idle(dev_priv), 10); - if (READ_ONCE(dev_priv->gt.active_requests)) - return; + end = ktime_add_ms(ktime_get(), 200); + do { + if (new_requests_since_last_retire(dev_priv)) + return; + + if (intel_engines_are_idle(dev_priv)) + break; + + usleep_range(100, 500); + } while (ktime_before(ktime_get(), end)); rearm_hangcheck = cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); - if (!mutex_trylock(&dev->struct_mutex)) { + if (!mutex_trylock(&dev_priv->drm.struct_mutex)) { /* Currently busy, come back later */ mod_delayed_work(dev_priv->wq, &dev_priv->gt.idle_work, @@ -3310,13 +3324,14 @@ i915_gem_idle_work_handler(struct work_struct *work) * New request retired after this work handler started, extend active * period until next instance of the work. */ - if (work_pending(work)) - goto out_unlock; - - if (dev_priv->gt.active_requests) + if (new_requests_since_last_retire(dev_priv)) goto out_unlock; - if (wait_for(intel_engines_are_idle(dev_priv), 10)) + /* + * We are committed now to parking the engines, make sure there + * will be no more interrupts arriving later. + */ + if (!intel_engines_are_idle(dev_priv)) DRM_ERROR("Timeout waiting for engines to idle\n"); intel_engines_mark_idle(dev_priv); @@ -3330,7 +3345,7 @@ i915_gem_idle_work_handler(struct work_struct *work) gen6_rps_idle(dev_priv); intel_runtime_pm_put(dev_priv); out_unlock: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev_priv->drm.struct_mutex); out_rearm: if (rearm_hangcheck) { -- 2.15.0.rc1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson @ 2017-10-23 20:06 ` Chris Wilson 2017-10-23 21:12 ` Chris Wilson 2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork 1 sibling, 1 reply; 11+ messages in thread From: Chris Wilson @ 2017-10-23 20:06 UTC (permalink / raw) To: intel-gfx; +Cc: Mika Kuoppala Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists context-switch when idle") we noticed the presence of late context-switch interrupts. We were able to filter those out by looking at whether the ELSP remained active, but in commit beecec901790 ("drm/i915/execlists: Preemption!") that became problematic as we now anticipate receiving a context-switch event for preemption while ELSP may be empty. To restore the spurious interrupt suppression, add a counter for the expected number of pending context-switches and skip if we do not need to handle this interrupt to make forward progress. v2: Don't forget to switch on for preempt. v3: Reduce the counter to a on/off boolean tracker. Declare the HW as active when we first submit, and idle after the final completion event (with which we confirm the HW says it is idle), and track each source of activity separately. With a finite number of sources, it should us debug which gets stuck. Fixes: beecec901790 ("drm/i915/execlists: Preemption!") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++ drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++-- drivers/gpu/drm/i915/intel_lrc.c | 27 ++++++++++++++++++------ drivers/gpu/drm/i915/intel_ringbuffer.h | 34 ++++++++++++++++++++++++++++-- 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index a2e8114b739d..f84c267728fd 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) execlists->first = rb; if (submit) { port_assign(port, last); + execlists_set_active(execlists, EXECLISTS_ACTIVE_USER); i915_guc_submit(engine); } spin_unlock_irq(&engine->timeline->lock); @@ -633,6 +634,8 @@ static void i915_guc_irq_handler(unsigned long data) rq = port_request(&port[0]); } + if (!rq) + execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); if (!port_isset(last_port)) i915_guc_dequeue(engine); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b1296a55c1e4..f8205841868b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift) bool tasklet = false; if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) { - __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); - tasklet = true; + if (READ_ONCE(engine->execlists.active)) { + __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); + tasklet = true; + } } if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) { diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index a47a9c6bea52..ab5bf4e2e28e 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) return false; - /* Both ports drained, no more ELSP submission? */ - if (port_request(&engine->execlists.port[0])) + /* Waiting to drain ELSP? */ + if (READ_ONCE(engine->execlists.active)) return false; /* ELSP is empty, but there are ready requests? */ @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m) idx); } } + drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active); rcu_read_unlock(); } else if (INTEL_GEN(dev_priv) > 6) { drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 7f45dd7dc3e5..233c992a886b 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -575,7 +575,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine) * the state of the GPU is known (idle). */ inject_preempt_context(engine); - execlists->preempt = true; + execlists_set_active(execlists, + EXECLISTS_ACTIVE_PREEMPT); goto unlock; } else { /* @@ -683,8 +684,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine) unlock: spin_unlock_irq(&engine->timeline->lock); - if (submit) + if (submit) { + execlists_set_active(execlists, EXECLISTS_ACTIVE_USER); execlists_submit_ports(engine); + } } static void @@ -696,6 +699,7 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists) while (num_ports-- && port_isset(port)) { struct drm_i915_gem_request *rq = port_request(port); + GEM_BUG_ON(!execlists->active); execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED); i915_gem_request_put(rq); @@ -861,15 +865,21 @@ static void intel_lrc_irq_handler(unsigned long data) unwind_incomplete_requests(engine); spin_unlock_irq(&engine->timeline->lock); - GEM_BUG_ON(!execlists->preempt); - execlists->preempt = false; + GEM_BUG_ON(!execlists_is_active(execlists, + EXECLISTS_ACTIVE_PREEMPT)); + execlists_clear_active(execlists, + EXECLISTS_ACTIVE_PREEMPT); continue; } if (status & GEN8_CTX_STATUS_PREEMPTED && - execlists->preempt) + execlists_is_active(execlists, + EXECLISTS_ACTIVE_PREEMPT)) continue; + GEM_BUG_ON(!execlists_is_active(execlists, + EXECLISTS_ACTIVE_USER)); + /* Check the context/desc id for this event matches */ GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id); @@ -892,6 +902,9 @@ static void intel_lrc_irq_handler(unsigned long data) /* After the final element, the hw should be idle */ GEM_BUG_ON(port_count(port) == 0 && !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); + if (port_count(port) == 0) + execlists_clear_active(execlists, + EXECLISTS_ACTIVE_USER); } if (head != execlists->csb_head) { @@ -901,7 +914,7 @@ static void intel_lrc_irq_handler(unsigned long data) } } - if (!execlists->preempt) + if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT)) execlists_dequeue(engine); intel_uncore_forcewake_put(dev_priv, execlists->fw_domains); @@ -1460,7 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) GT_CONTEXT_SWITCH_INTERRUPT << engine->irq_shift); clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); execlists->csb_head = -1; - execlists->preempt = false; + execlists->active = 0; /* After a GPU reset, we may have requests to replay */ if (!i915_modparams.enable_guc_submission && execlists->first) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 17186f067408..51bc704bf413 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -241,9 +241,17 @@ struct intel_engine_execlists { } port[EXECLIST_MAX_PORTS]; /** - * @preempt: are we currently handling a preempting context switch? + * @active: is the HW active? We consider the HW as active after + * submitted any context for execution until we have seen the last + * context completion event. After that, we do not expect any more + * events until we submit, and so can park the HW. + * + * As we have a small number of different sources from which we feed + * the HW, we track the state of each inside a single bitfield. */ - bool preempt; + unsigned int active; +#define EXECLISTS_ACTIVE_USER 0 +#define EXECLISTS_ACTIVE_PREEMPT 1 /** * @port_mask: number of execlist ports - 1 @@ -525,6 +533,27 @@ struct intel_engine_cs { u32 (*get_cmd_length_mask)(u32 cmd_header); }; +static inline void +execlists_set_active(struct intel_engine_execlists *execlists, + unsigned int bit) +{ + __set_bit(bit, (unsigned long *)&execlists->active); +} + +static inline void +execlists_clear_active(struct intel_engine_execlists *execlists, + unsigned int bit) +{ + __clear_bit(bit, (unsigned long *)&execlists->active); +} + +static inline bool +execlists_is_active(const struct intel_engine_execlists *execlists, + unsigned int bit) +{ + return test_bit(bit, (unsigned long *)&execlists->active); +} + static inline unsigned int execlists_num_ports(const struct intel_engine_execlists * const execlists) { @@ -538,6 +567,7 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, const unsigned int m = execlists->port_mask; GEM_BUG_ON(port_index(port, execlists) != 0); + GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER)); memmove(port, port + 1, m * sizeof(struct execlist_port)); memset(port + m, 0, sizeof(struct execlist_port)); -- 2.15.0.rc1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson @ 2017-10-23 21:12 ` Chris Wilson 0 siblings, 0 replies; 11+ messages in thread From: Chris Wilson @ 2017-10-23 21:12 UTC (permalink / raw) To: intel-gfx; +Cc: Mika Kuoppala Quoting Chris Wilson (2017-10-23 21:06:16) > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > context-switch when idle") we noticed the presence of late > context-switch interrupts. We were able to filter those out by looking > at whether the ELSP remained active, but in commit beecec901790 > ("drm/i915/execlists: Preemption!") that became problematic as we now > anticipate receiving a context-switch event for preemption while ELSP > may be empty. To restore the spurious interrupt suppression, add a > counter for the expected number of pending context-switches and skip if > we do not need to handle this interrupt to make forward progress. Looking at an example from https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_1299/ the common case is where we still get the interrupt after already parsing the whole CSB: <6>[ 22.723238] i915 0000:00:02.0: [drm] vecs0 <6>[ 22.723246] i915 0000:00:02.0: [drm] current seqno 8, last 8, hangcheck 0 [-277277 ms], inflight 0 <6>[ 22.723260] i915 0000:00:02.0: [drm] Reset count: 0 <6>[ 22.723269] i915 0000:00:02.0: [drm] Requests: <6>[ 22.723278] i915 0000:00:02.0: [drm] RING_START: 0x007fb000 [0x00000000] <6>[ 22.723289] i915 0000:00:02.0: [drm] RING_HEAD: 0x00000278 [0x00000000] <6>[ 22.723300] i915 0000:00:02.0: [drm] RING_TAIL: 0x00000278 [0x00000000] <6>[ 22.723311] i915 0000:00:02.0: [drm] RING_CTL: 0x00003001 [] <6>[ 22.723322] i915 0000:00:02.0: [drm] ACTHD: 0x00000000_00000278 <6>[ 22.723333] i915 0000:00:02.0: [drm] BBADDR: 0x00000000_00000004 <6>[ 22.723343] i915 0000:00:02.0: [drm] Execlist status: 0x00000301 00000000 <6>[ 22.723355] i915 0000:00:02.0: [drm] Execlist CSB read 1 [1 cached], write 1 [1 from hws], interrupt posted? no <6>[ 22.723370] i915 0000:00:02.0: [drm] ELSP[0] idle <6>[ 22.723378] i915 0000:00:02.0: [drm] ELSP[1] idle <6>[ 22.723387] i915 0000:00:02.0: [drm] HW active? 0x0 <6>[ 22.723402] i915 0000:00:02.0: [drm] Those should not lead to hitting BUG_ON(gt.awake) though as the tasklet is flushed before we clear gt.awake. Except if maybe the interrupt arrives after the tasklet_kill... Given that we wait for the engines to be idle before parking, we should be safe enough with diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index bb0e85043e01..fa46137d431a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3327,6 +3327,8 @@ i915_gem_idle_work_handler(struct work_struct *work) if (new_requests_since_last_retire(dev_priv)) goto out_unlock; + synchronize_irq(dev_priv->drm.irq); + /* * We are committed now to parking the engines, make sure there * will be no more interrupts arriving later. to flush a pending irq and not worry about a multi-phase park. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking 2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson 2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson @ 2017-10-23 20:26 ` Patchwork 1 sibling, 0 replies; 11+ messages in thread From: Patchwork @ 2017-10-23 20:26 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == Series Details == Series: series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking URL : https://patchwork.freedesktop.org/series/32486/ State : warning == Summary == Series 32486v1 series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking https://patchwork.freedesktop.org/api/1.0/series/32486/revisions/1/mbox/ Test pm_rpm: Subgroup basic-pci-d3-state: pass -> DMESG-WARN (fi-hsw-4770r) fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:443s fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:456s fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:371s fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:531s fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:265s fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:506s fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:501s fi-byt-j1900 total:289 pass:253 dwarn:1 dfail:0 fail:0 skip:35 time:495s fi-byt-n2820 total:289 pass:249 dwarn:1 dfail:0 fail:0 skip:39 time:478s fi-cfl-s total:289 pass:253 dwarn:4 dfail:0 fail:0 skip:32 time:557s fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:417s fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:249s fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:578s fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:432s fi-hsw-4770r total:289 pass:261 dwarn:1 dfail:0 fail:0 skip:27 time:426s fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:434s fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:497s fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:460s fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:494s fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:570s fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:473s fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:592s fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:548s fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:453s fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:645s fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:527s fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:506s fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:459s fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:567s fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:424s 5c82a37eff83ab4e60e760fbaf03db5ba0563497 drm-tip: 2017y-10m-23d-18h-06m-28s UTC integration manifest 077bf0f23d24 drm/i915: Filter out spurious execlists context-switch interrupts 6129d04f29c2 drm/i915: Bump wait-times for the final CS interrupt before parking == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6146/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking @ 2017-10-20 9:59 Chris Wilson 2017-10-20 9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson 0 siblings, 1 reply; 11+ messages in thread From: Chris Wilson @ 2017-10-20 9:59 UTC (permalink / raw) To: intel-gfx In the idle worker we drop the prolonged GT wakeref used to cover such essentials as interrupt delivery. (When a CS interrupt arrives, we also assert that the GT is awake.) However, it turns out that 10ms is not long enough to be assured that the last CS interrupt has been delivered, so bump that to 200ms, and move the entirety of that wait to before we take the struct_mutex to avoid blocking. As this is now a potentially long wait, restore the earlier behaviour of bailing out early when a new request arrives. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 026cb52ece0b..d3a638613857 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3281,8 +3281,8 @@ i915_gem_idle_work_handler(struct work_struct *work) { struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), gt.idle_work.work); - struct drm_device *dev = &dev_priv->drm; bool rearm_hangcheck; + ktime_t end; if (!READ_ONCE(dev_priv->gt.awake)) return; @@ -3291,14 +3291,22 @@ i915_gem_idle_work_handler(struct work_struct *work) * Wait for last execlists context complete, but bail out in case a * new request is submitted. */ - wait_for(intel_engines_are_idle(dev_priv), 10); - if (READ_ONCE(dev_priv->gt.active_requests)) - return; + end = ktime_add_ms(ktime_get(), 200); + do { + if (READ_ONCE(dev_priv->gt.active_requests) || + work_pending(work)) + return; + + if (intel_engines_are_idle(dev_priv)) + break; + + usleep_range(100, 500); + } while (ktime_before(ktime_get(), end)); rearm_hangcheck = cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); - if (!mutex_trylock(&dev->struct_mutex)) { + if (!mutex_trylock(&dev_priv->drm.struct_mutex)) { /* Currently busy, come back later */ mod_delayed_work(dev_priv->wq, &dev_priv->gt.idle_work, @@ -3310,13 +3318,14 @@ i915_gem_idle_work_handler(struct work_struct *work) * New request retired after this work handler started, extend active * period until next instance of the work. */ - if (work_pending(work)) + if (dev_priv->gt.active_requests || work_pending(work)) goto out_unlock; - if (dev_priv->gt.active_requests) - goto out_unlock; - - if (wait_for(intel_engines_are_idle(dev_priv), 10)) + /* + * We are committed now to parking the engines, make sure there + * will be no more interrupts arriving later. + */ + if (!intel_engines_are_idle(dev_priv)) DRM_ERROR("Timeout waiting for engines to idle\n"); intel_engines_mark_idle(dev_priv); @@ -3330,7 +3339,7 @@ i915_gem_idle_work_handler(struct work_struct *work) gen6_rps_idle(dev_priv); intel_runtime_pm_put(dev_priv); out_unlock: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev_priv->drm.struct_mutex); out_rearm: if (rearm_hangcheck) { -- 2.15.0.rc1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 9:59 [PATCH 1/2] " Chris Wilson @ 2017-10-20 9:59 ` Chris Wilson 2017-10-20 13:21 ` Mika Kuoppala 2017-10-20 13:59 ` Mika Kuoppala 0 siblings, 2 replies; 11+ messages in thread From: Chris Wilson @ 2017-10-20 9:59 UTC (permalink / raw) To: intel-gfx; +Cc: Mika Kuoppala Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists context-switch when idle") we noticed the presence of late context-switch interrupts. We were able to filter those out by looking at whether the ELSP remained active, but in commit beecec901790 ("drm/i915/execlists: Preemption!") that became problematic as we now anticipate receiving a context-switch event for preemption while ELSP may be empty. To restore the spurious interrupt suppression, add a counter for the expected number of pending context-switches and skip if we do not need to handle this interrupt to make forward progress. Fixes: beecec901790 ("drm/i915/execlists: Preemption!") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++-- drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++---- drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++ 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index a2e8114b739d..c22d0a07467c 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port, struct drm_i915_gem_request *rq) { GEM_BUG_ON(rq == port_request(port)); + GEM_BUG_ON(port_isset(port)); - if (port_isset(port)) - i915_gem_request_put(port_request(port)); - - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); + port_set(port, i915_gem_request_get(rq)); nested_enable_signaling(rq); } @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) goto done; } - if (submit) + if (submit) { port_assign(port, last); + execlists->active++; + } port++; } @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) execlists->first = rb; if (submit) { port_assign(port, last); + execlists->active++; i915_guc_submit(engine); } spin_unlock_irq(&engine->timeline->lock); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b1296a55c1e4..f8205841868b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift) bool tasklet = false; if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) { - __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); - tasklet = true; + if (READ_ONCE(engine->execlists.active)) { + __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); + tasklet = true; + } } if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) { diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index a47a9c6bea52..8aecbc321786 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) return false; - /* Both ports drained, no more ELSP submission? */ - if (port_request(&engine->execlists.port[0])) + /* Waiting to drain ELSP? */ + if (READ_ONCE(engine->execlists.active)) return false; /* ELSP is empty, but there are ready requests? */ @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m) idx); } } + drm_printf(m, "\t\tELSP active %d\n", execlists->active); rcu_read_unlock(); } else if (INTEL_GEN(dev_priv) > 6) { drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 7f45dd7dc3e5..8a47b8211c74 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev, return true; } -static void port_assign(struct execlist_port *port, +static bool port_assign(struct execlist_port *port, struct drm_i915_gem_request *rq) { + bool was_idle; + GEM_BUG_ON(rq == port_request(port)); - if (port_isset(port)) + if (port_isset(port)) { i915_gem_request_put(port_request(port)); + was_idle = false; + } else { + was_idle = true; + } port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); + + return was_idle; } static void inject_preempt_context(struct intel_engine_cs *engine) @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) GEM_BUG_ON(last->ctx == rq->ctx); if (submit) - port_assign(port, last); + execlists->active += port_assign(port, last); port++; GEM_BUG_ON(port_isset(port)); @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) done: execlists->first = rb; if (submit) - port_assign(port, last); + execlists->active += port_assign(port, last); unlock: spin_unlock_irq(&engine->timeline->lock); @@ -696,10 +704,12 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists) while (num_ports-- && port_isset(port)) { struct drm_i915_gem_request *rq = port_request(port); + GEM_BUG_ON(!execlists->active); execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED); i915_gem_request_put(rq); memset(port, 0, sizeof(*port)); + execlists->active--; port++; } } @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data) GEM_BUG_ON(!execlists->preempt); execlists->preempt = false; + execlists->active--; + GEM_BUG_ON(execlists->active); continue; } @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); execlists->csb_head = -1; execlists->preempt = false; + execlists->active = 0; /* After a GPU reset, we may have requests to replay */ if (!i915_modparams.enable_guc_submission && execlists->first) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 17186f067408..588f5fe9d2b7 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -245,6 +245,11 @@ struct intel_engine_execlists { */ bool preempt; + /** + * @active: count of the number of outstanding CSB events + */ + unsigned int active; + /** * @port_mask: number of execlist ports - 1 */ @@ -538,9 +543,11 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, const unsigned int m = execlists->port_mask; GEM_BUG_ON(port_index(port, execlists) != 0); + GEM_BUG_ON(!execlists->active); memmove(port, port + 1, m * sizeof(struct execlist_port)); memset(port + m, 0, sizeof(struct execlist_port)); + execlists->active--; } static inline unsigned int -- 2.15.0.rc1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson @ 2017-10-20 13:21 ` Mika Kuoppala 2017-10-20 13:24 ` Chris Wilson 2017-10-20 13:59 ` Mika Kuoppala 1 sibling, 1 reply; 11+ messages in thread From: Mika Kuoppala @ 2017-10-20 13:21 UTC (permalink / raw) To: Chris Wilson, intel-gfx Chris Wilson <chris@chris-wilson.co.uk> writes: > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > context-switch when idle") we noticed the presence of late > context-switch interrupts. We were able to filter those out by looking > at whether the ELSP remained active, but in commit beecec901790 > ("drm/i915/execlists: Preemption!") that became problematic as we now > anticipate receiving a context-switch event for preemption while ELSP > may be empty. To restore the spurious interrupt suppression, add a This confuses me, how can we preempt something that was never submitted. Could you elaborate? Thanks, -Mika > counter for the expected number of pending context-switches and skip if > we do not need to handle this interrupt to make forward progress. > > Fixes: beecec901790 ("drm/i915/execlists: Preemption!") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- > drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- > drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++-- > drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++---- > drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++ > 5 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index a2e8114b739d..c22d0a07467c 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port, > struct drm_i915_gem_request *rq) > { > GEM_BUG_ON(rq == port_request(port)); > + GEM_BUG_ON(port_isset(port)); > > - if (port_isset(port)) > - i915_gem_request_put(port_request(port)); > - > - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); > + port_set(port, i915_gem_request_get(rq)); > nested_enable_signaling(rq); > } > > @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) > goto done; > } > > - if (submit) > + if (submit) { > port_assign(port, last); > + execlists->active++; > + } > port++; > } > > @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) > execlists->first = rb; > if (submit) { > port_assign(port, last); > + execlists->active++; > i915_guc_submit(engine); > } > spin_unlock_irq(&engine->timeline->lock); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index b1296a55c1e4..f8205841868b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift) > bool tasklet = false; > > if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) { > - __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > - tasklet = true; > + if (READ_ONCE(engine->execlists.active)) { > + __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > + tasklet = true; > + } > } > > if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) { > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a47a9c6bea52..8aecbc321786 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) > if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) > return false; > > - /* Both ports drained, no more ELSP submission? */ > - if (port_request(&engine->execlists.port[0])) > + /* Waiting to drain ELSP? */ > + if (READ_ONCE(engine->execlists.active)) > return false; > > /* ELSP is empty, but there are ready requests? */ > @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m) > idx); > } > } > + drm_printf(m, "\t\tELSP active %d\n", execlists->active); > rcu_read_unlock(); > } else if (INTEL_GEN(dev_priv) > 6) { > drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 7f45dd7dc3e5..8a47b8211c74 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev, > return true; > } > > -static void port_assign(struct execlist_port *port, > +static bool port_assign(struct execlist_port *port, > struct drm_i915_gem_request *rq) > { > + bool was_idle; > + > GEM_BUG_ON(rq == port_request(port)); > > - if (port_isset(port)) > + if (port_isset(port)) { > i915_gem_request_put(port_request(port)); > + was_idle = false; > + } else { > + was_idle = true; > + } > > port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); > + > + return was_idle; > } > > static void inject_preempt_context(struct intel_engine_cs *engine) > @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > GEM_BUG_ON(last->ctx == rq->ctx); > > if (submit) > - port_assign(port, last); > + execlists->active += port_assign(port, last); > port++; > > GEM_BUG_ON(port_isset(port)); > @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > done: > execlists->first = rb; > if (submit) > - port_assign(port, last); > + execlists->active += port_assign(port, last); > unlock: > spin_unlock_irq(&engine->timeline->lock); > > @@ -696,10 +704,12 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists) > while (num_ports-- && port_isset(port)) { > struct drm_i915_gem_request *rq = port_request(port); > > + GEM_BUG_ON(!execlists->active); > execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED); > i915_gem_request_put(rq); > > memset(port, 0, sizeof(*port)); > + execlists->active--; > port++; > } > } > @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data) > > GEM_BUG_ON(!execlists->preempt); > execlists->preempt = false; > + execlists->active--; > + GEM_BUG_ON(execlists->active); > continue; > } > > @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > execlists->csb_head = -1; > execlists->preempt = false; > + execlists->active = 0; > > /* After a GPU reset, we may have requests to replay */ > if (!i915_modparams.enable_guc_submission && execlists->first) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 17186f067408..588f5fe9d2b7 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -245,6 +245,11 @@ struct intel_engine_execlists { > */ > bool preempt; > > + /** > + * @active: count of the number of outstanding CSB events > + */ > + unsigned int active; > + > /** > * @port_mask: number of execlist ports - 1 > */ > @@ -538,9 +543,11 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, > const unsigned int m = execlists->port_mask; > > GEM_BUG_ON(port_index(port, execlists) != 0); > + GEM_BUG_ON(!execlists->active); > > memmove(port, port + 1, m * sizeof(struct execlist_port)); > memset(port + m, 0, sizeof(struct execlist_port)); > + execlists->active--; > } > > static inline unsigned int > -- > 2.15.0.rc1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 13:21 ` Mika Kuoppala @ 2017-10-20 13:24 ` Chris Wilson 2017-10-20 13:31 ` Mika Kuoppala 0 siblings, 1 reply; 11+ messages in thread From: Chris Wilson @ 2017-10-20 13:24 UTC (permalink / raw) To: Mika Kuoppala, intel-gfx Quoting Mika Kuoppala (2017-10-20 14:21:08) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > > context-switch when idle") we noticed the presence of late > > context-switch interrupts. We were able to filter those out by looking > > at whether the ELSP remained active, but in commit beecec901790 > > ("drm/i915/execlists: Preemption!") that became problematic as we now > > anticipate receiving a context-switch event for preemption while ELSP > > may be empty. To restore the spurious interrupt suppression, add a > > This confuses me, how can we preempt something that was never submitted. > Could you elaborate? The ELSP may become empty after we told the hw to switch to the preempt context. So the preemption context-switch may appear as a separate interrupt after !port_isset(execlists.port[0]). The joy of asynchronous communications. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 13:24 ` Chris Wilson @ 2017-10-20 13:31 ` Mika Kuoppala 2017-10-20 13:47 ` Chris Wilson 0 siblings, 1 reply; 11+ messages in thread From: Mika Kuoppala @ 2017-10-20 13:31 UTC (permalink / raw) To: Chris Wilson, intel-gfx Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2017-10-20 14:21:08) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists >> > context-switch when idle") we noticed the presence of late >> > context-switch interrupts. We were able to filter those out by looking >> > at whether the ELSP remained active, but in commit beecec901790 >> > ("drm/i915/execlists: Preemption!") that became problematic as we now >> > anticipate receiving a context-switch event for preemption while ELSP >> > may be empty. To restore the spurious interrupt suppression, add a >> >> This confuses me, how can we preempt something that was never submitted. >> Could you elaborate? > > The ELSP may become empty after we told the hw to switch to the preempt > context. So the preemption context-switch may appear as a separate > interrupt after !port_isset(execlists.port[0]). The joy of asynchronous > communications. Let me check if I got this right: as we dont use port[0] to switch to preempt context and thus we might get 2 interrupts in a a row, first one clearing port[0], we can't assume any idleness by port[0] status alone? -Mika _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 13:31 ` Mika Kuoppala @ 2017-10-20 13:47 ` Chris Wilson 0 siblings, 0 replies; 11+ messages in thread From: Chris Wilson @ 2017-10-20 13:47 UTC (permalink / raw) To: Mika Kuoppala, intel-gfx Quoting Mika Kuoppala (2017-10-20 14:31:14) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2017-10-20 14:21:08) > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > >> > context-switch when idle") we noticed the presence of late > >> > context-switch interrupts. We were able to filter those out by looking > >> > at whether the ELSP remained active, but in commit beecec901790 > >> > ("drm/i915/execlists: Preemption!") that became problematic as we now > >> > anticipate receiving a context-switch event for preemption while ELSP > >> > may be empty. To restore the spurious interrupt suppression, add a > >> > >> This confuses me, how can we preempt something that was never submitted. > >> Could you elaborate? > > > > The ELSP may become empty after we told the hw to switch to the preempt > > context. So the preemption context-switch may appear as a separate > > interrupt after !port_isset(execlists.port[0]). The joy of asynchronous > > communications. > > Let me check if I got this right: as we dont use port[0] to > switch to preempt context and thus we might get 2 interrupts in a > a row, first one clearing port[0], we can't assume any idleness > by port[0] status alone? Correct. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson 2017-10-20 13:21 ` Mika Kuoppala @ 2017-10-20 13:59 ` Mika Kuoppala 2017-10-20 14:24 ` Chris Wilson 1 sibling, 1 reply; 11+ messages in thread From: Mika Kuoppala @ 2017-10-20 13:59 UTC (permalink / raw) To: Chris Wilson, intel-gfx Chris Wilson <chris@chris-wilson.co.uk> writes: > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > context-switch when idle") we noticed the presence of late > context-switch interrupts. We were able to filter those out by looking > at whether the ELSP remained active, but in commit beecec901790 > ("drm/i915/execlists: Preemption!") that became problematic as we now > anticipate receiving a context-switch event for preemption while ELSP > may be empty. To restore the spurious interrupt suppression, add a > counter for the expected number of pending context-switches and skip if > we do not need to handle this interrupt to make forward progress. > > Fixes: beecec901790 ("drm/i915/execlists: Preemption!") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- > drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- > drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++-- > drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++---- > drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++ > 5 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index a2e8114b739d..c22d0a07467c 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port, > struct drm_i915_gem_request *rq) > { > GEM_BUG_ON(rq == port_request(port)); > + GEM_BUG_ON(port_isset(port)); > > - if (port_isset(port)) > - i915_gem_request_put(port_request(port)); > - > - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); > + port_set(port, i915_gem_request_get(rq)); No lite restore with guc so we can make this more strict and lean? > nested_enable_signaling(rq); > } > > @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) > goto done; > } > > - if (submit) > + if (submit) { > port_assign(port, last); > + execlists->active++; > + } > port++; > } > > @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine) > execlists->first = rb; > if (submit) { > port_assign(port, last); > + execlists->active++; > i915_guc_submit(engine); > } > spin_unlock_irq(&engine->timeline->lock); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index b1296a55c1e4..f8205841868b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift) > bool tasklet = false; > > if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) { > - __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > - tasklet = true; > + if (READ_ONCE(engine->execlists.active)) { > + __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > + tasklet = true; > + } > } > > if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) { > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a47a9c6bea52..8aecbc321786 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) > if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) > return false; > > - /* Both ports drained, no more ELSP submission? */ > - if (port_request(&engine->execlists.port[0])) > + /* Waiting to drain ELSP? */ > + if (READ_ONCE(engine->execlists.active)) > return false; > It would not make sense now to keep the port[0] check but we could get more fine grained debugging info if we keep it? -Mika > /* ELSP is empty, but there are ready requests? */ > @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m) > idx); > } > } > + drm_printf(m, "\t\tELSP active %d\n", execlists->active); > rcu_read_unlock(); > } else if (INTEL_GEN(dev_priv) > 6) { > drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 7f45dd7dc3e5..8a47b8211c74 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev, > return true; > } > > -static void port_assign(struct execlist_port *port, > +static bool port_assign(struct execlist_port *port, > struct drm_i915_gem_request *rq) > { > + bool was_idle; > + > GEM_BUG_ON(rq == port_request(port)); > > - if (port_isset(port)) > + if (port_isset(port)) { > i915_gem_request_put(port_request(port)); > + was_idle = false; > + } else { > + was_idle = true; > + } > > port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); > + > + return was_idle; > } > > static void inject_preempt_context(struct intel_engine_cs *engine) > @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > GEM_BUG_ON(last->ctx == rq->ctx); > > if (submit) > - port_assign(port, last); > + execlists->active += port_assign(port, last); > port++; > > GEM_BUG_ON(port_isset(port)); > @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > done: > execlists->first = rb; > if (submit) > - port_assign(port, last); > + execlists->active += port_assign(port, last); > unlock: > spin_unlock_irq(&engine->timeline->lock); > > @@ -696,10 +704,12 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists) > while (num_ports-- && port_isset(port)) { > struct drm_i915_gem_request *rq = port_request(port); > > + GEM_BUG_ON(!execlists->active); > execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED); > i915_gem_request_put(rq); > > memset(port, 0, sizeof(*port)); > + execlists->active--; > port++; > } > } > @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data) > > GEM_BUG_ON(!execlists->preempt); > execlists->preempt = false; > + execlists->active--; > + GEM_BUG_ON(execlists->active); > continue; > } > > @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > execlists->csb_head = -1; > execlists->preempt = false; > + execlists->active = 0; > > /* After a GPU reset, we may have requests to replay */ > if (!i915_modparams.enable_guc_submission && execlists->first) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 17186f067408..588f5fe9d2b7 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -245,6 +245,11 @@ struct intel_engine_execlists { > */ > bool preempt; > > + /** > + * @active: count of the number of outstanding CSB events > + */ > + unsigned int active; > + > /** > * @port_mask: number of execlist ports - 1 > */ > @@ -538,9 +543,11 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, > const unsigned int m = execlists->port_mask; > > GEM_BUG_ON(port_index(port, execlists) != 0); > + GEM_BUG_ON(!execlists->active); > > memmove(port, port + 1, m * sizeof(struct execlist_port)); > memset(port + m, 0, sizeof(struct execlist_port)); > + execlists->active--; > } > > static inline unsigned int > -- > 2.15.0.rc1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts 2017-10-20 13:59 ` Mika Kuoppala @ 2017-10-20 14:24 ` Chris Wilson 0 siblings, 0 replies; 11+ messages in thread From: Chris Wilson @ 2017-10-20 14:24 UTC (permalink / raw) To: Mika Kuoppala, intel-gfx Quoting Mika Kuoppala (2017-10-20 14:59:44) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists > > context-switch when idle") we noticed the presence of late > > context-switch interrupts. We were able to filter those out by looking > > at whether the ELSP remained active, but in commit beecec901790 > > ("drm/i915/execlists: Preemption!") that became problematic as we now > > anticipate receiving a context-switch event for preemption while ELSP > > may be empty. To restore the spurious interrupt suppression, add a > > counter for the expected number of pending context-switches and skip if > > we do not need to handle this interrupt to make forward progress. > > > > Fixes: beecec901790 ("drm/i915/execlists: Preemption!") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- > > drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- > > drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++-- > > drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++---- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++ > > 5 files changed, 37 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > > index a2e8114b739d..c22d0a07467c 100644 > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port, > > struct drm_i915_gem_request *rq) > > { > > GEM_BUG_ON(rq == port_request(port)); > > + GEM_BUG_ON(port_isset(port)); > > > > - if (port_isset(port)) > > - i915_gem_request_put(port_request(port)); > > - > > - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port))); > > + port_set(port, i915_gem_request_get(rq)); > > No lite restore with guc so we can make this more strict and lean? Yes. We are not coalescing requests into an active slot, so we always know that we are assigning a new request into a new slot. > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a47a9c6bea52..8aecbc321786 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) > > if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) > > return false; > > > > - /* Both ports drained, no more ELSP submission? */ > > - if (port_request(&engine->execlists.port[0])) > > + /* Waiting to drain ELSP? */ > > + if (READ_ONCE(engine->execlists.active)) > > return false; > > > > It would not make sense now to keep the port[0] check but > we could get more fine grained debugging info if we keep > it? This function is the antithesis of fine grained debugging. It is called in a tight loop, watching for idle so reporting from here ends up with a lot of noise. Often I ask exactly what isn't idle... Now that we do a single upfront wait, and then a check all is well, moving this to intel_engine_park where we can be more verbose in the one-off state check is the next step. The importance of making this change is so the gen8_cs_irq_handler() and the idle_worker are in sync about when to drop the GT wakeref. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-10-23 21:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson 2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson 2017-10-23 21:12 ` Chris Wilson 2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork -- strict thread matches above, loose matches on Subject: below -- 2017-10-20 9:59 [PATCH 1/2] " Chris Wilson 2017-10-20 9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson 2017-10-20 13:21 ` Mika Kuoppala 2017-10-20 13:24 ` Chris Wilson 2017-10-20 13:31 ` Mika Kuoppala 2017-10-20 13:47 ` Chris Wilson 2017-10-20 13:59 ` Mika Kuoppala 2017-10-20 14:24 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.