Re: [Intel-gfx] [PATCH] drm/i915/execlists: Track begin/end of execlists submission sequences

[Chris Wilson]

Quoting Francisco Jerez (2018-03-29 18:20:23)
> Chris Wilson  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 
> > Cc: Mika Kuoppala 
> > Cc: Francisco Jerez 
> 
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/execlists: Track begin/end of execlists submission sequences

[Francisco Jerez]

Chris Wilson  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 
> Cc: Mika Kuoppala 
> Cc: Francisco Jerez 
> ---
>  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(>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 = >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([1]) &&
>  !(status & 
> GEN8_CTX_STATUS_ELEMENT_SWITCH));
> + GEM_BUG_ON(!port_isset([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 & 

[Intel-gfx] [PATCH] drm/i915/execlists: Track begin/end of execlists submission sequences

[Chris Wilson]

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 
Cc: Mika Kuoppala 
Cc: Francisco Jerez 
---
 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(>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 = >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([1]) &&
   !(status & 
GEN8_CTX_STATUS_ELEMENT_SWITCH));
+   GEM_BUG_ON(!port_isset([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);