Chris Wilson <[email protected]> writes: > Quoting Chris Wilson (2018-03-28 20:20:19) >> Quoting Francisco Jerez (2018-03-28 19:55:12) >> > Hi Chris, >> > >> [snip] >> > That said, it wouldn't hurt to call each of them once per sequence of >> > overlapping requests. Do you want me to call them from >> > execlists_set/clear_active() themselves when bit == EXECLISTS_ACTIVE_USER, >> > or at each callsite of execlists_set/clear_active()? [possibly protected >> > with a check that execlists_is_active(EXECLISTS_ACTIVE_USER) wasn't >> > already the expected value] >> >> No, I was thinking of adding an execlists_start()/execlists_stop() >> [naming suggestions welcome, begin/end?] where we could hook additional >> bookkeeping into. > > Trying to call execlist_begin() once didn't pan out. It's easier to > reuse for similar bookkeeping used in future patches if execlist_begin() > (or whatever name suits best) at the start of each context. > > Something along the lines of: > > @@ -374,6 +374,19 @@ execlists_context_status_change(struct i915_request *rq, > unsigned long status) > status, rq); > } > > +static inline void > +execlists_begin(struct intel_engine_execlists *execlists,
I had started writing something along the same lines in my working tree
called execlists_active_user_begin/end -- Which name do you prefer?
> + struct execlist_port *port)
> +{
What do you expect the port argument to be useful for? Is it ever going
to be anything other than execlists->port?
> + execlists_set_active_once(execlists, EXECLISTS_ACTIVE_USER);
> +}
> +
> +static inline void
> +execlists_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_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);
This line doesn't seem to exist in my working tree. I guess it was just
added?
> + execlists_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 an 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_begin(execlists, port);
Isn't this going to call execlists_begin() roughly once per request?
What's the purpose if we expect it to be a no-op except for the first
request submitted after execlists_end()? Isn't the intention to provide
a hook for bookkeeping that depends on idle to active and active to idle
transitions of the hardware?
> + else
> + execlists_end(execlists);
> } else {
> port_set(port, port_pack(rq, count));
> }
>
Isn't there an "execlists_clear_active(execlists,
EXECLISTS_ACTIVE_USER);" call in your tree a few lines below that is now
redundant?
> -Chris
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
