| Message ID | 20180329102631.11015-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > We would like to start doing some bookkeeping at the beginning, between > contexts and at the end of execlists submission. We already mark the > beginning and end using EXECLISTS_ACTIVE_USER, to provide an indication > when the HW is idle. This give us a pair of sequence points we can then > expand on for further bookkeeping. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Francisco Jerez <currojerez@riseup.net> > --- > drivers/gpu/drm/i915/intel_lrc.c | 42 ++++++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 11 ++++++++- > 2 files changed, 41 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 654634254b64..61fb1387feb3 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -374,6 +374,19 @@ execlists_context_status_change(struct i915_request *rq, unsigned long status) > status, rq); > } > > +static inline void > +execlists_user_begin(struct intel_engine_execlists *execlists, > + const struct execlist_port *port) > +{ > + execlists_set_active_once(execlists, EXECLISTS_ACTIVE_USER); > +} > + > +static inline void > +execlists_user_end(struct intel_engine_execlists *execlists) > +{ > + execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); > +} > + > static inline void > execlists_context_schedule_in(struct i915_request *rq) > { > @@ -710,7 +723,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > spin_unlock_irq(&engine->timeline->lock); > > if (submit) { > - execlists_set_active(execlists, EXECLISTS_ACTIVE_USER); > + execlists_user_begin(execlists, execlists->port); > execlists_submit_ports(engine); > } > > @@ -741,7 +754,7 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) > port++; > } > > - execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); > + execlists_user_end(execlists); > } > > static void clear_gtiir(struct intel_engine_cs *engine) > @@ -872,7 +885,7 @@ static void execlists_submission_tasklet(unsigned long data) > { > struct intel_engine_cs * const engine = (struct intel_engine_cs *)data; > struct intel_engine_execlists * const execlists = &engine->execlists; > - struct execlist_port * const port = execlists->port; > + struct execlist_port *port = execlists->port; > struct drm_i915_private *dev_priv = engine->i915; > bool fw = false; > > @@ -1010,9 +1023,19 @@ static void execlists_submission_tasklet(unsigned long data) > > GEM_BUG_ON(count == 0); > if (--count == 0) { > + /* > + * On the final event corresponding to the > + * submission of this context, we expect either > + * an element-switch event or a completion > + * event (and on completion, the active-idle > + * marker). No more preemptions, lite-restore > + * or otherwise Missing punctuation in the last sentence. > + */ > GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED); > GEM_BUG_ON(port_isset(&port[1]) && > !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH)); > + GEM_BUG_ON(!port_isset(&port[1]) && > + !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); > GEM_BUG_ON(!i915_request_completed(rq)); > execlists_context_schedule_out(rq); > trace_i915_request_out(rq); > @@ -1021,17 +1044,14 @@ static void execlists_submission_tasklet(unsigned long data) > GEM_TRACE("%s completed ctx=%d\n", > engine->name, port->context_id); > > - execlists_port_complete(execlists, port); > + port = execlists_port_complete(execlists, port); > + if (port_isset(port)) > + execlists_user_begin(execlists, port); > + else > + execlists_user_end(execlists); > } else { > port_set(port, port_pack(rq, count)); > } > - > - /* 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); Do we want to update the EXECLISTS_ACTIVE_USER tracking done in intel_guc_submission.c to use the same wrappers? Either way looks okay to me: Reviewed-by: Francisco Jerez <currojerez@riseup.net> > } > > if (head != execlists->csb_head) { > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index a02c7b3b9d55..2e20627e254b 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -638,6 +638,13 @@ execlists_set_active(struct intel_engine_execlists *execlists, > __set_bit(bit, (unsigned long *)&execlists->active); > } > > +static inline bool > +execlists_set_active_once(struct intel_engine_execlists *execlists, > + unsigned int bit) > +{ > + return !__test_and_set_bit(bit, (unsigned long *)&execlists->active); > +} > + > static inline void > execlists_clear_active(struct intel_engine_execlists *execlists, > unsigned int bit) > @@ -664,7 +671,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists) > return execlists->port_mask + 1; > } > > -static inline void > +static inline struct execlist_port * > execlists_port_complete(struct intel_engine_execlists * const execlists, > struct execlist_port * const port) > { > @@ -675,6 +682,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, > > memmove(port, port + 1, m * sizeof(struct execlist_port)); > memset(port + m, 0, sizeof(struct execlist_port)); > + > + return port; > } > > static inline unsigned int > -- > 2.16.3
Quoting Francisco Jerez (2018-03-29 18:20:23) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > We would like to start doing some bookkeeping at the beginning, between > > contexts and at the end of execlists submission. We already mark the > > beginning and end using EXECLISTS_ACTIVE_USER, to provide an indication > > when the HW is idle. This give us a pair of sequence points we can then > > expand on for further bookkeeping. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Francisco Jerez <currojerez@riseup.net> > > Do we want to update the EXECLISTS_ACTIVE_USER tracking done in > intel_guc_submission.c to use the same wrappers? Either way looks okay > to me: Yeah, looking at what's in the pipeline, the hook from guc will be used in the same way. -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 654634254b64..61fb1387feb3 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -374,6 +374,19 @@ execlists_context_status_change(struct i915_request *rq, unsigned long status) status, rq); } +static inline void +execlists_user_begin(struct intel_engine_execlists *execlists, + const struct execlist_port *port) +{ + execlists_set_active_once(execlists, EXECLISTS_ACTIVE_USER); +} + +static inline void +execlists_user_end(struct intel_engine_execlists *execlists) +{ + execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); +} + static inline void execlists_context_schedule_in(struct i915_request *rq) { @@ -710,7 +723,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) spin_unlock_irq(&engine->timeline->lock); if (submit) { - execlists_set_active(execlists, EXECLISTS_ACTIVE_USER); + execlists_user_begin(execlists, execlists->port); execlists_submit_ports(engine); } @@ -741,7 +754,7 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists) port++; } - execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); + execlists_user_end(execlists); } static void clear_gtiir(struct intel_engine_cs *engine) @@ -872,7 +885,7 @@ static void execlists_submission_tasklet(unsigned long data) { struct intel_engine_cs * const engine = (struct intel_engine_cs *)data; struct intel_engine_execlists * const execlists = &engine->execlists; - struct execlist_port * const port = execlists->port; + struct execlist_port *port = execlists->port; struct drm_i915_private *dev_priv = engine->i915; bool fw = false; @@ -1010,9 +1023,19 @@ static void execlists_submission_tasklet(unsigned long data) GEM_BUG_ON(count == 0); if (--count == 0) { + /* + * On the final event corresponding to the + * submission of this context, we expect either + * an element-switch event or a completion + * event (and on completion, the active-idle + * marker). No more preemptions, lite-restore + * or otherwise + */ GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED); GEM_BUG_ON(port_isset(&port[1]) && !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH)); + GEM_BUG_ON(!port_isset(&port[1]) && + !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); GEM_BUG_ON(!i915_request_completed(rq)); execlists_context_schedule_out(rq); trace_i915_request_out(rq); @@ -1021,17 +1044,14 @@ static void execlists_submission_tasklet(unsigned long data) GEM_TRACE("%s completed ctx=%d\n", engine->name, port->context_id); - execlists_port_complete(execlists, port); + port = execlists_port_complete(execlists, port); + if (port_isset(port)) + execlists_user_begin(execlists, port); + else + execlists_user_end(execlists); } else { port_set(port, port_pack(rq, count)); } - - /* 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) { diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index a02c7b3b9d55..2e20627e254b 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -638,6 +638,13 @@ execlists_set_active(struct intel_engine_execlists *execlists, __set_bit(bit, (unsigned long *)&execlists->active); } +static inline bool +execlists_set_active_once(struct intel_engine_execlists *execlists, + unsigned int bit) +{ + return !__test_and_set_bit(bit, (unsigned long *)&execlists->active); +} + static inline void execlists_clear_active(struct intel_engine_execlists *execlists, unsigned int bit) @@ -664,7 +671,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists) return execlists->port_mask + 1; } -static inline void +static inline struct execlist_port * execlists_port_complete(struct intel_engine_execlists * const execlists, struct execlist_port * const port) { @@ -675,6 +682,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists, memmove(port, port + 1, m * sizeof(struct execlist_port)); memset(port + m, 0, sizeof(struct execlist_port)); + + return port; } static inline unsigned int
We would like to start doing some bookkeeping at the beginning, between contexts and at the end of execlists submission. We already mark the beginning and end using EXECLISTS_ACTIVE_USER, to provide an indication when the HW is idle. This give us a pair of sequence points we can then expand on for further bookkeeping. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Francisco Jerez <currojerez@riseup.net> --- drivers/gpu/drm/i915/intel_lrc.c | 42 ++++++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_ringbuffer.h | 11 ++++++++- 2 files changed, 41 insertions(+), 12 deletions(-)