| Message ID | 20190105024001.37629-5-carlos.santa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Watchdog timeout: DRM kernel interface to set the timeout | expand |
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
Quoting Tvrtko Ursulin (2019-01-07 12:38:47) > On 05/01/2019 02:39, Carlos Santa wrote: > > +/* 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) > > + 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. The starting point here is that this bakes in OTHER_CLASS as uABI when clearly it is not uABI. The array must be variable size and so the first pass is to report the size to userspace (if they pass args->size = 0) and then to copy up to the user requested size. Since it is a variable array with uncertain indexing, the output should be along the lines of [class, instance, watchdog]. But what about virtual engine? Good question. (Remember set must follow the same pattern as get, so you can always do a rmw cleanly.) I guess we just have to accept class == -1 as meaning use instance as an index into ctx->engines[]. Tvrtko? Virtual engine would be reported as (-1, 0, watchdog), and settable similarly with the other engines being addressable either by index or by class-instance. -Chris
On 07/01/2019 12:50, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-01-07 12:38:47) >> On 05/01/2019 02:39, Carlos Santa wrote: >>> +/* 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) >>> + 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. > > The starting point here is that this bakes in OTHER_CLASS as uABI when > clearly it is not uABI. The array must be variable size and so the first > pass is to report the size to userspace (if they pass args->size = 0) > and then to copy up to the user requested size. Since it is a variable > array with uncertain indexing, the output should be along the lines of > [class, instance, watchdog]. But what about virtual engine? Good > question. (Remember set must follow the same pattern as get, so you can > always do a rmw cleanly.) > > I guess we just have to accept class == -1 as meaning use instance as an > index into ctx->engines[]. Tvrtko? Virtual engine would be reported as > (-1, 0, watchdog), and settable similarly with the other engines being > addressable either by index or by class-instance. Or use index based addressing mode if engine map is set? Index 0 only being valid if load balancing is enabled on top. So an array of structs like: struct drm_i915_watchdog_timeout { union { struct { u16 class; u16 instance; }; u32 index; }; u32 timeout_us; }; ? We can allow standalone set param and via extension as well. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-01-07 13:39:24) > > On 07/01/2019 12:50, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-01-07 12:38:47) > >> On 05/01/2019 02:39, Carlos Santa wrote: > >>> +/* 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) > >>> + 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. > > > > The starting point here is that this bakes in OTHER_CLASS as uABI when > > clearly it is not uABI. The array must be variable size and so the first > > pass is to report the size to userspace (if they pass args->size = 0) > > and then to copy up to the user requested size. Since it is a variable > > array with uncertain indexing, the output should be along the lines of > > [class, instance, watchdog]. But what about virtual engine? Good > > question. (Remember set must follow the same pattern as get, so you can > > always do a rmw cleanly.) > > > > I guess we just have to accept class == -1 as meaning use instance as an > > index into ctx->engines[]. Tvrtko? Virtual engine would be reported as > > (-1, 0, watchdog), and settable similarly with the other engines being > > addressable either by index or by class-instance. > > Or use index based addressing mode if engine map is set? Index 0 only > being valid if load balancing is enabled on top. I was trying to avoid tying ctx->engines[] into such an extensive set of API changes, but given that it affects execbuffer_ioctl, the die is cast. With that approach, every time we use class-instance (with respect to a context) we should also swap it over to using an index when ctx->engines[] is set. The advantage to using a flag (in this case instance=-1) is that we can then write igt (or library routines) oblivious to the state of ctx->engines[]. From an application pov, they can ignore it, as they will be doing the SET_ENGINES as part of their initial context construction (one presumes) and so will always have ctx->engines[] known to be active. -Chris
On 07/01/2019 12:38, Tvrtko Ursulin wrote: [snip] >> +#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. Here I missed the mark, please ignore. Regards, Tvrtko
On 07/01/2019 17:00, Tvrtko Ursulin wrote: > > On 07/01/2019 12:38, Tvrtko Ursulin wrote: > > [snip] > >>> +#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. > > Here I missed the mark, please ignore. One thing I am not sure though is whether CTC_MODE register affects this clock. In other words should this code read it to get the correct effective tick duration. Bspec chain of Watchdog Timers (14970) -> Reported Timestamp Count (12610) -> Timestamp Bases (13569) does not answer that for me. Regards, Tvrtko
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; +} + +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; + + 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); + } + + if (__copy_to_user(u64_to_user_ptr(args->value), + &threshold_in_us, + sizeof(threshold_in_us))) { + 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; + 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; + + if (copy_from_user(threshold, + u64_to_user_ptr(args->value), + sizeof(threshold))) { + mutex_lock(&dev_priv->drm.struct_mutex); + 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) 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; };