| Message ID | 20170425103835.31871-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25/04/2017 11:38, Chris Wilson wrote: > As we now share the execlist_port[] tracking for both execlists/guc, we > can reset the inflight count on both and report which requests are being > restarted. > > Suggested-by: Michel Thierry <michel.thierry@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index d3612969098f..961f4a2ad498 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > return ret; > } > > -static u32 port_seqno(struct execlist_port *port) > -{ > - return port->request ? port->request->global_seqno : 0; > -} > - > static int gen8_init_common_ring(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > + struct execlist_port *port = engine->execlist_port; > + unsigned int n; > int ret; > > ret = intel_mocs_init_engine(engine); > @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > > /* After a GPU reset, we may have requests to replay */ > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > - if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) { > - DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n", > - engine->name, > - port_seqno(&engine->execlist_port[0]), > - port_seqno(&engine->execlist_port[1])); > - engine->execlist_port[0].count = 0; > - engine->execlist_port[1].count = 0; > - execlists_submit_ports(engine); > + > + for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) { > + if (!port[n].request) > + break; At some point we'll maybe want to start thinking about the for_each_port_request or something. > + > + DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n", > + engine->name, n, > + port[n].request->global_seqno); > + > + /* Discard the current inflight count */ > + port[n].count = 0; > } > > + if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) > + execlists_submit_ports(engine); > + > return 0; > } > > Looks okay to me. Someone has plans to start using counts in guc mode? Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Tue, Apr 25, 2017 at 01:21:47PM +0100, Tvrtko Ursulin wrote: > > On 25/04/2017 11:38, Chris Wilson wrote: > >As we now share the execlist_port[] tracking for both execlists/guc, we > >can reset the inflight count on both and report which requests are being > >restarted. > > > >Suggested-by: Michel Thierry <michel.thierry@intel.com> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Michel Thierry <michel.thierry@intel.com> > >Cc: Mika Kuoppala <mika.kuoppala@intel.com> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++------------- > > 1 file changed, 16 insertions(+), 13 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index d3612969098f..961f4a2ad498 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > > return ret; > > } > > > >-static u32 port_seqno(struct execlist_port *port) > >-{ > >- return port->request ? port->request->global_seqno : 0; > >-} > >- > > static int gen8_init_common_ring(struct intel_engine_cs *engine) > > { > > struct drm_i915_private *dev_priv = engine->i915; > >+ struct execlist_port *port = engine->execlist_port; > >+ unsigned int n; > > int ret; > > > > ret = intel_mocs_init_engine(engine); > >@@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > > > > /* After a GPU reset, we may have requests to replay */ > > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > >- if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) { > >- DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n", > >- engine->name, > >- port_seqno(&engine->execlist_port[0]), > >- port_seqno(&engine->execlist_port[1])); > >- engine->execlist_port[0].count = 0; > >- engine->execlist_port[1].count = 0; > >- execlists_submit_ports(engine); > >+ > >+ for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) { > >+ if (!port[n].request) > >+ break; > > At some point we'll maybe want to start thinking about the > for_each_port_request or something. Something. > >+ > >+ DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n", > >+ engine->name, n, > >+ port[n].request->global_seqno); > >+ > >+ /* Discard the current inflight count */ > >+ port[n].count = 0; > > } > > > >+ if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) > >+ execlists_submit_ports(engine); > >+ > > return 0; > > } > > > > > > Looks okay to me. Someone has plans to start using counts in guc mode? Spoilers. I moved the submission out of a few locks to reduce lock contention (queued_spin_lock_slowpath exists for guc!), makes the CPU numbers look better, but bxt/guc is still 3x higher latency. I just hope it is broken firmware. -Chris
On Tue, Apr 25, 2017 at 11:58:46AM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Report request restarts for both execlists/guc > URL : https://patchwork.freedesktop.org/series/23502/ > State : success > > == Summary == > > Series 23502v1 drm/i915: Report request restarts for both execlists/guc > https://patchwork.freedesktop.org/api/1.0/series/23502/revisions/1/mbox/ > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-uc: > fail -> PASS (fi-snb-2600) fdo#100007 > Test gem_exec_suspend: > Subgroup basic-s4-devices: > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 > > fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 > fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 Pushed, thanks for the review. -Chris
On 25/04/17 05:30, Chris Wilson wrote: > On Tue, Apr 25, 2017 at 01:21:47PM +0100, Tvrtko Ursulin wrote: >> >> On 25/04/2017 11:38, Chris Wilson wrote: >>> As we now share the execlist_port[] tracking for both execlists/guc, we >>> can reset the inflight count on both and report which requests are being >>> restarted. >>> Thanks, one less patch for me (and I arrived late to the party, I see it's already merged). >>> Suggested-by: Michel Thierry <michel.thierry@intel.com> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++------------- >>> 1 file changed, 16 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index d3612969098f..961f4a2ad498 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >>> return ret; >>> } >>> >>> -static u32 port_seqno(struct execlist_port *port) >>> -{ >>> - return port->request ? port->request->global_seqno : 0; >>> -} >>> - >>> static int gen8_init_common_ring(struct intel_engine_cs *engine) >>> { >>> struct drm_i915_private *dev_priv = engine->i915; >>> + struct execlist_port *port = engine->execlist_port; >>> + unsigned int n; >>> int ret; >>> >>> ret = intel_mocs_init_engine(engine); >>> @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) >>> >>> /* After a GPU reset, we may have requests to replay */ >>> clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); >>> - if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) { >>> - DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n", >>> - engine->name, >>> - port_seqno(&engine->execlist_port[0]), >>> - port_seqno(&engine->execlist_port[1])); >>> - engine->execlist_port[0].count = 0; >>> - engine->execlist_port[1].count = 0; >>> - execlists_submit_ports(engine); >>> + >>> + for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) { >>> + if (!port[n].request) >>> + break; >> >> At some point we'll maybe want to start thinking about the >> for_each_port_request or something. > > Something. > >>> + >>> + DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n", >>> + engine->name, n, >>> + port[n].request->global_seqno); >>> + >>> + /* Discard the current inflight count */ >>> + port[n].count = 0; >>> } >>> >>> + if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) >>> + execlists_submit_ports(engine); >>> + >>> return 0; >>> } >>> >>> >> >> Looks okay to me. Someone has plans to start using counts in guc mode? > > Spoilers. I moved the submission out of a few locks to reduce lock > contention (queued_spin_lock_slowpath exists for guc!), makes the CPU > numbers look better, but bxt/guc is still 3x higher latency. I just hope > it is broken firmware. Beating a dead horse?
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index d3612969098f..961f4a2ad498 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) return ret; } -static u32 port_seqno(struct execlist_port *port) -{ - return port->request ? port->request->global_seqno : 0; -} - static int gen8_init_common_ring(struct intel_engine_cs *engine) { struct drm_i915_private *dev_priv = engine->i915; + struct execlist_port *port = engine->execlist_port; + unsigned int n; int ret; ret = intel_mocs_init_engine(engine); @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) /* After a GPU reset, we may have requests to replay */ clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); - if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) { - DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n", - engine->name, - port_seqno(&engine->execlist_port[0]), - port_seqno(&engine->execlist_port[1])); - engine->execlist_port[0].count = 0; - engine->execlist_port[1].count = 0; - execlists_submit_ports(engine); + + for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) { + if (!port[n].request) + break; + + DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n", + engine->name, n, + port[n].request->global_seqno); + + /* Discard the current inflight count */ + port[n].count = 0; } + if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) + execlists_submit_ports(engine); + return 0; }
As we now share the execlist_port[] tracking for both execlists/guc, we can reset the inflight count on both and report which requests are being restarted. Suggested-by: Michel Thierry <michel.thierry@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)