[PATCH v2 3/3] drm/xe: Normalize default param values

[Lucas De Marchi]

On Mon, Jun 16, 2025 at 11:46:57AM -0700, John Harrison wrote:
>On 6/13/2025 1:00 PM, Lucas De Marchi wrote:
>>Document xe module params with the default values following a similar
>>strategy for all of them:
>>
>>	1) Define a DEFAULT_* macro with the default value. When the
>>	   value can't be directly stringified, also define a *_STR
>>	   variant
>>	2) Use __stringify() or the _STR variant to make sure the
>>	   default value shows up in the param description
>>
>>This allows us to show the correct default according to the
>>configuration. max_vfs for example was wrongly documented for
>>CONFIG_DRM_XE_DEBUG and svm_notifier_size didn't have its default
>>documented.
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>---
>>  drivers/gpu/drm/xe/xe_module.c | 44 +++++++++++++++++++++++++++---------------
>>  1 file changed, 28 insertions(+), 16 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
>>index 192908fa074cb..2068c579d5c3b 100644
>>--- a/drivers/gpu/drm/xe/xe_module.c
>>+++ b/drivers/gpu/drm/xe/xe_module.c
>>@@ -19,34 +19,43 @@
>>  #include "xe_sched_job.h"
>>  #if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
>>-#define DEFAULT_GUC_LOG_LEVEL	3
>>+#define DEFAULT_GUC_LOG_LEVEL		3
>>+#define DEFAULT_MAX_VFS			~0
>>+#define DEFAULT_MAX_VFS_STR		"unlimited"
>>  #else
>>-#define DEFAULT_GUC_LOG_LEVEL	1
>>+#define DEFAULT_GUC_LOG_LEVEL		1
>>+#define DEFAULT_MAX_VFS			0
>>+#define DEFAULT_MAX_VFS_STR		"0"
>Could be 'stringify(MAX_VFS)'.

I think it's better to pair with the one above.
We don't want to print the the default is ~0 in the other case so we
create a manual _STR.


>
>>  #endif
>>+#define DEFAULT_PROBE_DISPLAY		true
>>+#define DEFAULT_FORCE_PROBE		CONFIG_DRM_XE_FORCE_PROBE
>>+#define DEFAULT_WEDGED_MODE		1
>>+#define DEFAULT_SVM_NOTIFIER_SIZE	512
>>+
>>  struct xe_modparam xe_modparam = {
>>-	.probe_display = true,
>>-	.guc_log_level = DEFAULT_GUC_LOG_LEVEL,
>>-	.force_probe = CONFIG_DRM_XE_FORCE_PROBE,
>>-#ifdef CONFIG_PCI_IOV
>This ifdef is lost from the new version?

yeah, dropped it by mistake... will need to add it back.
>
>>-	.max_vfs = IS_ENABLED(CONFIG_DRM_XE_DEBUG) ? ~0 : 0,
>>-#endif
>>-	.wedged_mode = 1,
>>-	.svm_notifier_size = 512,
>>+	.probe_display =	DEFAULT_PROBE_DISPLAY,
>>+	.guc_log_level =	DEFAULT_GUC_LOG_LEVEL,
>>+	.force_probe =		DEFAULT_FORCE_PROBE,
>>+	.max_vfs =		DEFAULT_MAX_VFS,
>>+	.wedged_mode =		DEFAULT_WEDGED_MODE,
>>+	.svm_notifier_size =	DEFAULT_SVM_NOTIFIER_SIZE,
>>  	/* the rest are 0 by default */
>>  };
>>  module_param_named(svm_notifier_size, xe_modparam.svm_notifier_size, uint, 0600);
>>-MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier size(in MiB), must be power of 2");
>>+MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier size in MiB, must be power of 2 "
>>+		 "[default=" __stringify(DEFAULT_SVM_NOTIFIER_SIZE) "]");
>>  module_param_named_unsafe(force_execlist, xe_modparam.force_execlist, bool, 0444);
>>  MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
>>  module_param_named(probe_display, xe_modparam.probe_display, bool, 0444);
>>-MODULE_PARM_DESC(probe_display, "Probe display HW, otherwise it's left untouched (default: true)");
>>+MODULE_PARM_DESC(probe_display, "Probe display HW, otherwise it's left untouched "
>>+		 "[default=" __stringify(DEFAULT_PROBE_DISPLAY) "])");
>>  module_param_named(vram_bar_size, xe_modparam.force_vram_bar_size, int, 0600);
>>-MODULE_PARM_DESC(vram_bar_size, "Set the vram bar size (in MiB) - <0=disable-resize, 0=max-needed-size[default], >0=force-size");
>>+MODULE_PARM_DESC(vram_bar_size, "Set the vram bar size in MiB (<0=disable-resize, 0=max-needed-size, >0=force-size [default=0])");
>Why leave this one as hard coded value rather than a define?

there wasn't an assignment xe_modparam, so I overlooked it. I will fix
those up in next rev

thanks
Lucas De Marchi


>
>John.
>
>>  module_param_named(guc_log_level, xe_modparam.guc_log_level, int, 0600);
>>  MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=disable, 1=normal, 2..5=verbose-levels "
>>@@ -66,18 +75,21 @@ MODULE_PARM_DESC(gsc_firmware_path,
>>  module_param_named_unsafe(force_probe, xe_modparam.force_probe, charp, 0400);
>>  MODULE_PARM_DESC(force_probe,
>>-		 "Force probe options for specified devices. See CONFIG_DRM_XE_FORCE_PROBE for details.");
>>+		 "Force probe options for specified devices. See CONFIG_DRM_XE_FORCE_PROBE for details "
>>+		 "[default=" DEFAULT_FORCE_PROBE "])");
>>  #ifdef CONFIG_PCI_IOV
>>  module_param_named(max_vfs, xe_modparam.max_vfs, uint, 0400);
>>  MODULE_PARM_DESC(max_vfs,
>>  		 "Limit number of Virtual Functions (VFs) that could be managed. "
>>-		 "(0 = no VFs [default]; N = allow up to N VFs)");
>>+		 "(0=no VFs; N=allow up to N VFs "
>>+		 "[default=" DEFAULT_MAX_VFS_STR "])");
>>  #endif
>>  module_param_named_unsafe(wedged_mode, xe_modparam.wedged_mode, int, 0600);
>>  MODULE_PARM_DESC(wedged_mode,
>>-		 "Module's default policy for the wedged mode - 0=never, 1=upon-critical-errors[default], 2=upon-any-hang");
>>+		 "Module's default policy for the wedged mode (0=never, 1=upon-critical-errors, 2=upon-any-hang "
>>+		 "[default=" __stringify(DEFAULT_WEDGED_MODE) "])");
>>  static int xe_check_nomodeset(void)
>>  {
>>
>