diff mbox

[v6,17/20] drm/i915: Watchdog timeout: Ringbuffer command emission for gen8+

Message ID 20170418202335.35232-18-michel.thierry@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michel Thierry April 18, 2017, 8:23 p.m. UTC 
Emit the required commands into the ring buffer for starting and
stopping the watchdog timer before/after batch buffer start during
batch buffer submission.

v2: Support watchdog threshold per context engine, merge lri commands,
and move watchdog commands emission to emit_bb_start. Request space of
combined start_watchdog, bb_start and stop_watchdog to avoid any error
after emitting bb_start.

Signed-off-by: Tomas Elf <tomas.elf@intel.com>
Signed-off-by: Ian Lister <ian.lister@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.h |  4 ++
 drivers/gpu/drm/i915/intel_lrc.c        | 81 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |  4 ++
 3 files changed, 87 insertions(+), 2 deletions(-)

Comments

Chris Wilson April 18, 2017, 9:20 p.m. UTC | #1 
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.

> +
> +		/* + 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.
-Chris
Michel Thierry April 18, 2017, 9:36 p.m. UTC | #2 
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
>
Chris Wilson April 18, 2017, 11:06 p.m. UTC | #3 
On Tue, Apr 18, 2017 at 02:36:14PM -0700, Michel Thierry wrote:
> 
> 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.

Yes, we shouldn't be setting the watchdog threshold if the watchdog is
not available. GEM_BUG_ON() would be fine. Throwing a very, very late
EINVAL is disconcerting.
 
> 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.

Seems reasonable.

> >>+
> >>+		/* + 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.

LRI(2), but only setting one register not two.
-Chris
Michel Thierry April 18, 2017, 11:11 p.m. UTC | #4 
On 18/04/17 16:06, Chris Wilson wrote:
> On Tue, Apr 18, 2017 at 02:36:14PM -0700, Michel Thierry wrote:
>>
>> 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.
>
> Yes, we shouldn't be setting the watchdog threshold if the watchdog is
> not available. GEM_BUG_ON() would be fine. Throwing a very, very late
> EINVAL is disconcerting.
>
>> 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.
>
> Seems reasonable.
>
>>>> +
>>>> +		/* + 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.
>
> LRI(2), but only setting one register not two.

Oh... proof that most of the time I only copy+paste stuff.

> -Chris
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 4af2ab94558b..88700bdbb4e1 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -150,6 +150,10 @@  struct i915_gem_context {
 		u32 *lrc_reg_state;
 		u64 lrc_desc;
 		int pin_count;
+		/** watchdog_threshold: hw watchdog threshold value,
+		 * in clock counts
+		 */
+		u32 watchdog_threshold;
 		bool initialised;
 	} engine[I915_NUM_ENGINES];
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2263b9fb9b50..7a202e73ce9b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1310,6 +1310,8 @@  static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 			      const unsigned int flags)
 {
 	u32 *cs;
+	u32 num_dwords;
+	bool watchdog_running = false;
 	int ret;
 
 	/* Don't rely in hw updating PDPs, specially in lite-restore.
@@ -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;
+
+		/* + start_watchdog (6) + stop_watchdog (4) */
+		num_dwords += 10;
+		watchdog_running = true;
+	}
+
+	cs = intel_ring_begin(req, num_dwords);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
+	if (watchdog_running) {
+		/* Start watchdog timer */
+		cs = req->engine->emit_start_watchdog(req, cs);
+	}
+
 	/* FIXME(BDW): Address space and security selectors. */
 	*cs++ = MI_BATCH_BUFFER_START_GEN8 |
 		(flags & I915_DISPATCH_SECURE ? 0 : BIT(8)) |
@@ -1340,8 +1361,13 @@  static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	*cs++ = lower_32_bits(offset);
 	*cs++ = upper_32_bits(offset);
 	*cs++ = MI_NOOP;
-	intel_ring_advance(req, cs);
 
+	if (watchdog_running) {
+		/* Cancel watchdog timer */
+		cs = req->engine->emit_stop_watchdog(req, cs);
+	}
+
+	intel_ring_advance(req, cs);
 	return 0;
 }
 
@@ -1508,6 +1534,49 @@  static void gen8_watchdog_irq_handler(unsigned long data)
 	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
 }
 
+static u32 *gen8_emit_start_watchdog(struct drm_i915_gem_request *req, u32 *cs)
+{
+	struct intel_engine_cs *engine = req->engine;
+	struct i915_gem_context *ctx = req->ctx;
+	struct intel_context *ce = &ctx->engine[engine->id];
+
+	/* XXX: no watchdog support in BCS engine */
+	GEM_BUG_ON(engine->id == BCS);
+
+	/*
+	 * watchdog register must never be programmed to zero. This would
+	 * cause the watchdog counter to exceed and not allow the engine to
+	 * go into IDLE state
+	 */
+	GEM_BUG_ON(ce->watchdog_threshold == 0);
+
+	/* Set counter period */
+	*cs++ = MI_LOAD_REGISTER_IMM(2);
+	*cs++ = i915_mmio_reg_offset(RING_THRESH(engine->mmio_base));
+	*cs++ = ce->watchdog_threshold;
+	/* Start counter */
+	*cs++ = i915_mmio_reg_offset(RING_CNTR(engine->mmio_base));
+	*cs++ = GEN8_WATCHDOG_ENABLE;
+	*cs++ = MI_NOOP;
+
+	return cs;
+}
+
+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;
+
+	return cs;
+}
+
 /*
  * Reserve space for 2 NOOPs at the end of each request to be
  * used as a workaround for not being allowed to do lite
@@ -1776,6 +1845,8 @@  int logical_render_ring_init(struct intel_engine_cs *engine)
 	engine->emit_flush = gen8_emit_flush_render;
 	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
 	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
+	engine->emit_start_watchdog = gen8_emit_start_watchdog;
+	engine->emit_stop_watchdog = gen8_emit_stop_watchdog;
 
 	ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 	if (ret)
@@ -1799,6 +1870,12 @@  int logical_xcs_ring_init(struct intel_engine_cs *engine)
 {
 	logical_ring_setup(engine);
 
+	/* BCS engine does not have a watchdog-expired irq */
+	if (engine->id != BCS) {
+		engine->emit_start_watchdog = gen8_emit_start_watchdog;
+		engine->emit_stop_watchdog = gen8_emit_stop_watchdog;
+	}
+
 	return logical_ring_init(engine);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 927f0c06cbe6..0392bd5fd300 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -279,6 +279,10 @@  struct intel_engine_cs {
 
 	int		(*emit_flush)(struct drm_i915_gem_request *request,
 				      u32 mode);
+	u32 *		(*emit_start_watchdog)(struct drm_i915_gem_request *req,
+					       u32 *cs);
+	u32 *		(*emit_stop_watchdog)(struct drm_i915_gem_request *req,
+					      u32 *cs);
 #define EMIT_INVALIDATE	BIT(0)
 #define EMIT_FLUSH	BIT(1)
 #define EMIT_BARRIER	(EMIT_INVALIDATE | EMIT_FLUSH)