drm/i915: modparam rework prep work

Abstract the one user in anticipation of more. Set the dangling pointers to NULL while at it.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c |  9 +--------
  drivers/gpu/drm/i915/i915_params.c    | 16 ++++++++++++++++
  drivers/gpu/drm/i915/i915_params.h    |  1 +
  3 files changed, 18 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_gpu_error.c +1 -8

index 8c04479c1586..2bd7991ec9af 100644
@@ -963,17 +963,10 @@ static void i915_error_object_free(struct drm_i915_error_object *obj)
  	kfree(obj);
  }
 
-static __always_inline void free_param(const char *type, void *x)
-{
-	if (!__builtin_strcmp(type, "char *"))
-		kfree(*(void **)x);
-}
 
  static void cleanup_params(struct i915_gpu_state *error)
  {
-#define FREE(T, x, ...) free_param(#T, &error->params.x);
-	I915_PARAMS_FOR_EACH(FREE);
-#undef FREE
+	i915_params_free(&error->params);
  }
 
  static void cleanup_uc_state(struct i915_gpu_state *error)

drivers/gpu/drm/i915/i915_params.c +16 -0

index ae3ece4ec7ab..81c73bfc7991 100644
@@ -217,3 +217,19 @@ void i915_params_copy(struct i915_params *dest, const struct i915_params *src)
  	I915_PARAMS_FOR_EACH(DUP);
  #undef DUP
  }
+
+static __always_inline void free_param(const char *type, void *x)
+{
+	if (!__builtin_strcmp(type, "char *")) {
+		kfree(*(void **)x);
+		*(void **)x = NULL;
+	}
+}
+
+/* free the allocated members, *not* the passed in params itself */
+void i915_params_free(struct i915_params *params)
+{
+#define FREE(T, x, ...) free_param(#T, &params->x);
+	I915_PARAMS_FOR_EACH(FREE);
+#undef FREE
+}

drivers/gpu/drm/i915/i915_params.h +0 -0

index fd1cf9415e60..93f665eced16 100644

Reveals the build fail fixed in the last hunk. Also prep work.

v2: name it i915 instead of dev_priv (Michal)

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h    |  6 +++---
  drivers/gpu/drm/i915/intel_uc.c    | 12 ++++++------
  drivers/gpu/drm/i915/intel_uc.h    |  6 +++---
  drivers/gpu/drm/i915/intel_wopcm.c |  2 +-
  4 files changed, 13 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +3 -3

index d44255a8655e..c60cf17e50a0 100644
@@ -2490,9 +2490,9 @@ intel_info(const struct drm_i915_private *dev_priv)
  #define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 
  /* Having a GuC is not the same as using a GuC */
-#define USES_GUC(dev_priv)		intel_uc_is_using_guc()
-#define USES_GUC_SUBMISSION(dev_priv)	intel_uc_is_using_guc_submission()
-#define USES_HUC(dev_priv)		intel_uc_is_using_huc()
+#define USES_GUC(dev_priv)		intel_uc_is_using_guc(dev_priv)
+#define USES_GUC_SUBMISSION(dev_priv)	intel_uc_is_using_guc_submission(dev_priv)
+#define USES_HUC(dev_priv)		intel_uc_is_using_huc(dev_priv)
 
  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
 

drivers/gpu/drm/i915/intel_uc.c +6 -6

index 447b1de77cc7..731b82afe636 100644
@@ -71,7 +71,7 @@ static int __get_default_guc_log_level(struct drm_i915_private *i915)
  {
  	int guc_log_level;
 
-	if (!HAS_GUC(i915) || !intel_uc_is_using_guc())
+	if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915))
  		guc_log_level = GUC_LOG_LEVEL_DISABLED;
  	else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
  		 IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))

@@ -112,11 +112,11 @@ static void sanitize_options_early(struct drm_i915_private *i915)
 
  	DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
  			 i915_modparams.enable_guc,
-			 yesno(intel_uc_is_using_guc_submission()),
-			 yesno(intel_uc_is_using_huc()));
+			 yesno(intel_uc_is_using_guc_submission(i915)),
+			 yesno(intel_uc_is_using_huc(i915)));
 
  	/* Verify GuC firmware availability */
-	if (intel_uc_is_using_guc() && !intel_uc_fw_is_selected(guc_fw)) {
+	if (intel_uc_is_using_guc(i915) && !intel_uc_fw_is_selected(guc_fw)) {
  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
  			 "enable_guc", i915_modparams.enable_guc,
  			 !HAS_GUC(i915) ? "no GuC hardware" :

@@ -124,7 +124,7 @@ static void sanitize_options_early(struct drm_i915_private *i915)
  	}
 
  	/* Verify HuC firmware availability */
-	if (intel_uc_is_using_huc() && !intel_uc_fw_is_selected(huc_fw)) {
+	if (intel_uc_is_using_huc(i915) && !intel_uc_fw_is_selected(huc_fw)) {
  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
  			 "enable_guc", i915_modparams.enable_guc,
  			 !HAS_HUC(i915) ? "no HuC hardware" :

@@ -136,7 +136,7 @@ static void sanitize_options_early(struct drm_i915_private *i915)
  		i915_modparams.guc_log_level =
  			__get_default_guc_log_level(i915);
 
-	if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc()) {
+	if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(i915)) {
  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
  			 "guc_log_level", i915_modparams.guc_log_level,
  			 !HAS_GUC(i915) ? "no GuC hardware" :

drivers/gpu/drm/i915/intel_uc.h +3 -3

index 25d73ada74ae..870faf9011b9 100644
@@ -41,19 +41,19 @@ void intel_uc_fini(struct drm_i915_private *dev_priv);
  int intel_uc_suspend(struct drm_i915_private *dev_priv);
  int intel_uc_resume(struct drm_i915_private *dev_priv);
 
-static inline bool intel_uc_is_using_guc(void)
+static inline bool intel_uc_is_using_guc(struct drm_i915_private *i915)
  {
  	GEM_BUG_ON(i915_modparams.enable_guc < 0);
  	return i915_modparams.enable_guc > 0;
  }
 
-static inline bool intel_uc_is_using_guc_submission(void)
+static inline bool intel_uc_is_using_guc_submission(struct drm_i915_private *i915)
  {
  	GEM_BUG_ON(i915_modparams.enable_guc < 0);
  	return i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION;
  }
 
-static inline bool intel_uc_is_using_huc(void)
+static inline bool intel_uc_is_using_huc(struct drm_i915_private *i915)
  {
  	GEM_BUG_ON(i915_modparams.enable_guc < 0);
  	return i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC;

drivers/gpu/drm/i915/intel_wopcm.c +0 -0

index 630c887682e8..f82a415ea2ba 100644

i915.enable_hangcheck has been an outlier since its introduction in commit 3e0dc6b01f53 ("drm/i915: hangcheck disable parameter") with 0644 permissions, while all the rest are either 0400 or 0600. Follow suit with 0600.

IGT never reads the value, so there should be no impact.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_params.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_params.c +0 -0

index 81c73bfc7991..9f0539bdaa39 100644

Quoted 18 lines · Jani Nikula

i915.enable_hangcheck has been an outlier since its introduction in commit 3e0dc6b01f53 ("drm/i915: hangcheck disable parameter") with 0644 permissions, while all the rest are either 0400 or 0600. Follow suit with 0600.

IGT never reads the value, so there should be no impact.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_params.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_params.c +0 -0

index 81c73bfc7991..9f0539bdaa39 100644

drivers/gpu/drm/i915/i915_params.c i915_param_named(error_capture, bool, 0600, · line 83

Quoted 8 lines

drivers/gpu/drm/i915/i915_params.c +1 -1

@@ -77,7 +77,7 @@ i915_param_named(error_capture, bool, 0600,
  	"triaging and debugging hangs.");
  #endif
 
-i915_param_named_unsafe(enable_hangcheck, bool, 0644,
+i915_param_named_unsafe(enable_hangcheck, bool, 0600,
  	"Periodically check GPU activity for detecting hangs. "
  	"WARNING: Disabling this can cause system wide hangs. "
  	"(default: true)");

Having dug out the introducing commit, there doesn't seem to be a special reason for it to be 0644 indeed:

commit 3e0dc6b01f5301d63046f6deddde2c7f5c57d67a
Author: Ben Widawsky <[email protected]>
Date: Wed Jun 29 10:26:42 2011 -0700

drm/i915: hangcheck disable parameter

Reviewed-by Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko

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 <[email protected]>

---
  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(-)

drivers/gpu/drm/i915/i915_drv.c +0 -25

index caa055ac9472..559a1b11f3e4 100644
@@ -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"

drivers/gpu/drm/i915/i915_drv.h +0 -15

index c60cf17e50a0..1f29f3933625 100644
@@ -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__)

drivers/gpu/drm/i915/i915_params.c +0 -5

index 9f0539bdaa39..2853b54570eb 100644
@@ -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)");
 

drivers/gpu/drm/i915/i915_params.h +0 -1

index 93f665eced16..85a9007c0ed6 100644
@@ -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) \

drivers/gpu/drm/i915/i915_selftest.h +10 -0

index a73472dd12fd..1266cef8bf17 100644
@@ -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.

drivers/gpu/drm/i915/selftests/i915_selftest.c +16 -0

index 86c54ea37f48..9e556fc4707d 100644
@@ -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;

Quoted 2 lines

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..

Quoted 3 lines

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.

Quoting Tvrtko Ursulin (2018-12-31 13:13:26)

Quoted 11 lines · Jani Nikula

Quoted 2 lines

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..

Quoted 3 lines

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.

Quoted 1 line

... 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.

Quoted 17 lines

Quoting Tvrtko Ursulin (2018-12-31 13:13:26)

Quoted 12 lines · Jani Nikula

Quoted 2 lines

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..

Quoted 3 lines

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/

Quoted 3 lines

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".

Macros with this much magic in them deserve some explanatory text.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_params.h | 9 +++++++++
  1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_params.h +0 -0

index 85a9007c0ed6..98eba6728095 100644
@@ -33,6 +33,15 @@ struct drm_printer;
  #define ENABLE_GUC_SUBMISSION		BIT(0)

Quoted 15 lines · Jani Nikula

Macros with this much magic in them deserve some explanatory text.

Signed-off-by Jani Nikula <[email protected]>

---
  drivers/gpu/drm/i915/i915_params.h | 9 +++++++++
  1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_params.h +0 -0

index 85a9007c0ed6..98eba6728095 100644
@@ -33,6 +33,15 @@ struct drm_printer;
  #define ENABLE_GUC_SUBMISSION		BIT(0)
  #define ENABLE_GUC_LOAD_HUC		BIT(1)
 

drivers/gpu/drm/i915/i915_params.h struct drm_printer; · line 43

Quoted 8 lines

drivers/gpu/drm/i915/i915_params.h +8 -0

+/*
+ * Invoke param, a function-like macro, for each i915 param, with arguments:
+ *
+ * param(type, name, value)
+ *
+ * type: parameter type, one of {bool, int, unsigned int, char *}
+ * name: name of the parameter
+ * value: initial/default value of the parameter

Deliberately not in kerneldoc format, or kerneldoc doesn't work for macros, or I am missing something else?

Regards,

Tvrtko

Quoted 5 lines

drivers/gpu/drm/i915/i915_params.h +1 -0

+ */
  #define I915_PARAMS_FOR_EACH(param) \
  	param(char *, vbt_firmware, NULL) \
  	param(int, modeset, -1) \

Quoted 16 lines · Tvrtko Ursulin

Quoted 15 lines · Jani Nikula

Macros with this much magic in them deserve some explanatory text.

Signed-off-by Jani Nikula <[email protected]>

---
   drivers/gpu/drm/i915/i915_params.h | 9 +++++++++
   1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_params.h +0 -0

index 85a9007c0ed6..98eba6728095 100644
@@ -33,6 +33,15 @@ struct drm_printer;
   #define ENABLE_GUC_SUBMISSION		BIT(0)
   #define ENABLE_GUC_LOAD_HUC		BIT(1)
 

Quoted 11 lines

drivers/gpu/drm/i915/i915_params.h struct drm_printer; · line 43

Quoted 8 lines

drivers/gpu/drm/i915/i915_params.h +8 -0

+/*
+ * Invoke param, a function-like macro, for each i915 param, with arguments:
+ *
+ * param(type, name, value)
+ *
+ * type: parameter type, one of {bool, int, unsigned int, char *}
+ * name: name of the parameter
+ * value: initial/default value of the parameter

Deliberately not in kerneldoc format, or kerneldoc doesn't work for macros, or I am missing something else?

The comment describes the parameters passed to the macro passed in as param, so it's too many levels of abstraction for poor kernel-doc. ;)

Quoted 29 lines · Jani Nikula

Quoted 27 lines · Tvrtko Ursulin

Quoted 13 lines · Jani Nikula

Macros with this much magic in them deserve some explanatory text.

Signed-off-by Jani Nikula <[email protected]>

---
   drivers/gpu/drm/i915/i915_params.h | 9 +++++++++
   1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_params.h +0 -0

index 85a9007c0ed6..98eba6728095 100644
@@ -33,6 +33,15 @@ struct drm_printer;
   #define ENABLE_GUC_SUBMISSION		BIT(0)

drivers/gpu/drm/i915/i915_params.h struct drm_printer; · line 43

Quoted 10 lines

drivers/gpu/drm/i915/i915_params.h +8 -0

   #define ENABLE_GUC_LOAD_HUC		BIT(1)
 
+/*
+ * Invoke param, a function-like macro, for each i915 param, with arguments:
+ *
+ * param(type, name, value)
+ *
+ * type: parameter type, one of {bool, int, unsigned int, char *}
+ * name: name of the parameter
+ * value: initial/default value of the parameter

Deliberately not in kerneldoc format, or kerneldoc doesn't work for macros, or I am missing something else?

Quoted 2 lines

The comment describes the parameters passed to the macro passed in as param, so it's too many levels of abstraction for poor kernel-doc. ;)

Ha! :)

Reviewed-by Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko