Re: [PATCH v2 10/11] drm/i915/execlists: Preemption!

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> When we write to ELSP, it triggers a context preemption at the earliest
> arbitration point (3DPRIMITIVE, some PIPECONTROLs, a few other
> operations and the explicit MI_ARB_CHECK). If this is to the same
> context, it triggers a LITE_RESTORE where the RING_TAIL is merely
> updated (used currently to chain requests from the same context
> together, avoiding bubbles). However, if it is to a different context, a
> full context-switch is performed and it will start to execute the new
> context saving the image of the old for later execution.
> 
> Previously we avoided preemption by only submitting a new context when
> the old was idle. But now we wish embrace it, and if the new request has
> a higher priority than the currently executing request, we write to the
> ELSP regardless, thus triggering preemption, but we tell the GPU to
> switch to our special preemption context (not the target). In the
> context-switch interrupt handler, we know that the previous contexts
> have finished execution and so can unwind all the incomplete requests
> and compute the new highest priority request to execute.
> 
> It would be feasible to avoid the switch-to-idle intermediate by
> programming the ELSP with the target context. The difficulty is in
> tracking which request that should be whilst maintaining the dependency
> change, the error comes in with coalesced requests. As we only track the
> most recent request and its priority, we may run into the issue of being
> tricked in preempting a high priority request that was followed by a
> low priority request from the same context (e.g. for PI);

"followed" is bit ambiguous here, depending on how you view the
ordering, wall time or ports.

> worse still
> that earlier request may be our own dependency and the order then broken
> by preemption. By injecting the switch-to-idle and then recomputing the
> priority queue, we avoid the issue with tracking in-flight coalesced
> requests. Having tried the preempt-to-busy approach, and failed to find
> a way around the coalesced priority issue, Michal's original proposal to
> inject an idle context (based on handling GuC preemption) succeeds.
> 
> The current heuristic for deciding when to preempt are only if the new
> request is of higher priority, and has the privileged priority of
> greater than 0. Note that the scheduler remains unfair!
> 
> v2: Disable for gen8 (bdw/bsw) as we need additional w/a for GPGPU.
> Since, the feature is now conditional and not always available when we
> have a scheduler, make it known via the HAS_SCHEDULER GETPARAM (now a
> capability mask).
> 
> Suggested-by: Michal Winiarski <michal.winiarski@intel.com>
> 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: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>

<SNIP>

> @@ -489,26 +489,44 @@ static void port_assign(struct execlist_port *port,
>  	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
>  }
>  
> +static void inject_preempt_context(struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce =
> +		&engine->i915->preempt_context->engine[engine->id];
> +	u32 __iomem *elsp =
> +		engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));

engine_elsp() helper or so?

> +	unsigned int n;
> +
> +	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);

I think this could/should be done way earlier?

> +
> +	memset(ce->ring->vaddr + ce->ring->tail, 0, 8);
> +	ce->ring->tail += 8;
> +	ce->ring->tail &= (ce->ring->size - 1);
> +	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;

An awful lot of pre-expectations here, would be shame if somebody
documented them.

> +
> +	for (n = execlists_num_ports(&engine->execlists); --n; ) {

This is fine detail compared to the other loop, "<=" vs "<" (or maybe
even <= -1) would make a more clear distinction, but I'm not arguing.

> +		writel(0, elsp);
> +		writel(0, elsp);
> +	}
> +	writel(upper_32_bits(ce->lrc_desc), elsp);
> +	writel(lower_32_bits(ce->lrc_desc), elsp);

Could also be elsp_write inline helper.

> @@ -696,7 +746,7 @@ static void intel_lrc_irq_handler(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 *port = execlists->port;
> +	struct execlist_port * const port = execlists->port;
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	/* We can skip acquiring intel_runtime_pm_get() here as it was taken
> @@ -781,6 +831,23 @@ static void intel_lrc_irq_handler(unsigned long data)
>  			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>  				continue;
>  
> +			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
> +			    buf[2*head + 1] == PREEMPT_ID) {

(2 * head + 1), could be a helper again, potentially with a descriptive
name so the below comment can be removed and doesn't need to be
duplicated :)

> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -238,6 +238,8 @@ struct intel_engine_execlists {
>  #define EXECLIST_MAX_PORTS 2
>  	} port[EXECLIST_MAX_PORTS];
>  
> +	bool preempt;

Definitely want to improve the variable name, "preempting" would be the
smallest acceptable change.

Combine that with kerneldoc here and fix at memset(), this is

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx