Re: drm/i915: Watchdog timeout: DRM kernel interface to set the timeout

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

On 05/01/2019 02:39, Carlos Santa wrote:
> From: Michel Thierry <michel.thierry@intel.com>
> 
> Final enablement patch for GPU hang detection using watchdog timeout.
> Using the gem_context_setparam ioctl, users can specify the desired
> timeout value in microseconds, and the driver will do the conversion to
> 'timestamps'.
> 
> The recommended default watchdog threshold for video engines is 60000 us,
> since this has been _empirically determined_ to be a good compromise for
> low-latency requirements and low rate of false positives. The default
> register value is ~106000us and the theoretical max value (all 1s) is
> 353 seconds.
> 
> Note, UABI engine ids and i915 engine ids are different, and this patch
> uses the i915 ones. Some kind of mapping table [1] is required if we
> decide to use the UABI engine ids.

This paragraphs is not true any more.

> 
> [1] http://patchwork.freedesktop.org/patch/msgid/20170329135831.30254-2-chris@chris-wilson.co.uk
 >> v2: Fixed get api to return values in microseconds. Threshold updated to
> be per context engine. Check for u32 overflow. Capture ctx threshold
> value in error state.
> 
> v3: Add a way to get array size, short-cut to disable all thresholds,
> return EFAULT / EINVAL as needed. Move the capture of the threshold
> value in the error state into a new patch. BXT has a different
> timestamp base (because why not?).
> 
> v4: Checking if watchdog is available should be the first thing to
> do, instead of giving false hopes to abi users; remove unnecessary & in
> set_watchdog; ignore args->size in getparam.
> 
> v5: GEN9-LP platforms have a different crystal clock frequency, use the
> right timestamp base for them (magic 8-ball predicts this will change
> again later on, so future-proof it). (Daniele)
> 
> v6: Rebase, no more mutex BLK in getparam_ioctl.
> 
> v7: use to_intel_context instead of ctx->engine.
> 
> v8: Rebase, remove extra mutex from i915_gem_context_set_watchdog (Tvrtko),
> Update UAPI to use engine class while keeping thresholds per
> engine class (Michel).
> 
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Carlos Santa <carlos.santa@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         | 56 +++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_context.c | 91 +++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_lrc.c        |  5 +-
>   include/uapi/drm/i915_drm.h             |  1 +
>   4 files changed, 151 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7fa2a405c5fe..96d59c22e2ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1552,6 +1552,9 @@ struct drm_i915_private {
>   	struct drm_i915_fence_reg fence_regs[I915_MAX_NUM_FENCES]; /* assume 965 */
>   	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
>   
> +	/* Command stream timestamp base - helps define watchdog threshold */
> +	u32 cs_timestamp_base;
> +
>   	unsigned int fsb_freq, mem_freq, is_ddr3;
>   	unsigned int skl_preferred_vco_freq;
>   	unsigned int max_cdclk_freq;
> @@ -3117,6 +3120,59 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
>   	return ctx;
>   }
>   
> +/*
> + * BDW, CHV & SKL+ Timestamp timer resolution = 0.080 uSec,
> + * or 12500000 counts per second, or ~12 counts per microsecond.
> + *
> + * But BXT/GLK Timestamp timer resolution is different, 0.052 uSec,
> + * or 19200000 counts per second, or ~19 counts per microsecond.
> + *
> + * Future-proofing, some day it won't be as simple as just GEN & IS_LP.
> + */
> +#define GEN8_TIMESTAMP_CNTS_PER_USEC 12
> +#define GEN9_LP_TIMESTAMP_CNTS_PER_USEC 19
> +static inline u32 cs_timestamp_in_us(struct drm_i915_private *dev_priv)
> +{
> +	u32 cs_timestamp_base = dev_priv->cs_timestamp_base;
> +
> +	if (cs_timestamp_base)
> +		return cs_timestamp_base;
> +
> +	switch (INTEL_GEN(dev_priv)) {
> +	default:
> +		MISSING_CASE(INTEL_GEN(dev_priv));
> +		/* fall through */
> +	case 9:
> +		cs_timestamp_base = IS_GEN9_LP(dev_priv) ?
> +					GEN9_LP_TIMESTAMP_CNTS_PER_USEC :
> +					GEN8_TIMESTAMP_CNTS_PER_USEC;
> +		break;
> +	case 8:
> +		cs_timestamp_base = GEN8_TIMESTAMP_CNTS_PER_USEC;
> +		break;
> +	}
> +
> +	dev_priv->cs_timestamp_base = cs_timestamp_base;
> +	return cs_timestamp_base;
> +}

We already have RUNTIME_INFO(i915)->cs_timestamp_frequency_khz and 
read_timestamp_frequency which sets it.

> +
> +static inline u32
> +watchdog_to_us(struct drm_i915_private *dev_priv, u32 value_in_clock_counts)
> +{
> +	return value_in_clock_counts / cs_timestamp_in_us(dev_priv);
> +}
> +
> +static inline u32
> +watchdog_to_clock_counts(struct drm_i915_private *dev_priv, u64 value_in_us)
> +{
> +	u64 threshold = value_in_us * cs_timestamp_in_us(dev_priv);
> +
> +	if (overflows_type(threshold, u32))
> +		return -EINVAL;
> +
> +	return threshold;
> +}
> +
>   int i915_perf_open_ioctl(struct drm_device *dev, void *data,
>   			 struct drm_file *file);
>   int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 40fefed8c92f..4e421b79ac07 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -840,6 +840,91 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>   	return 0;
>   }
>   
> +/* Return the timer count threshold in microseconds. */
> +int i915_gem_context_get_watchdog(struct i915_gem_context *ctx,
> +				  struct drm_i915_gem_context_param *args)
> +{
> +	struct drm_i915_private *dev_priv = ctx->i915;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	u32 threshold_in_us[OTHER_CLASS];
> +
> +	if (!dev_priv->engine[VCS]->emit_start_watchdog)
> +		return -ENODEV;

This could use the engine->flags, but the question is which engine. The 
answer will depend on the uapi so we will come to it later.

> +
> +	for_each_engine(engine, dev_priv, id) {
> +		struct intel_context *ce = to_intel_context(ctx, engine);
> +
> +		threshold_in_us[engine->class] = watchdog_to_us(dev_priv,
> +								ce->watchdog_threshold);

Would there be some lossyness in read-back due us -> internal -> us 
conversion?

> +	}
> +
> +	if (__copy_to_user(u64_to_user_ptr(args->value),
> +			   &threshold_in_us,
> +			   sizeof(threshold_in_us))) {

I think user pointer hasn't been verified for write access yet so 
standard copy_to_user should be used.

> +		return -EFAULT;
> +	}
> +
> +	args->size = sizeof(threshold_in_us);
> +
> +	return 0;
> +}
> +
> +/*
> + * Based on time out value in microseconds (us) calculate
> + * timer count thresholds needed based on core frequency.
> + * Watchdog can be disabled by setting it to 0.
> + */
> +int i915_gem_context_set_watchdog(struct i915_gem_context *ctx,
> +				  struct drm_i915_gem_context_param *args)
> +{
> +	struct drm_i915_private *dev_priv = ctx->i915;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	u32 i;

Incorrect type for class iterator.

> +	u32 threshold[OTHER_CLASS];
> +
> +	if (!dev_priv->engine[VCS]->emit_start_watchdog)
> +		return -ENODEV;
> +
> +	memset(threshold, 0, sizeof(threshold));
> +
> +	/* shortcut to disable in all engines */
> +	if (args->size == 0)
> +		goto set_watchdog;
> +
> +	if (args->size < sizeof(threshold))
> +		return -EFAULT;

This would make the uapi not backward compatible. (Old userspace on new 
kernels). But uapi will probably need other changes as mentioned already.

> +
> +	if (copy_from_user(threshold,
> +			   u64_to_user_ptr(args->value),
> +			   sizeof(threshold))) {
> +		mutex_lock(&dev_priv->drm.struct_mutex);

Ooops!

> +		return -EFAULT;
> +	}
> +
> +	/* not supported in blitter engine */
> +	if (threshold[COPY_ENGINE_CLASS] != 0)
> +		return -EINVAL;
> +
> +	for (i = RENDER_CLASS; i < OTHER_CLASS; i++) {
> +		threshold[i] = watchdog_to_clock_counts(dev_priv, threshold[i]);
> +
> +		if (threshold[i] == -EINVAL)
> +			return -EINVAL;
> +	}
> +
> +set_watchdog:
> +	for_each_engine(engine, dev_priv, id) {
> +		struct intel_context *ce = to_intel_context(ctx, engine);
> +
> +		ce->watchdog_threshold = threshold[engine->class];
> +	}
> +
> +	return 0;
> +}
> +
> +
>   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>   				    struct drm_file *file)
>   {
> @@ -877,6 +962,9 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>   	case I915_CONTEXT_PARAM_PRIORITY:
>   		args->value = ctx->sched.priority >> I915_USER_PRIORITY_SHIFT;
>   		break;
> +	case I915_CONTEXT_PARAM_WATCHDOG:
> +		ret = i915_gem_context_get_watchdog(ctx, args);
> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> @@ -949,6 +1037,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>   		}
>   		break;
>   
> +	case I915_CONTEXT_PARAM_WATCHDOG:
> +		ret = i915_gem_context_set_watchdog(ctx, args);
> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0ea5a37c3357..0afcbeb18329 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2061,7 +2061,7 @@ static inline u32 get_watchdog_disable(struct intel_engine_cs *engine)
>   		return GEN8_XCS_WATCHDOG_DISABLE;
>   }
>   
> -#define GEN8_WATCHDOG_1000US 0x2ee0 //XXX: Temp, replace with helper function
> +#define GEN8_WATCHDOG_1000US(dev_priv) watchdog_to_clock_counts(dev_priv, 1000)

Right, hm, I think you should introduce a helper in that earlier patch 
to avoid the ugly comment.

I doesn't have to do anything yet. However as said in that review, I am 
not sure this approach of restarting the timer is safe to begin with.

>   static void gen8_watchdog_irq_handler(unsigned long data)
>   {
>   	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
> @@ -2108,7 +2108,8 @@ static void gen8_watchdog_irq_handler(unsigned long data)
>   	} else {
>   		engine->hangcheck.watchdog = current_seqno;
>   		/* Re-start the counter, if really hung, it will expire again */
> -		I915_WRITE_FW(RING_THRESH(engine->mmio_base), GEN8_WATCHDOG_1000US);
> +		I915_WRITE_FW(RING_THRESH(engine->mmio_base),
> +			      GEN8_WATCHDOG_1000US(dev_priv));
>   		I915_WRITE_FW(RING_CNTR(engine->mmio_base), GEN8_WATCHDOG_ENABLE);
>   	}
>   
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 0bc9e00e66ce..da7efaf66b0e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1490,6 +1490,7 @@ struct drm_i915_gem_context_param {
>   #define   I915_CONTEXT_MAX_USER_PRIORITY	1023 /* inclusive */
>   #define   I915_CONTEXT_DEFAULT_PRIORITY		0
>   #define   I915_CONTEXT_MIN_USER_PRIORITY	-1023 /* inclusive */
> +#define I915_CONTEXT_PARAM_WATCHDOG	0x7
>   	__u64 value;
>   };
>   
> 

So UAPI is not really defined at all as far as userspace (looking at 
i915_drm.h can see).

We will have to come up with something better and also considering the 
Virtual Engine work. And also work out the details semantics etc.

Regards,

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