Re: drm/i915/watchdog: move emit_stop_watchdog until the very end of the ring commands

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 05/01/2019 02:40, Carlos Santa wrote:
> From: Michel Thierry <michel.thierry@intel.com>
> 
> On command streams that could potentially hang the GPU after a last
> flush command, it's best not to cancel the watchdog
> until after all commands have executed.
> 
> Patch shared by Michel Thierry through IIRC after reproduction on

Joonas pointed out on IRC that IRC is called IRC. :)

> my local setup.
> 
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> CC: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Carlos Santa <carlos.santa@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 53 +++++++++++++++++++++++++++-----
>   1 file changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0afcbeb18329..25ba5fcc9466 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1885,8 +1885,8 @@ static int gen8_emit_bb_start(struct i915_request *rq,
>   		GEM_BUG_ON(!engine->emit_start_watchdog ||
>   			   !engine->emit_stop_watchdog);
>   
> -		/* + start_watchdog (6) + stop_watchdog (4) */
> -		num_dwords += 10;
> +		/* + start_watchdog (6) */
> +		num_dwords += 6;
>   		watchdog_running = true;
>           }
>   
> @@ -1927,10 +1927,7 @@ static int gen8_emit_bb_start(struct i915_request *rq,
>   	*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
>   	*cs++ = MI_NOOP;
>   
> -	if (watchdog_running) {
> -		/* Cancel watchdog timer */
> -		cs = engine->emit_stop_watchdog(rq, cs);
> -	}
> +	// XXX: emit_stop_watchdog happens in gen8_emit_breadcrumb_vcs

No C++ comments please. And what does XXX mean? Doesn't feel like it 
belongs.

>   
>   	intel_ring_advance(rq, cs);
>   	return 0;
> @@ -2189,6 +2186,37 @@ static void gen8_emit_breadcrumb(struct i915_request *request, u32 *cs)
>   }
>   static const int gen8_emit_breadcrumb_sz = 6 + WA_TAIL_DWORDS;
>   
> +static void gen8_emit_breadcrumb_vcs(struct i915_request *request, u32 *cs)
> +{
> +	/* w/a: bit 5 needs to be zero for MI_FLUSH_DW address. */
> +	BUILD_BUG_ON(I915_GEM_HWS_INDEX_ADDR & (1 << 5));
> +
> +	cs = gen8_emit_ggtt_write(cs, request->global_seqno,
> +				  intel_hws_seqno_address(request->engine));
> +	*cs++ = MI_USER_INTERRUPT;
> +	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> +
> +	// stop_watchdog at the very end of the ring commands
> +	if (request->gem_context->__engine[VCS].watchdog_threshold != 0)

VCS is wrong. Whole check needs to be to_intel_context(ctx, 
engine)->watchdog_threshold I think.

> +	{
> +		/* Cancel watchdog timer */
> +		GEM_BUG_ON(!request->engine->emit_stop_watchdog);
> +		cs = request->engine->emit_stop_watchdog(request, cs);
> +	}
> +	else
> +	{

Coding style is wrong (curly braces for if else).

> +		*cs++ = MI_NOOP;
> +		*cs++ = MI_NOOP;
> +		*cs++ = MI_NOOP;
> +		*cs++ = MI_NOOP;
> +	}
> +
> +	request->tail = intel_ring_offset(request, cs);
> +	assert_ring_tail_valid(request->ring, request->tail);
> +	gen8_emit_wa_tail(request, cs);
> +}
> +static const int gen8_emit_breadcrumb_vcs_sz = 6 + WA_TAIL_DWORDS + 4; //+4 for optional stop_watchdog
> +
>   static void gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs)
>   {
>   	/* We're using qword write, seqno should be aligned to 8 bytes. */
> @@ -2306,8 +2334,17 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>   	engine->request_alloc = execlists_request_alloc;
>   
>   	engine->emit_flush = gen8_emit_flush;
> -	engine->emit_breadcrumb = gen8_emit_breadcrumb;
> -	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_sz;
> +
> +	if (engine->id == VCS || engine->id == VCS2)

What about VCS3 or 4? Hint use engine class.

And what about RCS and VECS?

> +	{
> +		engine->emit_breadcrumb = gen8_emit_breadcrumb_vcs;
> +		engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_vcs_sz;
> +	}
> +	else
> +	{
> +		engine->emit_breadcrumb = gen8_emit_breadcrumb;
> +		engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_sz;
> +	}
>   
>   	engine->set_default_submission = intel_execlists_set_default_submission;
>   
> 

Looks like the patch should be squashed with the one which implements 
watchdog emit start/end? I mean if the whole setup has broken edge cases 
without this..

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx