[Intel-gfx] [PATCH v6 17/20] drm/i915: Watchdog timeout: Ringbuffer command emission for gen8+

[Michel Thierry]

On 18/04/17 14:20, Chris Wilson wrote:
> On Tue, Apr 18, 2017 at 01:23:32PM -0700, Michel Thierry wrote:
>> @@ -1329,10 +1331,29 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
>>  		req->ctx->ppgtt->pd_dirty_rings &= ~intel_engine_flag(req->engine);
>>  	}
>>
>> -	cs = intel_ring_begin(req, 4);
>> +	/* bb_start only */
>> +	num_dwords = 4;
>> +
>> +	/* check if watchdog will be required */
>> +	if (req->ctx->engine[req->engine->id].watchdog_threshold != 0) {
>> +		if (!req->engine->emit_start_watchdog ||
>> +		    !req->engine->emit_stop_watchdog)
>> +			return -EINVAL;
>
> This is still a bug in the context setparam to get to this point without
> a watchdog.
>

This can't happen (threshold != 0 && no emit_watchdog func), 
i915_gem_context_set_watchdog returns -ENODEV if vcs's 
emit_start_watchdog is not defined (the assumption is if the vcs has it, 
rcs does too).

I can remove it, if that's what you mean.

But re i915_gem_context_set_watchdog, I think maybe it should return 
ENODEV when there's no watchdog and the user is trying to get the array 
size (args->size == 0), and don't give false hopes.

>> +
>> +		/* + start_watchdog (6) + stop_watchdog (4) */
>> +		num_dwords += 10;
>> +		watchdog_running = true;
>> +	}
>> +static u32 *gen8_emit_stop_watchdog(struct drm_i915_gem_request *req, u32 *cs)
>> +{
>> +	struct intel_engine_cs *engine = req->engine;
>> +
>> +	/* XXX: no watchdog support in BCS engine */
>> +	GEM_BUG_ON(engine->id == BCS);
>> +
>> +	*cs++ = MI_LOAD_REGISTER_IMM(2);
>> +	*cs++ = i915_mmio_reg_offset(RING_CNTR(engine->mmio_base));
>> +	*cs++ = get_watchdog_disable(engine);
>> +	*cs++ = MI_NOOP;
>
> Oops.

_context_set_watchdog also rejects if threshold[BCS] != 0.

> -Chris
>