Re: [CI 1/4] drm/i915/gt: Mark the execlists->active as the primary volatile access

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Since we want to do a lockless read of the current active request, and
> that request is written to by process_csb also without serialisation, we
> need to instruct gcc to take care in reading the pointer itself.
>
> Otherwise, we have observed execlists_active() to report 0x40.
>
> [ 2400.760381] igt/para-4098    1..s. 2376479300us : process_csb: rcs0 cs-irq head=3, tail=4
> [ 2400.760826] igt/para-4098    1..s. 2376479303us : process_csb: rcs0 csb[4]: status=0x00000001:0x00000000
> [ 2400.761271] igt/para-4098    1..s. 2376479306us : trace_ports: rcs0: promote { b9c59:2622, b9c55:2624 }
> [ 2400.761726] igt/para-4097    0d... 2376479311us : __i915_schedule: rcs0: -2147483648->3, inflight:0000000000000040, rq:ffff888208c1e940

Where is this exact tracepoint? My grep skills are failing me.

>
> which is impossible!
>
> The answer is that as we keep the existing execlists->active pointing
> into the array as we copy over that array, the unserialised read may see
> a partial pointer value.

...otherwise we will see ?

Also, the 0x40 is bothering me as I didn't find the tracepoint. If we
only displayed pointer values, where did the offset appear. 

>
> Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine.h |  4 +---
>  drivers/gpu/drm/i915/gt/intel_lrc.c    | 24 ++++++++++++++----------
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index bc3b72bfa9e3..01765a7ec18f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -100,9 +100,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
>  static inline struct i915_request *
>  execlists_active(const struct intel_engine_execlists *execlists)
>  {
> -	GEM_BUG_ON(execlists->active - execlists->inflight >
> -		   execlists_num_ports(execlists));
> -	return READ_ONCE(*execlists->active);
> +	return *READ_ONCE(execlists->active);

Yes this seems proper as we need apriori read before deferencing.

>  }
>  
>  static inline void
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 0e2065a13f24..0d0dca3d6724 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2169,23 +2169,27 @@ static void process_csb(struct intel_engine_cs *engine)
>  		else
>  			promote = gen8_csb_parse(execlists, buf + 2 * head);
>  		if (promote) {
> +			struct i915_request * const *old = execlists->active;
> +
> +			/* Point active to the new ELSP; prevent overwriting */
> +			WRITE_ONCE(execlists->active, execlists->pending);
> +			set_timeslice(engine);

If we set the active to pending here...

> +
>  			if (!inject_preempt_hang(execlists))
>  				ring_set_paused(engine, 0);
>  
>  			/* cancel old inflight, prepare for switch */
> -			trace_ports(execlists, "preempted", execlists->active);
> -			while (*execlists->active)
> -				execlists_schedule_out(*execlists->active++);
> +			trace_ports(execlists, "preempted", old);
> +			while (*old)
> +				execlists_schedule_out(*old++);
>  
>  			/* switch pending to inflight */
>  			GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
> -			execlists->active =
> -				memcpy(execlists->inflight,
> -				       execlists->pending,
> -				       execlists_num_ports(execlists) *
> -				       sizeof(*execlists->pending));
> -
> -			set_timeslice(engine);
> +			WRITE_ONCE(execlists->active,
> +				   memcpy(execlists->inflight,
> +					  execlists->pending,
> +					  execlists_num_ports(execlists) *
> +					  sizeof(*execlists->pending)));

Why we rewrite it in here, is the pending moving beneath us?

-Mika

>  
>  			WRITE_ONCE(execlists->pending[0], NULL);
>  		} else {
> -- 
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx