Re: [PATCH 5/6] drm/i915: move load failure injection to selftests

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Mon, 31 Dec 2018 13:13:26 +0000

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@xxxxxxxxx>
---
  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;
+}

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







