| Message ID | 3d744354188c7ae8f809b8b56c2857bd3846fdec.1545920737.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: modparam rework prep work | expand |
On 27/12/2018 14:33, Jani Nikula wrote: > Seems like selftests is a better home for everything related to load > failure injection, including the module parameter. Hm not sure I would immediately want to couple the two, however.. > The failure injection code gets moved from under DRM_I915_DEBUG to > DRM_I915_SELFTEST config option. This should be of no consequence, as > the former selects the latter. ... I also don't see why debug would auto-select self-tests. So don't know. Personally I'd have selftests separate from debug, and load failure injection separate from selftests. But I don't feel that strongly about it. At least I agree that losing failure injection from the error state is not an issue. (Since all of them are strictly during driver load phase.) Regards, Tvrtko > > With the parameter no longer part of i915_params, its value will not be > recorded in error capture or debugfs param dump. Note that the value > would have been zeroed anyway if a selftest had been hit, so there > should not be meaningful information loss here, especially with all the > logging around failure injection. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 25 ------------------------- > drivers/gpu/drm/i915/i915_drv.h | 15 --------------- > drivers/gpu/drm/i915/i915_params.c | 5 ----- > drivers/gpu/drm/i915/i915_params.h | 1 - > drivers/gpu/drm/i915/i915_selftest.h | 10 ++++++++++ > drivers/gpu/drm/i915/selftests/i915_selftest.c | 26 ++++++++++++++++++++++++++ > 6 files changed, 36 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index caa055ac9472..559a1b11f3e4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -57,31 +57,6 @@ > > static struct drm_driver driver; > > -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > -static unsigned int i915_load_fail_count; > - > -bool __i915_inject_load_failure(const char *func, int line) > -{ > - if (i915_load_fail_count >= i915_modparams.inject_load_failure) > - return false; > - > - if (++i915_load_fail_count == i915_modparams.inject_load_failure) { > - DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n", > - i915_modparams.inject_load_failure, func, line); > - i915_modparams.inject_load_failure = 0; > - return true; > - } > - > - return false; > -} > - > -bool i915_error_injected(void) > -{ > - return i915_load_fail_count && !i915_modparams.inject_load_failure; > -} > - > -#endif > - > #define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI" > #define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \ > "providing the dmesg log by booting with drm.debug=0xf" > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c60cf17e50a0..1f29f3933625 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -111,21 +111,6 @@ > #define I915_STATE_WARN_ON(x) \ > I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") > > -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > - > -bool __i915_inject_load_failure(const char *func, int line); > -#define i915_inject_load_failure() \ > - __i915_inject_load_failure(__func__, __LINE__) > - > -bool i915_error_injected(void); > - > -#else > - > -#define i915_inject_load_failure() false > -#define i915_error_injected() false > - > -#endif > - > #define i915_load_error(i915, fmt, ...) \ > __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ > fmt, ##__VA_ARGS__) > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 9f0539bdaa39..2853b54570eb 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -159,11 +159,6 @@ i915_param_named_unsafe(dmc_firmware_path, charp, 0400, > i915_param_named_unsafe(enable_dp_mst, bool, 0600, > "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); > > -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > -i915_param_named_unsafe(inject_load_failure, uint, 0400, > - "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); > -#endif > - > i915_param_named(enable_dpcd_backlight, bool, 0600, > "Enable support for DPCD backlight control (default:false)"); > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 93f665eced16..85a9007c0ed6 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -53,7 +53,6 @@ struct drm_printer; > param(int, mmio_debug, 0) \ > param(int, edp_vswing, 0) \ > param(int, reset, 2) \ > - param(unsigned int, inject_load_failure, 0) \ > /* leave bools at the end to not create holes */ \ > param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \ > param(bool, enable_hangcheck, true) \ > diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h > index a73472dd12fd..1266cef8bf17 100644 > --- a/drivers/gpu/drm/i915/i915_selftest.h > +++ b/drivers/gpu/drm/i915/i915_selftest.h > @@ -33,6 +33,7 @@ struct i915_selftest { > unsigned int random_seed; > int mock; > int live; > + unsigned int inject_load_failure; > }; > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > @@ -77,6 +78,12 @@ int __i915_subtests(const char *caller, > #define I915_SELFTEST_DECLARE(x) x > #define I915_SELFTEST_ONLY(x) unlikely(x) > > +bool __i915_inject_load_failure(const char *func, int line); > +#define i915_inject_load_failure() \ > + __i915_inject_load_failure(__func__, __LINE__) > + > +bool i915_error_injected(void); > + > #else /* !IS_ENABLED(CONFIG_DRM_I915_SELFTEST) */ > > static inline int i915_mock_selftests(void) { return 0; } > @@ -85,6 +92,9 @@ static inline int i915_live_selftests(struct pci_dev *pdev) { return 0; } > #define I915_SELFTEST_DECLARE(x) > #define I915_SELFTEST_ONLY(x) 0 > > +static inline int i915_inject_load_failure(void) { return false; } > +static inline int i915_error_injected(void) { return false; } > + > #endif > > /* Using the i915_selftest_ prefix becomes a little unwieldy with the helpers. > diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c > index 86c54ea37f48..9e556fc4707d 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c > +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c > @@ -250,3 +250,29 @@ MODULE_PARM_DESC(mock_selftests, "Run selftests before loading, using mock hardw > > module_param_named_unsafe(live_selftests, i915_selftest.live, int, 0400); > MODULE_PARM_DESC(live_selftests, "Run selftests after driver initialisation on the live system (0:disabled [default], 1:run tests then continue, -1:run tests then exit module)"); > + > +module_param_named_unsafe(inject_load_failure, i915_selftest.inject_load_failure, uint, 0400); > +MODULE_PARM_DESC(inject_load_failure, > + "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); > + > +static unsigned int i915_load_fail_count; > + > +bool __i915_inject_load_failure(const char *func, int line) > +{ > + if (i915_load_fail_count >= i915_selftest.inject_load_failure) > + return false; > + > + if (++i915_load_fail_count == i915_selftest.inject_load_failure) { > + DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n", > + i915_selftest.inject_load_failure, func, line); > + i915_selftest.inject_load_failure = 0; > + return true; > + } > + > + return false; > +} > + > +bool i915_error_injected(void) > +{ > + return i915_load_fail_count && !i915_selftest.inject_load_failure; > +} >
Quoting Tvrtko Ursulin (2018-12-31 13:13:26) > > On 27/12/2018 14:33, Jani Nikula wrote: > > Seems like selftests is a better home for everything related to load > > failure injection, including the module parameter. > > Hm not sure I would immediately want to couple the two, however.. > > > The failure injection code gets moved from under DRM_I915_DEBUG to > > DRM_I915_SELFTEST config option. This should be of no consequence, as > > the former selects the latter. > > ... I also don't see why debug would auto-select self-tests. DRM_I915_DEBUG is now the list of things we use for igt s/DRM_I915_DEBUG/DRM_I915_IGT/ Just so we can have a single switch for "if you want to run igt, enable this CONFIG to enable all debug features used by igt". Handy for CI testing when more features are required. -Chris
On Mon, 31 Dec 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Tvrtko Ursulin (2018-12-31 13:13:26) >> >> On 27/12/2018 14:33, Jani Nikula wrote: >> > Seems like selftests is a better home for everything related to load >> > failure injection, including the module parameter. >> >> Hm not sure I would immediately want to couple the two, however.. >> >> > The failure injection code gets moved from under DRM_I915_DEBUG to >> > DRM_I915_SELFTEST config option. This should be of no consequence, as >> > the former selects the latter. >> >> ... I also don't see why debug would auto-select self-tests. > > DRM_I915_DEBUG is now the list of things we use for igt > s/DRM_I915_DEBUG/DRM_I915_IGT/ > > Just so we can have a single switch for "if you want to run igt, enable > this CONFIG to enable all debug features used by igt". Handy for CI > testing when more features are required. So what say you on the patch? I can keep it in i915_params.c too, it just needs some extra juggling because it gets used before i915 gets allocated. So not very easy as a "device param". BR, Jani.
Quoting Jani Nikula (2018-12-31 15:12:41) > On Mon, 31 Dec 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Tvrtko Ursulin (2018-12-31 13:13:26) > >> > >> On 27/12/2018 14:33, Jani Nikula wrote: > >> > Seems like selftests is a better home for everything related to load > >> > failure injection, including the module parameter. > >> > >> Hm not sure I would immediately want to couple the two, however.. > >> > >> > The failure injection code gets moved from under DRM_I915_DEBUG to > >> > DRM_I915_SELFTEST config option. This should be of no consequence, as > >> > the former selects the latter. > >> > >> ... I also don't see why debug would auto-select self-tests. > > > > DRM_I915_DEBUG is now the list of things we use for igt > > s/DRM_I915_DEBUG/DRM_I915_IGT/ > > > > Just so we can have a single switch for "if you want to run igt, enable > > this CONFIG to enable all debug features used by igt". Handy for CI > > testing when more features are required. > > So what say you on the patch? I can keep it in i915_params.c too, it > just needs some extra juggling because it gets used before i915 gets > allocated. So not very easy as a "device param". I had no objections to the patch. When you raised the idea, I was thinking of doing the entire iterative fault injection inside a selftest and remove the kernel parameter entirely :) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index caa055ac9472..559a1b11f3e4 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -57,31 +57,6 @@ static struct drm_driver driver; -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) -static unsigned int i915_load_fail_count; - -bool __i915_inject_load_failure(const char *func, int line) -{ - if (i915_load_fail_count >= i915_modparams.inject_load_failure) - return false; - - if (++i915_load_fail_count == i915_modparams.inject_load_failure) { - DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n", - i915_modparams.inject_load_failure, func, line); - i915_modparams.inject_load_failure = 0; - return true; - } - - return false; -} - -bool i915_error_injected(void) -{ - return i915_load_fail_count && !i915_modparams.inject_load_failure; -} - -#endif - #define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI" #define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \ "providing the dmesg log by booting with drm.debug=0xf" diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c60cf17e50a0..1f29f3933625 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -111,21 +111,6 @@ #define I915_STATE_WARN_ON(x) \ I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) - -bool __i915_inject_load_failure(const char *func, int line); -#define i915_inject_load_failure() \ - __i915_inject_load_failure(__func__, __LINE__) - -bool i915_error_injected(void); - -#else - -#define i915_inject_load_failure() false -#define i915_error_injected() false - -#endif - #define i915_load_error(i915, fmt, ...) \ __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ fmt, ##__VA_ARGS__) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 9f0539bdaa39..2853b54570eb 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -159,11 +159,6 @@ i915_param_named_unsafe(dmc_firmware_path, charp, 0400, i915_param_named_unsafe(enable_dp_mst, bool, 0600, "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) -i915_param_named_unsafe(inject_load_failure, uint, 0400, - "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); -#endif - i915_param_named(enable_dpcd_backlight, bool, 0600, "Enable support for DPCD backlight control (default:false)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 93f665eced16..85a9007c0ed6 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -53,7 +53,6 @@ struct drm_printer; param(int, mmio_debug, 0) \ param(int, edp_vswing, 0) \ param(int, reset, 2) \ - param(unsigned int, inject_load_failure, 0) \ /* leave bools at the end to not create holes */ \ param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \ param(bool, enable_hangcheck, true) \ diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h index a73472dd12fd..1266cef8bf17 100644 --- a/drivers/gpu/drm/i915/i915_selftest.h +++ b/drivers/gpu/drm/i915/i915_selftest.h @@ -33,6 +33,7 @@ struct i915_selftest { unsigned int random_seed; int mock; int live; + unsigned int inject_load_failure; }; #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) @@ -77,6 +78,12 @@ int __i915_subtests(const char *caller, #define I915_SELFTEST_DECLARE(x) x #define I915_SELFTEST_ONLY(x) unlikely(x) +bool __i915_inject_load_failure(const char *func, int line); +#define i915_inject_load_failure() \ + __i915_inject_load_failure(__func__, __LINE__) + +bool i915_error_injected(void); + #else /* !IS_ENABLED(CONFIG_DRM_I915_SELFTEST) */ static inline int i915_mock_selftests(void) { return 0; } @@ -85,6 +92,9 @@ static inline int i915_live_selftests(struct pci_dev *pdev) { return 0; } #define I915_SELFTEST_DECLARE(x) #define I915_SELFTEST_ONLY(x) 0 +static inline int i915_inject_load_failure(void) { return false; } +static inline int i915_error_injected(void) { return false; } + #endif /* Using the i915_selftest_ prefix becomes a little unwieldy with the helpers. diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c index 86c54ea37f48..9e556fc4707d 100644 --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c @@ -250,3 +250,29 @@ MODULE_PARM_DESC(mock_selftests, "Run selftests before loading, using mock hardw module_param_named_unsafe(live_selftests, i915_selftest.live, int, 0400); MODULE_PARM_DESC(live_selftests, "Run selftests after driver initialisation on the live system (0:disabled [default], 1:run tests then continue, -1:run tests then exit module)"); + +module_param_named_unsafe(inject_load_failure, i915_selftest.inject_load_failure, uint, 0400); +MODULE_PARM_DESC(inject_load_failure, + "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); + +static unsigned int i915_load_fail_count; + +bool __i915_inject_load_failure(const char *func, int line) +{ + if (i915_load_fail_count >= i915_selftest.inject_load_failure) + return false; + + if (++i915_load_fail_count == i915_selftest.inject_load_failure) { + DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n", + i915_selftest.inject_load_failure, func, line); + i915_selftest.inject_load_failure = 0; + return true; + } + + return false; +} + +bool i915_error_injected(void) +{ + return i915_load_fail_count && !i915_selftest.inject_load_failure; +}
Seems like selftests is a better home for everything related to load failure injection, including the module parameter. The failure injection code gets moved from under DRM_I915_DEBUG to DRM_I915_SELFTEST config option. This should be of no consequence, as the former selects the latter. With the parameter no longer part of i915_params, its value will not be recorded in error capture or debugfs param dump. Note that the value would have been zeroed anyway if a selftest had been hit, so there should not be meaningful information loss here, especially with all the logging around failure injection. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 25 ------------------------- drivers/gpu/drm/i915/i915_drv.h | 15 --------------- drivers/gpu/drm/i915/i915_params.c | 5 ----- drivers/gpu/drm/i915/i915_params.h | 1 - drivers/gpu/drm/i915/i915_selftest.h | 10 ++++++++++ drivers/gpu/drm/i915/selftests/i915_selftest.c | 26 ++++++++++++++++++++++++++ 6 files changed, 36 insertions(+), 46 deletions(-)