[Intel-gfx] [PATCH] drm/i915: Park signaling thread while wrapping the seqno

[Tvrtko Ursulin]

On 24/10/2018 11:49, Chris Wilson wrote:
> A danger encountered when resetting the seqno (using
> debugfs/i915_next_seqno) is that as we change the breadcrumb stored in
> the HWSP, it may be inspected by the signaler thread leading to
> confusion in our sanity checks.
> 
> <0> [136.331342] i915/sig-347     3..s1 136336154us : execlists_submission_tasklet: rcs0 awake?=1, active=5
> <0> [136.331373] i915/sig-347     3d.s2 136336155us : process_csb: rcs0 cs-irq head=5, tail=0
> <0> [136.331402] i915/sig-347     3d.s2 136336155us : process_csb: rcs0 csb[0]: status=0x00000018:0x00000002, active=0x5
> <0> [136.331434] i915/sig-347     3d.s2 136336156us : process_csb: rcs0 out[0]: ctx=2.1, global=219 (fence 46:8455) (current 219), prio=0
> <0> [136.331466] i915/sig-347     3d.s2 136336156us : process_csb: rcs0 completed ctx=2
> <0> [136.332027] gem_exec-1049    0.... 136336246us : reset_all_global_seqno.part.5: rcs0 seqno 219 (current 219) -> -43
> <0> [136.332056] gem_exec-1049    0.... 136336251us : reset_all_global_seqno.part.5: bcs0 seqno 183 (current 183) -> -43
> <0> [136.332085] gem_exec-1049    0.... 136336255us : reset_all_global_seqno.part.5: vcs0 seqno 191 (current 191) -> -43
> <0> [136.332114] gem_exec-1049    0.... 136336259us : reset_all_global_seqno.part.5: vcs1 seqno 180 (current 180) -> -43
> <0> [136.332143] gem_exec-1049    0.... 136336262us : reset_all_global_seqno.part.5: vecs0 seqno 212 (current 212) -> -43
> <0> [136.332174] i915/sig-347     3.... 136336280us : intel_breadcrumbs_signaler: intel_breadcrumbs_signaler:673 GEM_BUG_ON(!i915_request_completed(rq))

Why this can't happen with normal seqno wrap? And if it can, do we need 
a fixes tag?

> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 28819f8c4da6..71107540581d 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -136,6 +136,8 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
>   			  intel_engine_get_seqno(engine),
>   			  seqno);
>   
> +		kthread_park(engine->breadcrumbs.signaler);
> +

Needed even if increasing the seqno? It wouldn't work to do it under the 
i915_seqno_passed below?

>   		if (!i915_seqno_passed(seqno, engine->timeline.seqno)) {
>   			/* Flush any waiters before we reuse the seqno */
>   			intel_engine_disarm_breadcrumbs(engine);
> @@ -150,6 +152,8 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
>   		/* Finally reset hw state */
>   		intel_engine_init_global_seqno(engine, seqno);
>   		engine->timeline.seqno = seqno;
> +
> +		kthread_unpark(engine->breadcrumbs.signaler);
>   	}
>   
>   	list_for_each_entry(timeline, &i915->gt.timelines, link)
> 

Regards,

Tvrtko