[Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Stuart Summers]

This is attempting to take the best parts of i915 module parameters
(minus the actual module parameters) and add to xe to allow for better
debuggability and configuration in order to help isolate problems
on a per-device level instead of global module parameters.

Note that I did review a few options here: configfs (not generally
used by the drm stack), module parameters (we have some negative
history here), sysfs (not the right approach given the focus on
user interface here). Debugfs is used in various drm drivers to
configure various device characteristics. The infrastructure being
presented here has at a high level been present in the i915 driver
for some years now, so provides a good starting point for quick
debug additions without exposing users to some of the challenges
faced with module parameters in the past.

Stuart Summers (4):
  drm/xe: Refactor early device init
  drm/xe: Refactor debugfs into an early and late part
  drm/xe: Add new device configuration debugfs infrastructure
  drm/xe: Migrate module parameters to new debugfs structure

 drivers/gpu/drm/xe/Makefile            |   1 +
 drivers/gpu/drm/xe/xe_debugfs.c        |  14 +-
 drivers/gpu/drm/xe/xe_debugfs.h        |   1 +
 drivers/gpu/drm/xe/xe_debugfs_params.c | 235 +++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
 drivers/gpu/drm/xe/xe_device.c         |  23 ++-
 drivers/gpu/drm/xe/xe_device.h         |   4 +-
 drivers/gpu/drm/xe/xe_device_types.h   |   4 +
 drivers/gpu/drm/xe/xe_display.c        |   3 +-
 drivers/gpu/drm/xe/xe_guc_log.c        |   2 +-
 drivers/gpu/drm/xe/xe_mmio.c           |   4 +-
 drivers/gpu/drm/xe/xe_module.c         |  16 --
 drivers/gpu/drm/xe/xe_module.h         |   5 -
 drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
 drivers/gpu/drm/xe/xe_params.h         |  43 +++++
 drivers/gpu/drm/xe/xe_pci.c            |  14 +-
 16 files changed, 461 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
 create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
 create mode 100644 drivers/gpu/drm/xe/xe_params.c
 create mode 100644 drivers/gpu/drm/xe/xe_params.h

-- 
2.38.1.143.g1fc3c0ad40

[Intel-xe] [PATCH 1/4] drm/xe: Refactor early device init

[Stuart Summers]

Split the early device initialization sequence into
an early device_info definition and an integration
with the rest of drm (ttm, etc) a little later. This
Allows the device info to be used early from the
hard coded structures in xe_pci.c.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c | 19 +++++++++++++------
 drivers/gpu/drm/xe/xe_device.h |  4 ++--
 drivers/gpu/drm/xe/xe_pci.c    |  6 +++++-
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index a79f934e3d2d..867051d74eea 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -162,8 +162,7 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
 	ttm_device_fini(&xe->ttm);
 }
 
-struct xe_device *xe_device_create(struct pci_dev *pdev,
-				   const struct pci_device_id *ent)
+struct xe_device *xe_device_create(struct pci_dev *pdev)
 {
 	struct xe_device *xe;
 	int err;
@@ -180,14 +179,22 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	if (IS_ERR(xe))
 		return xe;
 
+	xe->info.devid = pdev->device;
+	xe->info.revid = pdev->revision;
+
+	return xe;
+}
+
+int xe_device_init(struct xe_device *xe)
+{
+	int err;
+
 	err = ttm_device_init(&xe->ttm, &xe_ttm_funcs, xe->drm.dev,
 			      xe->drm.anon_inode->i_mapping,
 			      xe->drm.vma_offset_manager, false, false);
 	if (WARN_ON(err))
 		goto err_put;
 
-	xe->info.devid = pdev->device;
-	xe->info.revid = pdev->revision;
 	xe->info.enable_guc = enable_guc;
 	xe->info.enable_display = enable_display;
 
@@ -216,12 +223,12 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	if (err)
 		goto err_put;
 
-	return xe;
+	return 0;
 
 err_put:
 	drm_dev_put(&xe->drm);
 
-	return ERR_PTR(err);
+	return err;
 }
 
 static void xe_device_sanitize(struct drm_device *drm, void *arg)
diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
index d277f8985f7b..c9ad7d20dc22 100644
--- a/drivers/gpu/drm/xe/xe_device.h
+++ b/drivers/gpu/drm/xe/xe_device.h
@@ -31,8 +31,8 @@ static inline struct xe_device *ttm_to_xe_device(struct ttm_device *ttm)
 	return container_of(ttm, struct xe_device, ttm);
 }
 
-struct xe_device *xe_device_create(struct pci_dev *pdev,
-				   const struct pci_device_id *ent);
+struct xe_device *xe_device_create(struct pci_dev *pdev);
+int xe_device_init(struct xe_device *xe);
 int xe_device_probe(struct xe_device *xe);
 void xe_device_remove(struct xe_device *xe);
 void xe_device_shutdown(struct xe_device *xe);
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 91fed9d3105e..c968c9a19de3 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -402,7 +402,7 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return -ENODEV;
 	}
 
-	xe = xe_device_create(pdev, ent);
+	xe = xe_device_create(pdev);
 	if (IS_ERR(xe))
 		return PTR_ERR(xe);
 
@@ -452,6 +452,10 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	xe_display_info_init(xe);
 
+	err = xe_device_init(xe);
+	if (err)
+		return err;
+
 	drm_dbg(&xe->drm, "%s %s %04x:%04x dgfx:%d gfx100:%d media100:%d dma_m_s:%d tc:%d",
 		desc->platform_name, spd ? spd->name : "",
 		xe->info.devid, xe->info.revid,
-- 
2.38.1.143.g1fc3c0ad40

[Intel-xe] [PATCH 2/4] drm/xe: Refactor debugfs into an early and late part

[Stuart Summers]

Split the device info piece of debugfs into an earlier
call to allow the device info to be defined prior to
full device initialization in the xe_device_probe()
routine.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/xe/xe_debugfs.c | 14 ++++++++++----
 drivers/gpu/drm/xe/xe_debugfs.h |  1 +
 drivers/gpu/drm/xe/xe_pci.c     |  2 ++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index 7827a785b020..4b381861344a 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -94,6 +94,16 @@ static const struct file_operations forcewake_all_fops = {
 	.release = forcewake_release,
 };
 
+void xe_debugfs_register_early(struct xe_device *xe)
+{
+	struct drm_minor *minor = xe->drm.primary;
+	struct dentry *root = minor->debugfs_root;
+
+	drm_debugfs_create_files(debugfs_list,
+				 ARRAY_SIZE(debugfs_list),
+				 root, minor);
+}
+
 void xe_debugfs_register(struct xe_device *xe)
 {
 	struct ttm_device *bdev = &xe->ttm;
@@ -104,10 +114,6 @@ void xe_debugfs_register(struct xe_device *xe)
 	u32 mem_type;
 	u8 id;
 
-	drm_debugfs_create_files(debugfs_list,
-				 ARRAY_SIZE(debugfs_list),
-				 root, minor);
-
 	debugfs_create_file("forcewake_all", 0400, root, xe,
 			    &forcewake_all_fops);
 
diff --git a/drivers/gpu/drm/xe/xe_debugfs.h b/drivers/gpu/drm/xe/xe_debugfs.h
index 715b8e2e0bd9..5fe31520ac98 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.h
+++ b/drivers/gpu/drm/xe/xe_debugfs.h
@@ -8,6 +8,7 @@
 
 struct xe_device;
 
+void xe_debugfs_register_early(struct xe_device *xe);
 void xe_debugfs_register(struct xe_device *xe);
 
 #endif
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index c968c9a19de3..f8ca179254fa 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -452,6 +452,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	xe_display_info_init(xe);
 
+	xe_debugfs_register_early(xe);
+
 	err = xe_device_init(xe);
 	if (err)
 		return err;
-- 
2.38.1.143.g1fc3c0ad40

[Intel-xe] [PATCH 3/4] drm/xe: Add new device configuration debugfs infrastructure

[Stuart Summers]

The i915 driver defined an infrastructure for module parameters
which would push these into debugfs for configurability at runtime
with an eventual goal of removing those module parameters completely.

There is a goal in the xe driver to avoid using module parameters
altogether. However, we do need some way to configure the driver
for debug purposes on a per-device level either before initializing
the GPU or even during runtime in some cases.

Bring in the debugfs infrastructure being used in i915, but drop
the references to module parameters.

This patch adds a dummy debugfs entry just to illustrate the new
infrastructure here. It is intended that this will be replaced with
actual driver-usable parameters in a future patch.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/xe/Makefile            |   1 +
 drivers/gpu/drm/xe/xe_debugfs_params.c | 235 +++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
 drivers/gpu/drm/xe/xe_device_types.h   |   4 +
 drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
 drivers/gpu/drm/xe/xe_params.h         |  40 +++++
 drivers/gpu/drm/xe/xe_pci.c            |   6 +
 7 files changed, 418 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
 create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
 create mode 100644 drivers/gpu/drm/xe/xe_params.c
 create mode 100644 drivers/gpu/drm/xe/xe_params.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 5799589d2634..6a8794578f21 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -67,6 +67,7 @@ xe-y += xe_bb.o \
 	xe_mmio.o \
 	xe_mocs.o \
 	xe_module.o \
+	xe_params.o \
 	xe_pci.o \
 	xe_pcode.o \
 	xe_pm.o \
diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.c b/drivers/gpu/drm/xe/xe_debugfs_params.c
new file mode 100644
index 000000000000..9e7272ce153d
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_debugfs_params.c
@@ -0,0 +1,235 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <linux/kernel.h>
+
+#include "xe_debugfs_params.h"
+#include "xe_params.h"
+
+/* int param */
+static int xe_param_int_show(struct seq_file *m, void *data)
+{
+	int *value = m->private;
+
+	seq_printf(m, "%d\n", *value);
+
+	return 0;
+}
+
+static int xe_param_int_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, xe_param_int_show, inode->i_private);
+}
+
+static ssize_t xe_param_int_write(struct file *file,
+				    const char __user *ubuf, size_t len,
+				    loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	int *value = m->private;
+	int ret;
+
+	ret = kstrtoint_from_user(ubuf, len, 0, value);
+	if (ret) {
+		/* support boolean values too */
+		bool b;
+
+		ret = kstrtobool_from_user(ubuf, len, &b);
+		if (!ret)
+			*value = b;
+	}
+
+	return ret ?: len;
+}
+
+static const struct file_operations xe_param_int_fops = {
+	.owner = THIS_MODULE,
+	.open = xe_param_int_open,
+	.read = seq_read,
+	.write = xe_param_int_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations xe_param_int_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = xe_param_int_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+/* unsigned int param */
+static int xe_param_uint_show(struct seq_file *m, void *data)
+{
+	unsigned int *value = m->private;
+
+	seq_printf(m, "%u\n", *value);
+
+	return 0;
+}
+
+static int xe_param_uint_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, xe_param_uint_show, inode->i_private);
+}
+
+static ssize_t xe_param_uint_write(struct file *file,
+				     const char __user *ubuf, size_t len,
+				     loff_t *offp)
+{
+	struct drm_xe_private *xe;
+	struct seq_file *m = file->private_data;
+	unsigned int *value = m->private;
+	unsigned int old = *value;
+	int ret;
+
+	ret = kstrtouint_from_user(ubuf, len, 0, value);
+	if (ret) {
+		/* support boolean values too */
+		bool b;
+
+		ret = kstrtobool_from_user(ubuf, len, &b);
+		if (!ret)
+			*value = b;
+	}
+
+	return ret ?: len;
+}
+
+static const struct file_operations xe_param_uint_fops = {
+	.owner = THIS_MODULE,
+	.open = xe_param_uint_open,
+	.read = seq_read,
+	.write = xe_param_uint_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations xe_param_uint_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = xe_param_uint_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+/* char * param */
+static int xe_param_charp_show(struct seq_file *m, void *data)
+{
+	const char **s = m->private;
+
+	seq_printf(m, "%s\n", *s);
+
+	return 0;
+}
+
+static int xe_param_charp_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, xe_param_charp_show, inode->i_private);
+}
+
+static ssize_t xe_param_charp_write(struct file *file,
+				      const char __user *ubuf, size_t len,
+				      loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	char **s = m->private;
+	char *new, *old;
+
+	old = *s;
+	new = strndup_user(ubuf, PAGE_SIZE);
+	if (IS_ERR(new)) {
+		len = PTR_ERR(new);
+		goto out;
+	}
+
+	*s = new;
+
+	kfree(old);
+out:
+	return len;
+}
+
+static const struct file_operations xe_param_charp_fops = {
+	.owner = THIS_MODULE,
+	.open = xe_param_charp_open,
+	.read = seq_read,
+	.write = xe_param_charp_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations xe_param_charp_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = xe_param_charp_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+#define RO(mode) (((mode) & 0222) == 0)
+
+static struct dentry *
+xe_debugfs_create_int(const char *name, umode_t mode,
+			struct dentry *parent, int *value)
+{
+	return debugfs_create_file_unsafe(name, mode, parent, value,
+					  RO(mode) ? &xe_param_int_fops_ro :
+					  &xe_param_int_fops);
+}
+
+static struct dentry *
+xe_debugfs_create_uint(const char *name, umode_t mode,
+			 struct dentry *parent, unsigned int *value)
+{
+	return debugfs_create_file_unsafe(name, mode, parent, value,
+					  RO(mode) ? &xe_param_uint_fops_ro :
+					  &xe_param_uint_fops);
+}
+
+static struct dentry *
+xe_debugfs_create_charp(const char *name, umode_t mode,
+			  struct dentry *parent, char **value)
+{
+	return debugfs_create_file(name, mode, parent, value,
+				   RO(mode) ? &xe_param_charp_fops_ro :
+				   &xe_param_charp_fops);
+}
+
+#define _xe_param_create_file(parent, name, mode, valp)		\
+	do {								\
+		if (mode)						\
+			_Generic(valp,					\
+				 bool *: debugfs_create_bool,		\
+				 int *: xe_debugfs_create_int,	\
+				 unsigned int *: xe_debugfs_create_uint, \
+				 unsigned long *: debugfs_create_ulong,	\
+				 char **: xe_debugfs_create_charp)(name, mode, parent, valp); \
+	} while(0)
+
+/* add a subdirectory with files for each xe param */
+struct dentry *xe_debugfs_params(struct drm_xe_private *xe)
+{
+	struct drm_minor *minor = xe->drm.primary;
+	struct xe_params *params = &xe->info.params;
+	struct dentry *dir;
+
+	dir = debugfs_create_dir("xe_params", minor->debugfs_root);
+	if (IS_ERR(dir))
+		return dir;
+
+	/*
+	 * Note: We could create files for params needing special handling
+	 * here. Set mode in params to 0 to skip the generic create file, or
+	 * just let the generic create file fail silently with -EEXIST.
+	 */
+
+#define REGISTER(T, x, unused, mode, ...) _xe_param_create_file(dir, #x, mode, &params->x);
+	XE_PARAMS_FOR_EACH(REGISTER);
+#undef REGISTER
+
+	return dir;
+}
diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.h b/drivers/gpu/drm/xe/xe_debugfs_params.h
new file mode 100644
index 000000000000..d4f9b394a443
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_debugfs_params.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef __XE_DEBUGFS_PARAMS__
+#define __XE_DEBUGFS_PARAMS__
+
+struct dentry;
+struct drm_xe_private;
+
+struct dentry *xe_debugfs_params(struct drm_xe_private *xe);
+
+#endif /* __XE_DEBUGFS_PARAMS__ */
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index a02c4eb6bd0d..6847fb2d035c 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -15,6 +15,7 @@
 #include "xe_gt_types.h"
 #include "xe_platform_types.h"
 #include "xe_step_types.h"
+#include "xe_params.h"
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 #include "ext/intel_device_info.h"
@@ -152,6 +153,9 @@ struct xe_device {
 			u32 rawclk_freq;
 		} display;
 #endif
+
+		/* device_info params configurable through debugfs */
+		struct xe_params params;
 	} info;
 
 	/** @irq: device interrupt state */
diff --git a/drivers/gpu/drm/xe/xe_params.c b/drivers/gpu/drm/xe/xe_params.c
new file mode 100644
index 000000000000..7c8f4e07b49c
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_params.c
@@ -0,0 +1,118 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <linux/string_helpers.h>
+
+#include <drm/drm_print.h>
+
+#include "xe_params.h"
+
+struct xe_params xe_modparams __read_mostly = {
+#define MEMBER(T, member, value, ...) .member = (value),
+	XE_PARAMS_FOR_EACH(MEMBER)
+#undef MEMBER
+};
+
+/*
+ * Note: As a rule, keep module parameter sysfs permissions read-only
+ * 0400. Runtime changes are only supported through xe debugfs.
+ *
+ * For any exceptions requiring write access and runtime changes through module
+ * parameter sysfs, prevent debugfs file creation by setting the parameter's
+ * debugfs mode to 0.
+ */
+
+static void _param_print_bool(struct drm_printer *p, const char *name,
+			      bool val)
+{
+	drm_printf(p, "xe.%s=%s\n", name, str_yes_no(val));
+}
+
+static void _param_print_int(struct drm_printer *p, const char *name,
+			     int val)
+{
+	drm_printf(p, "xe.%s=%d\n", name, val);
+}
+
+static void _param_print_uint(struct drm_printer *p, const char *name,
+			      unsigned int val)
+{
+	drm_printf(p, "xe.%s=%u\n", name, val);
+}
+
+static void _param_print_ulong(struct drm_printer *p, const char *name,
+			       unsigned long val)
+{
+	drm_printf(p, "xe.%s=%lu\n", name, val);
+}
+
+static void _param_print_charp(struct drm_printer *p, const char *name,
+			       const char *val)
+{
+	drm_printf(p, "xe.%s=%s\n", name, val);
+}
+
+#define _param_print(p, name, val)				\
+	_Generic(val,						\
+		 bool: _param_print_bool,			\
+		 int: _param_print_int,				\
+		 unsigned int: _param_print_uint,		\
+		 unsigned long: _param_print_ulong,		\
+		 char *: _param_print_charp)(p, name, val)
+
+/**
+ * xe_params_dump - dump xe modparams
+ * @params: xe modparams
+ * @p: the &drm_printer
+ *
+ * Pretty printer for xe modparams.
+ */
+void xe_params_dump(const struct xe_params *params, struct drm_printer *p)
+{
+#define PRINT(T, x, ...) _param_print(p, #x, params->x);
+	XE_PARAMS_FOR_EACH(PRINT);
+#undef PRINT
+}
+
+static void _param_dup_charp(char **valp)
+{
+	*valp = kstrdup(*valp, GFP_ATOMIC);
+}
+
+static void _param_nop(void *valp)
+{
+}
+
+#define _param_dup(valp)				\
+	_Generic(valp,					\
+		 char **: _param_dup_charp,		\
+		 default: _param_nop)(valp)
+
+void xe_params_copy(struct xe_params *dest, const struct xe_params *src)
+{
+	*dest = *src;
+#define DUP(T, x, ...) _param_dup(&dest->x);
+	XE_PARAMS_FOR_EACH(DUP);
+#undef DUP
+}
+
+static void _param_free_charp(char **valp)
+{
+	kfree(*valp);
+	*valp = NULL;
+}
+
+#define _param_free(valp)				\
+	_Generic(valp,					\
+		 char **: _param_free_charp,		\
+		 default: _param_nop)(valp)
+
+/* free the allocated members, *not* the passed in params itself */
+void xe_params_free(struct xe_params *params)
+{
+#define FREE(T, x, ...) _param_free(&params->x);
+	XE_PARAMS_FOR_EACH(FREE);
+#undef FREE
+}
diff --git a/drivers/gpu/drm/xe/xe_params.h b/drivers/gpu/drm/xe/xe_params.h
new file mode 100644
index 000000000000..26fb592e9cd6
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_params.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _XE_PARAMS_H_
+#define _XE_PARAMS_H_
+
+#include <linux/bitops.h>
+#include <linux/cache.h> /* for __read_mostly */
+
+struct drm_printer;
+
+/*
+ * Invoke param, a function-like macro, for each xe param, with arguments:
+ *
+ * param(type, name, value, mode)
+ *
+ * type: parameter type, one of {bool, int, unsigned int, unsigned long, char *}
+ * name: name of the parameter
+ * value: initial/default value of the parameter
+ * mode: debugfs file permissions, one of {0400, 0600, 0}, use 0 to not create
+ *       debugfs file
+ */
+#define XE_PARAMS_FOR_EACH(param) \
+	param(bool, dummy, true, 0444)
+
+#define MEMBER(T, member, ...) T member;
+struct xe_params {
+	XE_PARAMS_FOR_EACH(MEMBER);
+};
+#undef MEMBER
+
+extern struct xe_params xe_modparams __read_mostly;
+
+void xe_params_dump(const struct xe_params *params, struct drm_printer *p);
+void xe_params_copy(struct xe_params *dest, const struct xe_params *src);
+void xe_params_free(struct xe_params *params);
+
+#endif
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index f8ca179254fa..11da5abe587f 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -22,6 +22,8 @@
 #include "xe_module.h"
 #include "xe_pm.h"
 #include "xe_step.h"
+#include "xe_params.h"
+#include "xe_debugfs.h"
 
 #define DEV_INFO_FOR_EACH_FLAG(func) \
 	func(require_force_probe); \
@@ -370,6 +372,8 @@ static void xe_pci_remove(struct pci_dev *pdev)
 	if (!xe) /* driver load aborted, nothing to cleanup */
 		return;
 
+	xe_params_free(&xe->info.params);
+
 	xe_device_remove(xe);
 	xe_pm_runtime_fini(xe);
 	pci_set_drvdata(pdev, NULL);
@@ -406,6 +410,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (IS_ERR(xe))
 		return PTR_ERR(xe);
 
+	xe_params_copy(&xe->info.params, &xe_modparams);
+
 	xe->info.graphics_verx100 = desc->graphics_ver * 100 +
 				    desc->graphics_rel;
 	xe->info.media_verx100 = desc->media_ver * 100 +
-- 
2.38.1.143.g1fc3c0ad40

[Intel-xe] [PATCH 4/4] drm/xe: Migrate module parameters to new debugfs structure

[Stuart Summers]

Use the new debugfs structure for the current module parameters
outside of force_probe. This still allows the driver to be
manually probed without any need to change the autoprobe settings.

For the rest, they can now be tuned prior to binding to the device
by disabling the driver autoprobe.

Drop the current dummy parameter.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c  |  4 ++--
 drivers/gpu/drm/xe/xe_display.c |  3 ++-
 drivers/gpu/drm/xe/xe_guc_log.c |  2 +-
 drivers/gpu/drm/xe/xe_mmio.c    |  4 ++--
 drivers/gpu/drm/xe/xe_module.c  | 16 ----------------
 drivers/gpu/drm/xe/xe_module.h  |  5 -----
 drivers/gpu/drm/xe/xe_params.h  |  5 ++++-
 7 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 867051d74eea..b07f8fc1a515 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -195,8 +195,8 @@ int xe_device_init(struct xe_device *xe)
 	if (WARN_ON(err))
 		goto err_put;
 
-	xe->info.enable_guc = enable_guc;
-	xe->info.enable_display = enable_display;
+	xe->info.enable_guc = xe->info.params.enable_guc;
+	xe->info.enable_display = xe->info.params.enable_display;
 
 	spin_lock_init(&xe->irq.lock);
 
diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
index c67f6a591a58..35f8344fdc5a 100644
--- a/drivers/gpu/drm/xe/xe_display.c
+++ b/drivers/gpu/drm/xe/xe_display.c
@@ -29,6 +29,7 @@
 #include "intel_hotplug.h"
 #include "intel_opregion.h"
 #include "xe_module.h"
+#include "xe_params.h"
 
 /* Xe device functions */
 
@@ -49,7 +50,7 @@ static void xe_display_last_close(struct drm_device *dev)
  */
 int xe_display_set_driver_hooks(struct pci_dev *pdev, struct drm_driver *driver)
 {
-	if (!enable_display)
+	if (!xe_modparams.enable_display)
 		return 0;
 
 	/* Detect if we need to wait for other drivers early on */
diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
index 9a7b5d5906c1..9a2930f6613f 100644
--- a/drivers/gpu/drm/xe/xe_guc_log.c
+++ b/drivers/gpu/drm/xe/xe_guc_log.c
@@ -100,7 +100,7 @@ int xe_guc_log_init(struct xe_guc_log *log)
 
 	xe_map_memset(xe, &bo->vmap, 0, 0, guc_log_size());
 	log->bo = bo;
-	log->level = xe_guc_log_level;
+	log->level = xe->info.params.guc_log_level;
 
 	err = drmm_add_action_or_reset(&xe->drm, guc_log_fini, log);
 	if (err)
diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index 5cacaa05759a..58c459d852ed 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -78,7 +78,7 @@ static int xe_resize_vram_bar(struct xe_device *xe, resource_size_t vram_size)
 	u32 pci_cmd;
 	int i;
 	int ret;
-	u64 force_vram_bar_size = xe_force_vram_bar_size;
+	u64 force_vram_bar_size = xe->info.params.force_vram_bar_size;
 
 	current_size = roundup_pow_of_two(pci_resource_len(pdev, GEN12_LMEM_BAR));
 
@@ -225,7 +225,7 @@ int xe_mmio_probe_vram(struct xe_device *xe)
 		drm_info(&xe->drm, "Successfully resize VRAM from %lluMiB to %lluMiB\n",
 			 (u64)original_size >> 20,
 			 (u64)xe->mem.vram.io_size >> 20);
-	else if (xe->mem.vram.io_size < usable_size && !xe_force_vram_bar_size)
+	else if (xe->mem.vram.io_size < usable_size && !xe->info.params.force_vram_bar_size)
 		drm_info(&xe->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' support in your BIOS.\n",
 			 (u64)xe->mem.vram.size >> 20);
 	if (xe->mem.vram.size < vram_size)
diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index 6860586ce7f8..962c89d371d8 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -14,22 +14,6 @@
 #include "xe_pci.h"
 #include "xe_sched_job.h"
 
-bool enable_guc = true;
-module_param_named_unsafe(enable_guc, enable_guc, bool, 0444);
-MODULE_PARM_DESC(enable_guc, "Enable GuC submission");
-
-bool enable_display = true;
-module_param_named(enable_display, enable_display, bool, 0444);
-MODULE_PARM_DESC(enable_display, "Enable display");
-
-u32 xe_force_vram_bar_size;
-module_param_named(vram_bar_size, xe_force_vram_bar_size, uint, 0600);
-MODULE_PARM_DESC(vram_bar_size, "Set the vram bar size(in MiB)");
-
-int xe_guc_log_level = 5;
-module_param_named(guc_log_level, xe_guc_log_level, int, 0600);
-MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=disable, 1..5=enable with verbosity min..max)");
-
 char *xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE;
 module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0400);
 MODULE_PARM_DESC(force_probe,
diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_module.h
index 86916c176382..fb95bff2c2f4 100644
--- a/drivers/gpu/drm/xe/xe_module.h
+++ b/drivers/gpu/drm/xe/xe_module.h
@@ -5,9 +5,4 @@
 
 #include <linux/init.h>
 
-/* Module modprobe variables */
-extern bool enable_guc;
-extern bool enable_display;
-extern u32 xe_force_vram_bar_size;
-extern int xe_guc_log_level;
 extern char *xe_param_force_probe;
diff --git a/drivers/gpu/drm/xe/xe_params.h b/drivers/gpu/drm/xe/xe_params.h
index 26fb592e9cd6..7d17d75c1a85 100644
--- a/drivers/gpu/drm/xe/xe_params.h
+++ b/drivers/gpu/drm/xe/xe_params.h
@@ -23,7 +23,10 @@ struct drm_printer;
  *       debugfs file
  */
 #define XE_PARAMS_FOR_EACH(param) \
-	param(bool, dummy, true, 0444)
+	param(bool, enable_guc, true, 0444) \
+	param(bool, enable_display, true, 0444) \
+	param(unsigned int, force_vram_bar_size, -1, 0600) \
+	param(int, guc_log_level, 5, 0600)
 
 #define MEMBER(T, member, ...) T member;
 struct xe_params {
-- 
2.38.1.143.g1fc3c0ad40

[Intel-xe] ✗ CI.Patch_applied: failure for RFC: Add new device configuration infrastructure to

[Patchwork]

== Series Details ==

Series: RFC: Add new device configuration infrastructure to
URL   : https://patchwork.freedesktop.org/series/116706/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: 633f26258 fixup! drm/xe: Select graphics/media descriptors from GMD_ID
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_pci.c:452
error: drivers/gpu/drm/xe/xe_pci.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/xe: Refactor early device init
Patch failed at 0001 drm/xe: Refactor early device init
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Jani Nikula]

On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com> wrote:
> This is attempting to take the best parts of i915 module parameters
> (minus the actual module parameters) and add to xe to allow for better
> debuggability and configuration in order to help isolate problems
> on a per-device level instead of global module parameters.
>
> Note that I did review a few options here: configfs (not generally
> used by the drm stack), module parameters (we have some negative
> history here), sysfs (not the right approach given the focus on
> user interface here). Debugfs is used in various drm drivers to
> configure various device characteristics. The infrastructure being
> presented here has at a high level been present in the i915 driver
> for some years now, so provides a good starting point for quick
> debug additions without exposing users to some of the challenges
> faced with module parameters in the past.

Looking into configfs has been on my todo list for a long time. It's
something that often gets recommended as a replacement to module
parameters, but the documentation as well as the existing examples in
the kernel are, I think, less than stellar. Basically would require
implementing it and seeing how it actually works.

In any case, I don't think it should be dismissed with just "not
generally used by the drm stack". A decent implementation could set the
example going forward.

The main problem with debugfs is the inability to set the default values
prior to probing the device. This is where module parameters are handy,
but they aren't device specific (and, as you note, generally
discouraged).

Looking at the patches, I'm not sure I understand what the procedure for
setting the debugfs values before probing the device would be. Can you
provide an example sequence on the command-line please?


BR,
Jani.

>
> Stuart Summers (4):
>   drm/xe: Refactor early device init
>   drm/xe: Refactor debugfs into an early and late part
>   drm/xe: Add new device configuration debugfs infrastructure
>   drm/xe: Migrate module parameters to new debugfs structure
>
>  drivers/gpu/drm/xe/Makefile            |   1 +
>  drivers/gpu/drm/xe/xe_debugfs.c        |  14 +-
>  drivers/gpu/drm/xe/xe_debugfs.h        |   1 +
>  drivers/gpu/drm/xe/xe_debugfs_params.c | 235 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
>  drivers/gpu/drm/xe/xe_device.c         |  23 ++-
>  drivers/gpu/drm/xe/xe_device.h         |   4 +-
>  drivers/gpu/drm/xe/xe_device_types.h   |   4 +
>  drivers/gpu/drm/xe/xe_display.c        |   3 +-
>  drivers/gpu/drm/xe/xe_guc_log.c        |   2 +-
>  drivers/gpu/drm/xe/xe_mmio.c           |   4 +-
>  drivers/gpu/drm/xe/xe_module.c         |  16 --
>  drivers/gpu/drm/xe/xe_module.h         |   5 -
>  drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
>  drivers/gpu/drm/xe/xe_params.h         |  43 +++++
>  drivers/gpu/drm/xe/xe_pci.c            |  14 +-
>  16 files changed, 461 insertions(+), 40 deletions(-)
>  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
>  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
>  create mode 100644 drivers/gpu/drm/xe/xe_params.c
>  create mode 100644 drivers/gpu/drm/xe/xe_params.h

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Summers, Stuart]

On Thu, 2023-04-20 at 12:32 +0300, Jani Nikula wrote:
> On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com> wrote:
> > This is attempting to take the best parts of i915 module parameters
> > (minus the actual module parameters) and add to xe to allow for
> > better
> > debuggability and configuration in order to help isolate problems
> > on a per-device level instead of global module parameters.
> > 
> > Note that I did review a few options here: configfs (not generally
> > used by the drm stack), module parameters (we have some negative
> > history here), sysfs (not the right approach given the focus on
> > user interface here). Debugfs is used in various drm drivers to
> > configure various device characteristics. The infrastructure being
> > presented here has at a high level been present in the i915 driver
> > for some years now, so provides a good starting point for quick
> > debug additions without exposing users to some of the challenges
> > faced with module parameters in the past.
> 
> Looking into configfs has been on my todo list for a long time. It's
> something that often gets recommended as a replacement to module
> parameters, but the documentation as well as the existing examples in
> the kernel are, I think, less than stellar. Basically would require
> implementing it and seeing how it actually works.

Really appreciate that perspective! Yes I understand. Let me take a
closer look here and see what might make the most sense.

> 
> In any case, I don't think it should be dismissed with just "not
> generally used by the drm stack". A decent implementation could set
> the
> example going forward.
> 
> The main problem with debugfs is the inability to set the default
> values
> prior to probing the device. This is where module parameters are
> handy,
> but they aren't device specific (and, as you note, generally
> discouraged).
> 
> Looking at the patches, I'm not sure I understand what the procedure
> for
> setting the debugfs values before probing the device would be. Can
> you
> provide an example sequence on the command-line please?

Well it doesn't seem to be working for me here... so for now let's hold
on this. I'll dig deeper and post an update with examples.

Thanks,
Stuart

> 
> 
> BR,
> Jani.
> 
> > Stuart Summers (4):
> >   drm/xe: Refactor early device init
> >   drm/xe: Refactor debugfs into an early and late part
> >   drm/xe: Add new device configuration debugfs infrastructure
> >   drm/xe: Migrate module parameters to new debugfs structure
> > 
> >  drivers/gpu/drm/xe/Makefile            |   1 +
> >  drivers/gpu/drm/xe/xe_debugfs.c        |  14 +-
> >  drivers/gpu/drm/xe/xe_debugfs.h        |   1 +
> >  drivers/gpu/drm/xe/xe_debugfs_params.c | 235
> > +++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
> >  drivers/gpu/drm/xe/xe_device.c         |  23 ++-
> >  drivers/gpu/drm/xe/xe_device.h         |   4 +-
> >  drivers/gpu/drm/xe/xe_device_types.h   |   4 +
> >  drivers/gpu/drm/xe/xe_display.c        |   3 +-
> >  drivers/gpu/drm/xe/xe_guc_log.c        |   2 +-
> >  drivers/gpu/drm/xe/xe_mmio.c           |   4 +-
> >  drivers/gpu/drm/xe/xe_module.c         |  16 --
> >  drivers/gpu/drm/xe/xe_module.h         |   5 -
> >  drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
> >  drivers/gpu/drm/xe/xe_params.h         |  43 +++++
> >  drivers/gpu/drm/xe/xe_pci.c            |  14 +-
> >  16 files changed, 461 insertions(+), 40 deletions(-)
> >  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
> >  create mode 100644 drivers/gpu/drm/xe/xe_params.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_params.h

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Lucas De Marchi]

On Thu, Apr 20, 2023 at 12:32:55PM +0300, Jani Nikula wrote:
>On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com> wrote:
>> This is attempting to take the best parts of i915 module parameters
>> (minus the actual module parameters) and add to xe to allow for better
>> debuggability and configuration in order to help isolate problems
>> on a per-device level instead of global module parameters.
>>
>> Note that I did review a few options here: configfs (not generally
>> used by the drm stack), module parameters (we have some negative
>> history here), sysfs (not the right approach given the focus on
>> user interface here). Debugfs is used in various drm drivers to
>> configure various device characteristics. The infrastructure being
>> presented here has at a high level been present in the i915 driver
>> for some years now, so provides a good starting point for quick
>> debug additions without exposing users to some of the challenges
>> faced with module parameters in the past.
>
>Looking into configfs has been on my todo list for a long time. It's
>something that often gets recommended as a replacement to module
>parameters, but the documentation as well as the existing examples in
>the kernel are, I think, less than stellar. Basically would require
>implementing it and seeing how it actually works.
>
>In any case, I don't think it should be dismissed with just "not
>generally used by the drm stack". A decent implementation could set the
>example going forward.
>
>The main problem with debugfs is the inability to set the default values
>prior to probing the device. This is where module parameters are handy,
>but they aren't device specific (and, as you note, generally
>discouraged).
>
>Looking at the patches, I'm not sure I understand what the procedure for
>setting the debugfs values before probing the device would be. Can you
>provide an example sequence on the command-line please?

I think that for any solution != parameters is:  split the module load
and the module bind phases.  I've been using that for a long time since
it's a general solution for attaching tracepoints on early probe and
it applies to any kernel module (differently than e.g. adding a "nomodeset"
parameter to achive similar thing). For a debugging thing, usually for early
bring up that would be acceptable I think.

More context:
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/230

Lucas De Marchi

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Summers, Stuart]

On Thu, 2023-04-20 at 11:53 -0700, Lucas De Marchi wrote:
> On Thu, Apr 20, 2023 at 12:32:55PM +0300, Jani Nikula wrote:
> > On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com>
> > wrote:
> > > This is attempting to take the best parts of i915 module
> > > parameters
> > > (minus the actual module parameters) and add to xe to allow for
> > > better
> > > debuggability and configuration in order to help isolate problems
> > > on a per-device level instead of global module parameters.
> > > 
> > > Note that I did review a few options here: configfs (not
> > > generally
> > > used by the drm stack), module parameters (we have some negative
> > > history here), sysfs (not the right approach given the focus on
> > > user interface here). Debugfs is used in various drm drivers to
> > > configure various device characteristics. The infrastructure
> > > being
> > > presented here has at a high level been present in the i915
> > > driver
> > > for some years now, so provides a good starting point for quick
> > > debug additions without exposing users to some of the challenges
> > > faced with module parameters in the past.
> > 
> > Looking into configfs has been on my todo list for a long time.
> > It's
> > something that often gets recommended as a replacement to module
> > parameters, but the documentation as well as the existing examples
> > in
> > the kernel are, I think, less than stellar. Basically would require
> > implementing it and seeing how it actually works.
> > 
> > In any case, I don't think it should be dismissed with just "not
> > generally used by the drm stack". A decent implementation could set
> > the
> > example going forward.
> > 
> > The main problem with debugfs is the inability to set the default
> > values
> > prior to probing the device. This is where module parameters are
> > handy,
> > but they aren't device specific (and, as you note, generally
> > discouraged).
> > 
> > Looking at the patches, I'm not sure I understand what the
> > procedure for
> > setting the debugfs values before probing the device would be. Can
> > you
> > provide an example sequence on the command-line please?
> 
> I think that for any solution != parameters is:  split the module
> load
> and the module bind phases.  I've been using that for a long time
> since
> it's a general solution for attaching tracepoints on early probe and
> it applies to any kernel module (differently than e.g. adding a
> "nomodeset"
> parameter to achive similar thing). For a debugging thing, usually
> for early
> bring up that would be acceptable I think.

Right I know we've done this many times with i915 without issue. This
is the way we're configuring these things per device now. But it's
still a good point about configfs.

Anyway it isn't working with xe today, but that's why I sent this as an
RFC first to get feedback.

I was also thinking the same about debug... really all of these should
be considered debugging features/functions since the driver should to
able to autodetect the default settings for each device configuration
based on various registers and per-platform definitions. Still, let me
dig into configfs a bit more before making that final determination.

Really appreciate the thoughts here though!

-Stuart

> 
> More context:
> https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/230
> 
> Lucas De Marchi

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Lucas De Marchi]

On Thu, Apr 20, 2023 at 08:44:12PM +0000, Stuart Summers wrote:
>On Thu, 2023-04-20 at 11:53 -0700, Lucas De Marchi wrote:
>> On Thu, Apr 20, 2023 at 12:32:55PM +0300, Jani Nikula wrote:
>> > On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com>
>> > wrote:
>> > > This is attempting to take the best parts of i915 module
>> > > parameters
>> > > (minus the actual module parameters) and add to xe to allow for
>> > > better
>> > > debuggability and configuration in order to help isolate problems
>> > > on a per-device level instead of global module parameters.
>> > >
>> > > Note that I did review a few options here: configfs (not
>> > > generally
>> > > used by the drm stack), module parameters (we have some negative
>> > > history here), sysfs (not the right approach given the focus on
>> > > user interface here). Debugfs is used in various drm drivers to
>> > > configure various device characteristics. The infrastructure
>> > > being
>> > > presented here has at a high level been present in the i915
>> > > driver
>> > > for some years now, so provides a good starting point for quick
>> > > debug additions without exposing users to some of the challenges
>> > > faced with module parameters in the past.
>> >
>> > Looking into configfs has been on my todo list for a long time.
>> > It's
>> > something that often gets recommended as a replacement to module
>> > parameters, but the documentation as well as the existing examples
>> > in
>> > the kernel are, I think, less than stellar. Basically would require
>> > implementing it and seeing how it actually works.
>> >
>> > In any case, I don't think it should be dismissed with just "not
>> > generally used by the drm stack". A decent implementation could set
>> > the
>> > example going forward.
>> >
>> > The main problem with debugfs is the inability to set the default
>> > values
>> > prior to probing the device. This is where module parameters are
>> > handy,
>> > but they aren't device specific (and, as you note, generally
>> > discouraged).
>> >
>> > Looking at the patches, I'm not sure I understand what the
>> > procedure for
>> > setting the debugfs values before probing the device would be. Can
>> > you
>> > provide an example sequence on the command-line please?
>>
>> I think that for any solution != parameters is:  split the module
>> load
>> and the module bind phases.  I've been using that for a long time
>> since
>> it's a general solution for attaching tracepoints on early probe and
>> it applies to any kernel module (differently than e.g. adding a
>> "nomodeset"
>> parameter to achive similar thing). For a debugging thing, usually
>> for early
>> bring up that would be acceptable I think.
>
>Right I know we've done this many times with i915 without issue. This
>is the way we're configuring these things per device now. But it's
>still a good point about configfs.
>
>Anyway it isn't working with xe today, but that's why I sent this as an
>RFC first to get feedback.

yes, I know it doesn't work. My point is that any solution we've come up
with that is not per-module like parameters, means that when testing, the
user will have to split the module loading from module binding to the device
under test. See the issue I pointed where we discussed a little bit
about the options. The separation between load/bind is the same I
use for tracepoints that I blogged about some time ago
(https://politreco.com/2022/03/tracepoints-and-kernel-modules/)

For the approach with debugfs it would be no different:  first you have
to a) load the module, then b) set the options then c) bind it to the
device. But if you accept the split between load/bind, doing it via
debugfs is basically reinventing configfs.

Lucas De Marchi

>
>I was also thinking the same about debug... really all of these should
>be considered debugging features/functions since the driver should to
>able to autodetect the default settings for each device configuration
>based on various registers and per-platform definitions. Still, let me
>dig into configfs a bit more before making that final determination.
>
>Really appreciate the thoughts here though!
>
>-Stuart
>
>>
>> More context:
>> https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/230
>>
>> Lucas De Marchi

Re: [Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Summers, Stuart]

On Thu, 2023-04-20 at 13:55 -0700, Lucas De Marchi wrote:
> On Thu, Apr 20, 2023 at 08:44:12PM +0000, Stuart Summers wrote:
> > On Thu, 2023-04-20 at 11:53 -0700, Lucas De Marchi wrote:
> > > On Thu, Apr 20, 2023 at 12:32:55PM +0300, Jani Nikula wrote:
> > > > On Wed, 19 Apr 2023, Stuart Summers <stuart.summers@intel.com>
> > > > wrote:
> > > > > This is attempting to take the best parts of i915 module
> > > > > parameters
> > > > > (minus the actual module parameters) and add to xe to allow
> > > > > for
> > > > > better
> > > > > debuggability and configuration in order to help isolate
> > > > > problems
> > > > > on a per-device level instead of global module parameters.
> > > > > 
> > > > > Note that I did review a few options here: configfs (not
> > > > > generally
> > > > > used by the drm stack), module parameters (we have some
> > > > > negative
> > > > > history here), sysfs (not the right approach given the focus
> > > > > on
> > > > > user interface here). Debugfs is used in various drm drivers
> > > > > to
> > > > > configure various device characteristics. The infrastructure
> > > > > being
> > > > > presented here has at a high level been present in the i915
> > > > > driver
> > > > > for some years now, so provides a good starting point for
> > > > > quick
> > > > > debug additions without exposing users to some of the
> > > > > challenges
> > > > > faced with module parameters in the past.
> > > > 
> > > > Looking into configfs has been on my todo list for a long time.
> > > > It's
> > > > something that often gets recommended as a replacement to
> > > > module
> > > > parameters, but the documentation as well as the existing
> > > > examples
> > > > in
> > > > the kernel are, I think, less than stellar. Basically would
> > > > require
> > > > implementing it and seeing how it actually works.
> > > > 
> > > > In any case, I don't think it should be dismissed with just
> > > > "not
> > > > generally used by the drm stack". A decent implementation could
> > > > set
> > > > the
> > > > example going forward.
> > > > 
> > > > The main problem with debugfs is the inability to set the
> > > > default
> > > > values
> > > > prior to probing the device. This is where module parameters
> > > > are
> > > > handy,
> > > > but they aren't device specific (and, as you note, generally
> > > > discouraged).
> > > > 
> > > > Looking at the patches, I'm not sure I understand what the
> > > > procedure for
> > > > setting the debugfs values before probing the device would be.
> > > > Can
> > > > you
> > > > provide an example sequence on the command-line please?
> > > 
> > > I think that for any solution != parameters is:  split the module
> > > load
> > > and the module bind phases.  I've been using that for a long time
> > > since
> > > it's a general solution for attaching tracepoints on early probe
> > > and
> > > it applies to any kernel module (differently than e.g. adding a
> > > "nomodeset"
> > > parameter to achive similar thing). For a debugging thing,
> > > usually
> > > for early
> > > bring up that would be acceptable I think.
> > 
> > Right I know we've done this many times with i915 without issue.
> > This
> > is the way we're configuring these things per device now. But it's
> > still a good point about configfs.
> > 
> > Anyway it isn't working with xe today, but that's why I sent this
> > as an
> > RFC first to get feedback.
> 
> yes, I know it doesn't work. My point is that any solution we've come
> up
> with that is not per-module like parameters, means that when testing,
> the
> user will have to split the module loading from module binding to the
> device
> under test. See the issue I pointed where we discussed a little bit
> about the options. The separation between load/bind is the same I
> use for tracepoints that I blogged about some time ago
> (https://politreco.com/2022/03/tracepoints-and-kernel-modules/)
> 
> For the approach with debugfs it would be no different:  first you
> have
> to a) load the module, then b) set the options then c) bind it to the
> device. But if you accept the split between load/bind, doing it via
> debugfs is basically reinventing configfs.

Right, but then again, these aren't meant for the typical user. Again,
these are intended to be debug specific settings. As such, the extra
steps needed to set up the file system in whatever way is either on the
kernel developer or possibly the CI developer. In both cases, this will
be very targeted and not something we would want to document as a
normal end-user experience.

As for reinventing configfs, point taken there. Let me look a bit
deeper there and get back.

Thanks,
Stuart

> 
> Lucas De Marchi
> 
> > I was also thinking the same about debug... really all of these
> > should
> > be considered debugging features/functions since the driver should
> > to
> > able to autodetect the default settings for each device
> > configuration
> > based on various registers and per-platform definitions. Still, let
> > me
> > dig into configfs a bit more before making that final
> > determination.
> > 
> > Really appreciate the thoughts here though!
> > 
> > -Stuart
> > 
> > > More context:
> > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/230
> > > 
> > > Lucas De Marchi

Re: [Intel-xe] [PATCH 3/4] drm/xe: Add new device configuration debugfs infrastructure

[Rodrigo Vivi]

On Wed, Apr 19, 2023 at 05:55:00PM +0000, Stuart Summers wrote:
> The i915 driver defined an infrastructure for module parameters
> which would push these into debugfs for configurability at runtime
> with an eventual goal of removing those module parameters completely.

I wonder if we have here something that could become a more generic
infra in drm or even the core kernel...

> 
> There is a goal in the xe driver to avoid using module parameters
> altogether. However, we do need some way to configure the driver
> for debug purposes on a per-device level either before initializing
> the GPU or even during runtime in some cases.
> 
> Bring in the debugfs infrastructure being used in i915, but drop
> the references to module parameters.
> 
> This patch adds a dummy debugfs entry just to illustrate the new
> infrastructure here. It is intended that this will be replaced with
> actual driver-usable parameters in a future patch.
> 
> Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> ---
>  drivers/gpu/drm/xe/Makefile            |   1 +
>  drivers/gpu/drm/xe/xe_debugfs_params.c | 235 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
>  drivers/gpu/drm/xe/xe_device_types.h   |   4 +
>  drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
>  drivers/gpu/drm/xe/xe_params.h         |  40 +++++
>  drivers/gpu/drm/xe/xe_pci.c            |   6 +
>  7 files changed, 418 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
>  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
>  create mode 100644 drivers/gpu/drm/xe/xe_params.c
>  create mode 100644 drivers/gpu/drm/xe/xe_params.h
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 5799589d2634..6a8794578f21 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -67,6 +67,7 @@ xe-y += xe_bb.o \
>  	xe_mmio.o \
>  	xe_mocs.o \
>  	xe_module.o \
> +	xe_params.o \
>  	xe_pci.o \
>  	xe_pcode.o \
>  	xe_pm.o \
> diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.c b/drivers/gpu/drm/xe/xe_debugfs_params.c
> new file mode 100644
> index 000000000000..9e7272ce153d
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_debugfs_params.c
> @@ -0,0 +1,235 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +
> +#include "xe_debugfs_params.h"
> +#include "xe_params.h"
> +
> +/* int param */
> +static int xe_param_int_show(struct seq_file *m, void *data)
> +{
> +	int *value = m->private;
> +
> +	seq_printf(m, "%d\n", *value);
> +
> +	return 0;
> +}
> +
> +static int xe_param_int_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, xe_param_int_show, inode->i_private);
> +}
> +
> +static ssize_t xe_param_int_write(struct file *file,
> +				    const char __user *ubuf, size_t len,
> +				    loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	int *value = m->private;
> +	int ret;
> +
> +	ret = kstrtoint_from_user(ubuf, len, 0, value);
> +	if (ret) {
> +		/* support boolean values too */
> +		bool b;
> +
> +		ret = kstrtobool_from_user(ubuf, len, &b);
> +		if (!ret)
> +			*value = b;
> +	}
> +
> +	return ret ?: len;
> +}
> +
> +static const struct file_operations xe_param_int_fops = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_int_open,
> +	.read = seq_read,
> +	.write = xe_param_int_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations xe_param_int_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_int_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +/* unsigned int param */
> +static int xe_param_uint_show(struct seq_file *m, void *data)
> +{
> +	unsigned int *value = m->private;
> +
> +	seq_printf(m, "%u\n", *value);
> +
> +	return 0;
> +}
> +
> +static int xe_param_uint_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, xe_param_uint_show, inode->i_private);
> +}
> +
> +static ssize_t xe_param_uint_write(struct file *file,
> +				     const char __user *ubuf, size_t len,
> +				     loff_t *offp)
> +{
> +	struct drm_xe_private *xe;
> +	struct seq_file *m = file->private_data;
> +	unsigned int *value = m->private;
> +	unsigned int old = *value;
> +	int ret;
> +
> +	ret = kstrtouint_from_user(ubuf, len, 0, value);
> +	if (ret) {
> +		/* support boolean values too */
> +		bool b;
> +
> +		ret = kstrtobool_from_user(ubuf, len, &b);
> +		if (!ret)
> +			*value = b;
> +	}
> +
> +	return ret ?: len;
> +}
> +
> +static const struct file_operations xe_param_uint_fops = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_uint_open,
> +	.read = seq_read,
> +	.write = xe_param_uint_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations xe_param_uint_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_uint_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +/* char * param */
> +static int xe_param_charp_show(struct seq_file *m, void *data)
> +{
> +	const char **s = m->private;
> +
> +	seq_printf(m, "%s\n", *s);
> +
> +	return 0;
> +}
> +
> +static int xe_param_charp_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, xe_param_charp_show, inode->i_private);
> +}
> +
> +static ssize_t xe_param_charp_write(struct file *file,
> +				      const char __user *ubuf, size_t len,
> +				      loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	char **s = m->private;
> +	char *new, *old;
> +
> +	old = *s;
> +	new = strndup_user(ubuf, PAGE_SIZE);
> +	if (IS_ERR(new)) {
> +		len = PTR_ERR(new);
> +		goto out;
> +	}
> +
> +	*s = new;
> +
> +	kfree(old);
> +out:
> +	return len;
> +}
> +
> +static const struct file_operations xe_param_charp_fops = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_charp_open,
> +	.read = seq_read,
> +	.write = xe_param_charp_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations xe_param_charp_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = xe_param_charp_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +#define RO(mode) (((mode) & 0222) == 0)
> +
> +static struct dentry *
> +xe_debugfs_create_int(const char *name, umode_t mode,
> +			struct dentry *parent, int *value)
> +{
> +	return debugfs_create_file_unsafe(name, mode, parent, value,
> +					  RO(mode) ? &xe_param_int_fops_ro :
> +					  &xe_param_int_fops);
> +}
> +
> +static struct dentry *
> +xe_debugfs_create_uint(const char *name, umode_t mode,
> +			 struct dentry *parent, unsigned int *value)
> +{
> +	return debugfs_create_file_unsafe(name, mode, parent, value,
> +					  RO(mode) ? &xe_param_uint_fops_ro :
> +					  &xe_param_uint_fops);
> +}
> +
> +static struct dentry *
> +xe_debugfs_create_charp(const char *name, umode_t mode,
> +			  struct dentry *parent, char **value)
> +{
> +	return debugfs_create_file(name, mode, parent, value,
> +				   RO(mode) ? &xe_param_charp_fops_ro :
> +				   &xe_param_charp_fops);
> +}
> +
> +#define _xe_param_create_file(parent, name, mode, valp)		\
> +	do {								\
> +		if (mode)						\
> +			_Generic(valp,					\
> +				 bool *: debugfs_create_bool,		\
> +				 int *: xe_debugfs_create_int,	\
> +				 unsigned int *: xe_debugfs_create_uint, \
> +				 unsigned long *: debugfs_create_ulong,	\
> +				 char **: xe_debugfs_create_charp)(name, mode, parent, valp); \
> +	} while(0)
> +
> +/* add a subdirectory with files for each xe param */
> +struct dentry *xe_debugfs_params(struct drm_xe_private *xe)
> +{
> +	struct drm_minor *minor = xe->drm.primary;
> +	struct xe_params *params = &xe->info.params;
> +	struct dentry *dir;
> +
> +	dir = debugfs_create_dir("xe_params", minor->debugfs_root);
> +	if (IS_ERR(dir))
> +		return dir;
> +
> +	/*
> +	 * Note: We could create files for params needing special handling
> +	 * here. Set mode in params to 0 to skip the generic create file, or
> +	 * just let the generic create file fail silently with -EEXIST.
> +	 */
> +
> +#define REGISTER(T, x, unused, mode, ...) _xe_param_create_file(dir, #x, mode, &params->x);
> +	XE_PARAMS_FOR_EACH(REGISTER);
> +#undef REGISTER
> +
> +	return dir;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.h b/drivers/gpu/drm/xe/xe_debugfs_params.h
> new file mode 100644
> index 000000000000..d4f9b394a443
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_debugfs_params.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __XE_DEBUGFS_PARAMS__
> +#define __XE_DEBUGFS_PARAMS__
> +
> +struct dentry;
> +struct drm_xe_private;
> +
> +struct dentry *xe_debugfs_params(struct drm_xe_private *xe);
> +
> +#endif /* __XE_DEBUGFS_PARAMS__ */
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index a02c4eb6bd0d..6847fb2d035c 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -15,6 +15,7 @@
>  #include "xe_gt_types.h"
>  #include "xe_platform_types.h"
>  #include "xe_step_types.h"
> +#include "xe_params.h"
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>  #include "ext/intel_device_info.h"
> @@ -152,6 +153,9 @@ struct xe_device {
>  			u32 rawclk_freq;
>  		} display;
>  #endif
> +
> +		/* device_info params configurable through debugfs */
> +		struct xe_params params;
>  	} info;
>  
>  	/** @irq: device interrupt state */
> diff --git a/drivers/gpu/drm/xe/xe_params.c b/drivers/gpu/drm/xe/xe_params.c
> new file mode 100644
> index 000000000000..7c8f4e07b49c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_params.c
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/string_helpers.h>
> +
> +#include <drm/drm_print.h>
> +
> +#include "xe_params.h"
> +
> +struct xe_params xe_modparams __read_mostly = {
> +#define MEMBER(T, member, value, ...) .member = (value),
> +	XE_PARAMS_FOR_EACH(MEMBER)
> +#undef MEMBER
> +};
> +
> +/*
> + * Note: As a rule, keep module parameter sysfs permissions read-only
> + * 0400. Runtime changes are only supported through xe debugfs.
> + *
> + * For any exceptions requiring write access and runtime changes through module
> + * parameter sysfs, prevent debugfs file creation by setting the parameter's
> + * debugfs mode to 0.
> + */
> +
> +static void _param_print_bool(struct drm_printer *p, const char *name,
> +			      bool val)
> +{
> +	drm_printf(p, "xe.%s=%s\n", name, str_yes_no(val));
> +}
> +
> +static void _param_print_int(struct drm_printer *p, const char *name,
> +			     int val)
> +{
> +	drm_printf(p, "xe.%s=%d\n", name, val);
> +}
> +
> +static void _param_print_uint(struct drm_printer *p, const char *name,
> +			      unsigned int val)
> +{
> +	drm_printf(p, "xe.%s=%u\n", name, val);
> +}
> +
> +static void _param_print_ulong(struct drm_printer *p, const char *name,
> +			       unsigned long val)
> +{
> +	drm_printf(p, "xe.%s=%lu\n", name, val);
> +}
> +
> +static void _param_print_charp(struct drm_printer *p, const char *name,
> +			       const char *val)
> +{
> +	drm_printf(p, "xe.%s=%s\n", name, val);
> +}
> +
> +#define _param_print(p, name, val)				\
> +	_Generic(val,						\
> +		 bool: _param_print_bool,			\
> +		 int: _param_print_int,				\
> +		 unsigned int: _param_print_uint,		\
> +		 unsigned long: _param_print_ulong,		\
> +		 char *: _param_print_charp)(p, name, val)
> +
> +/**
> + * xe_params_dump - dump xe modparams
> + * @params: xe modparams
> + * @p: the &drm_printer
> + *
> + * Pretty printer for xe modparams.
> + */
> +void xe_params_dump(const struct xe_params *params, struct drm_printer *p)
> +{
> +#define PRINT(T, x, ...) _param_print(p, #x, params->x);
> +	XE_PARAMS_FOR_EACH(PRINT);
> +#undef PRINT
> +}
> +
> +static void _param_dup_charp(char **valp)
> +{
> +	*valp = kstrdup(*valp, GFP_ATOMIC);
> +}
> +
> +static void _param_nop(void *valp)
> +{
> +}
> +
> +#define _param_dup(valp)				\
> +	_Generic(valp,					\
> +		 char **: _param_dup_charp,		\
> +		 default: _param_nop)(valp)
> +
> +void xe_params_copy(struct xe_params *dest, const struct xe_params *src)
> +{
> +	*dest = *src;
> +#define DUP(T, x, ...) _param_dup(&dest->x);
> +	XE_PARAMS_FOR_EACH(DUP);
> +#undef DUP
> +}
> +
> +static void _param_free_charp(char **valp)
> +{
> +	kfree(*valp);
> +	*valp = NULL;
> +}
> +
> +#define _param_free(valp)				\
> +	_Generic(valp,					\
> +		 char **: _param_free_charp,		\
> +		 default: _param_nop)(valp)
> +
> +/* free the allocated members, *not* the passed in params itself */
> +void xe_params_free(struct xe_params *params)
> +{
> +#define FREE(T, x, ...) _param_free(&params->x);
> +	XE_PARAMS_FOR_EACH(FREE);
> +#undef FREE
> +}
> diff --git a/drivers/gpu/drm/xe/xe_params.h b/drivers/gpu/drm/xe/xe_params.h
> new file mode 100644
> index 000000000000..26fb592e9cd6
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_params.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_PARAMS_H_
> +#define _XE_PARAMS_H_
> +
> +#include <linux/bitops.h>
> +#include <linux/cache.h> /* for __read_mostly */
> +
> +struct drm_printer;
> +
> +/*
> + * Invoke param, a function-like macro, for each xe param, with arguments:
> + *
> + * param(type, name, value, mode)
> + *
> + * type: parameter type, one of {bool, int, unsigned int, unsigned long, char *}
> + * name: name of the parameter
> + * value: initial/default value of the parameter
> + * mode: debugfs file permissions, one of {0400, 0600, 0}, use 0 to not create
> + *       debugfs file
> + */
> +#define XE_PARAMS_FOR_EACH(param) \
> +	param(bool, dummy, true, 0444)
> +
> +#define MEMBER(T, member, ...) T member;
> +struct xe_params {
> +	XE_PARAMS_FOR_EACH(MEMBER);
> +};
> +#undef MEMBER
> +
> +extern struct xe_params xe_modparams __read_mostly;
> +
> +void xe_params_dump(const struct xe_params *params, struct drm_printer *p);
> +void xe_params_copy(struct xe_params *dest, const struct xe_params *src);
> +void xe_params_free(struct xe_params *params);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index f8ca179254fa..11da5abe587f 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -22,6 +22,8 @@
>  #include "xe_module.h"
>  #include "xe_pm.h"
>  #include "xe_step.h"
> +#include "xe_params.h"
> +#include "xe_debugfs.h"
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func) \
>  	func(require_force_probe); \
> @@ -370,6 +372,8 @@ static void xe_pci_remove(struct pci_dev *pdev)
>  	if (!xe) /* driver load aborted, nothing to cleanup */
>  		return;
>  
> +	xe_params_free(&xe->info.params);
> +
>  	xe_device_remove(xe);
>  	xe_pm_runtime_fini(xe);
>  	pci_set_drvdata(pdev, NULL);
> @@ -406,6 +410,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (IS_ERR(xe))
>  		return PTR_ERR(xe);
>  
> +	xe_params_copy(&xe->info.params, &xe_modparams);
> +
>  	xe->info.graphics_verx100 = desc->graphics_ver * 100 +
>  				    desc->graphics_rel;
>  	xe->info.media_verx100 = desc->media_ver * 100 +
> -- 
> 2.38.1.143.g1fc3c0ad40
> 

Re: [Intel-xe] [PATCH 3/4] drm/xe: Add new device configuration debugfs infrastructure

[Summers, Stuart]

On Fri, 2023-05-05 at 10:55 -0400, Rodrigo Vivi wrote:
> On Wed, Apr 19, 2023 at 05:55:00PM +0000, Stuart Summers wrote:
> > The i915 driver defined an infrastructure for module parameters
> > which would push these into debugfs for configurability at runtime
> > with an eventual goal of removing those module parameters
> > completely.
> 
> I wonder if we have here something that could become a more generic
> infra in drm or even the core kernel...

For sure I had the same thought. I haven't been able to really dig into
the configfs pieces yet, but I was planning on looking at that.

Thanks,
Stuart

> 
> > 
> > There is a goal in the xe driver to avoid using module parameters
> > altogether. However, we do need some way to configure the driver
> > for debug purposes on a per-device level either before initializing
> > the GPU or even during runtime in some cases.
> > 
> > Bring in the debugfs infrastructure being used in i915, but drop
> > the references to module parameters.
> > 
> > This patch adds a dummy debugfs entry just to illustrate the new
> > infrastructure here. It is intended that this will be replaced with
> > actual driver-usable parameters in a future patch.
> > 
> > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > ---
> >  drivers/gpu/drm/xe/Makefile            |   1 +
> >  drivers/gpu/drm/xe/xe_debugfs_params.c | 235
> > +++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_debugfs_params.h |  14 ++
> >  drivers/gpu/drm/xe/xe_device_types.h   |   4 +
> >  drivers/gpu/drm/xe/xe_params.c         | 118 +++++++++++++
> >  drivers/gpu/drm/xe/xe_params.h         |  40 +++++
> >  drivers/gpu/drm/xe/xe_pci.c            |   6 +
> >  7 files changed, 418 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_debugfs_params.h
> >  create mode 100644 drivers/gpu/drm/xe/xe_params.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_params.h
> > 
> > diff --git a/drivers/gpu/drm/xe/Makefile
> > b/drivers/gpu/drm/xe/Makefile
> > index 5799589d2634..6a8794578f21 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -67,6 +67,7 @@ xe-y += xe_bb.o \
> >         xe_mmio.o \
> >         xe_mocs.o \
> >         xe_module.o \
> > +       xe_params.o \
> >         xe_pci.o \
> >         xe_pcode.o \
> >         xe_pm.o \
> > diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.c
> > b/drivers/gpu/drm/xe/xe_debugfs_params.c
> > new file mode 100644
> > index 000000000000..9e7272ce153d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_debugfs_params.c
> > @@ -0,0 +1,235 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#include <linux/kernel.h>
> > +
> > +#include "xe_debugfs_params.h"
> > +#include "xe_params.h"
> > +
> > +/* int param */
> > +static int xe_param_int_show(struct seq_file *m, void *data)
> > +{
> > +       int *value = m->private;
> > +
> > +       seq_printf(m, "%d\n", *value);
> > +
> > +       return 0;
> > +}
> > +
> > +static int xe_param_int_open(struct inode *inode, struct file
> > *file)
> > +{
> > +       return single_open(file, xe_param_int_show, inode-
> > >i_private);
> > +}
> > +
> > +static ssize_t xe_param_int_write(struct file *file,
> > +                                   const char __user *ubuf, size_t
> > len,
> > +                                   loff_t *offp)
> > +{
> > +       struct seq_file *m = file->private_data;
> > +       int *value = m->private;
> > +       int ret;
> > +
> > +       ret = kstrtoint_from_user(ubuf, len, 0, value);
> > +       if (ret) {
> > +               /* support boolean values too */
> > +               bool b;
> > +
> > +               ret = kstrtobool_from_user(ubuf, len, &b);
> > +               if (!ret)
> > +                       *value = b;
> > +       }
> > +
> > +       return ret ?: len;
> > +}
> > +
> > +static const struct file_operations xe_param_int_fops = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_int_open,
> > +       .read = seq_read,
> > +       .write = xe_param_int_write,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +static const struct file_operations xe_param_int_fops_ro = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_int_open,
> > +       .read = seq_read,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +/* unsigned int param */
> > +static int xe_param_uint_show(struct seq_file *m, void *data)
> > +{
> > +       unsigned int *value = m->private;
> > +
> > +       seq_printf(m, "%u\n", *value);
> > +
> > +       return 0;
> > +}
> > +
> > +static int xe_param_uint_open(struct inode *inode, struct file
> > *file)
> > +{
> > +       return single_open(file, xe_param_uint_show, inode-
> > >i_private);
> > +}
> > +
> > +static ssize_t xe_param_uint_write(struct file *file,
> > +                                    const char __user *ubuf,
> > size_t len,
> > +                                    loff_t *offp)
> > +{
> > +       struct drm_xe_private *xe;
> > +       struct seq_file *m = file->private_data;
> > +       unsigned int *value = m->private;
> > +       unsigned int old = *value;
> > +       int ret;
> > +
> > +       ret = kstrtouint_from_user(ubuf, len, 0, value);
> > +       if (ret) {
> > +               /* support boolean values too */
> > +               bool b;
> > +
> > +               ret = kstrtobool_from_user(ubuf, len, &b);
> > +               if (!ret)
> > +                       *value = b;
> > +       }
> > +
> > +       return ret ?: len;
> > +}
> > +
> > +static const struct file_operations xe_param_uint_fops = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_uint_open,
> > +       .read = seq_read,
> > +       .write = xe_param_uint_write,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +static const struct file_operations xe_param_uint_fops_ro = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_uint_open,
> > +       .read = seq_read,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +/* char * param */
> > +static int xe_param_charp_show(struct seq_file *m, void *data)
> > +{
> > +       const char **s = m->private;
> > +
> > +       seq_printf(m, "%s\n", *s);
> > +
> > +       return 0;
> > +}
> > +
> > +static int xe_param_charp_open(struct inode *inode, struct file
> > *file)
> > +{
> > +       return single_open(file, xe_param_charp_show, inode-
> > >i_private);
> > +}
> > +
> > +static ssize_t xe_param_charp_write(struct file *file,
> > +                                     const char __user *ubuf,
> > size_t len,
> > +                                     loff_t *offp)
> > +{
> > +       struct seq_file *m = file->private_data;
> > +       char **s = m->private;
> > +       char *new, *old;
> > +
> > +       old = *s;
> > +       new = strndup_user(ubuf, PAGE_SIZE);
> > +       if (IS_ERR(new)) {
> > +               len = PTR_ERR(new);
> > +               goto out;
> > +       }
> > +
> > +       *s = new;
> > +
> > +       kfree(old);
> > +out:
> > +       return len;
> > +}
> > +
> > +static const struct file_operations xe_param_charp_fops = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_charp_open,
> > +       .read = seq_read,
> > +       .write = xe_param_charp_write,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +static const struct file_operations xe_param_charp_fops_ro = {
> > +       .owner = THIS_MODULE,
> > +       .open = xe_param_charp_open,
> > +       .read = seq_read,
> > +       .llseek = default_llseek,
> > +       .release = single_release,
> > +};
> > +
> > +#define RO(mode) (((mode) & 0222) == 0)
> > +
> > +static struct dentry *
> > +xe_debugfs_create_int(const char *name, umode_t mode,
> > +                       struct dentry *parent, int *value)
> > +{
> > +       return debugfs_create_file_unsafe(name, mode, parent,
> > value,
> > +                                         RO(mode) ?
> > &xe_param_int_fops_ro :
> > +                                         &xe_param_int_fops);
> > +}
> > +
> > +static struct dentry *
> > +xe_debugfs_create_uint(const char *name, umode_t mode,
> > +                        struct dentry *parent, unsigned int
> > *value)
> > +{
> > +       return debugfs_create_file_unsafe(name, mode, parent,
> > value,
> > +                                         RO(mode) ?
> > &xe_param_uint_fops_ro :
> > +                                         &xe_param_uint_fops);
> > +}
> > +
> > +static struct dentry *
> > +xe_debugfs_create_charp(const char *name, umode_t mode,
> > +                         struct dentry *parent, char **value)
> > +{
> > +       return debugfs_create_file(name, mode, parent, value,
> > +                                  RO(mode) ?
> > &xe_param_charp_fops_ro :
> > +                                  &xe_param_charp_fops);
> > +}
> > +
> > +#define _xe_param_create_file(parent, name, mode,
> > valp)                \
> > +       do
> > {                                                            \
> > +               if
> > (mode)                                               \
> > +                       _Generic(valp,                             
> >      \
> > +                                bool *:
> > debugfs_create_bool,           \
> > +                                int *: xe_debugfs_create_int,  \
> > +                                unsigned int *:
> > xe_debugfs_create_uint, \
> > +                                unsigned long *:
> > debugfs_create_ulong, \
> > +                                char **:
> > xe_debugfs_create_charp)(name, mode, parent, valp); \
> > +       } while(0)
> > +
> > +/* add a subdirectory with files for each xe param */
> > +struct dentry *xe_debugfs_params(struct drm_xe_private *xe)
> > +{
> > +       struct drm_minor *minor = xe->drm.primary;
> > +       struct xe_params *params = &xe->info.params;
> > +       struct dentry *dir;
> > +
> > +       dir = debugfs_create_dir("xe_params", minor->debugfs_root);
> > +       if (IS_ERR(dir))
> > +               return dir;
> > +
> > +       /*
> > +        * Note: We could create files for params needing special
> > handling
> > +        * here. Set mode in params to 0 to skip the generic create
> > file, or
> > +        * just let the generic create file fail silently with -
> > EEXIST.
> > +        */
> > +
> > +#define REGISTER(T, x, unused, mode, ...)
> > _xe_param_create_file(dir, #x, mode, &params->x);
> > +       XE_PARAMS_FOR_EACH(REGISTER);
> > +#undef REGISTER
> > +
> > +       return dir;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_debugfs_params.h
> > b/drivers/gpu/drm/xe/xe_debugfs_params.h
> > new file mode 100644
> > index 000000000000..d4f9b394a443
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_debugfs_params.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#ifndef __XE_DEBUGFS_PARAMS__
> > +#define __XE_DEBUGFS_PARAMS__
> > +
> > +struct dentry;
> > +struct drm_xe_private;
> > +
> > +struct dentry *xe_debugfs_params(struct drm_xe_private *xe);
> > +
> > +#endif /* __XE_DEBUGFS_PARAMS__ */
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > b/drivers/gpu/drm/xe/xe_device_types.h
> > index a02c4eb6bd0d..6847fb2d035c 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -15,6 +15,7 @@
> >  #include "xe_gt_types.h"
> >  #include "xe_platform_types.h"
> >  #include "xe_step_types.h"
> > +#include "xe_params.h"
> >  
> >  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >  #include "ext/intel_device_info.h"
> > @@ -152,6 +153,9 @@ struct xe_device {
> >                         u32 rawclk_freq;
> >                 } display;
> >  #endif
> > +
> > +               /* device_info params configurable through debugfs
> > */
> > +               struct xe_params params;
> >         } info;
> >  
> >         /** @irq: device interrupt state */
> > diff --git a/drivers/gpu/drm/xe/xe_params.c
> > b/drivers/gpu/drm/xe/xe_params.c
> > new file mode 100644
> > index 000000000000..7c8f4e07b49c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_params.c
> > @@ -0,0 +1,118 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#include <linux/string_helpers.h>
> > +
> > +#include <drm/drm_print.h>
> > +
> > +#include "xe_params.h"
> > +
> > +struct xe_params xe_modparams __read_mostly = {
> > +#define MEMBER(T, member, value, ...) .member = (value),
> > +       XE_PARAMS_FOR_EACH(MEMBER)
> > +#undef MEMBER
> > +};
> > +
> > +/*
> > + * Note: As a rule, keep module parameter sysfs permissions read-
> > only
> > + * 0400. Runtime changes are only supported through xe debugfs.
> > + *
> > + * For any exceptions requiring write access and runtime changes
> > through module
> > + * parameter sysfs, prevent debugfs file creation by setting the
> > parameter's
> > + * debugfs mode to 0.
> > + */
> > +
> > +static void _param_print_bool(struct drm_printer *p, const char
> > *name,
> > +                             bool val)
> > +{
> > +       drm_printf(p, "xe.%s=%s\n", name, str_yes_no(val));
> > +}
> > +
> > +static void _param_print_int(struct drm_printer *p, const char
> > *name,
> > +                            int val)
> > +{
> > +       drm_printf(p, "xe.%s=%d\n", name, val);
> > +}
> > +
> > +static void _param_print_uint(struct drm_printer *p, const char
> > *name,
> > +                             unsigned int val)
> > +{
> > +       drm_printf(p, "xe.%s=%u\n", name, val);
> > +}
> > +
> > +static void _param_print_ulong(struct drm_printer *p, const char
> > *name,
> > +                              unsigned long val)
> > +{
> > +       drm_printf(p, "xe.%s=%lu\n", name, val);
> > +}
> > +
> > +static void _param_print_charp(struct drm_printer *p, const char
> > *name,
> > +                              const char *val)
> > +{
> > +       drm_printf(p, "xe.%s=%s\n", name, val);
> > +}
> > +
> > +#define _param_print(p, name, val)                             \
> > +       _Generic(val,                                           \
> > +                bool: _param_print_bool,                       \
> > +                int: _param_print_int,                         \
> > +                unsigned int: _param_print_uint,               \
> > +                unsigned long: _param_print_ulong,             \
> > +                char *: _param_print_charp)(p, name, val)
> > +
> > +/**
> > + * xe_params_dump - dump xe modparams
> > + * @params: xe modparams
> > + * @p: the &drm_printer
> > + *
> > + * Pretty printer for xe modparams.
> > + */
> > +void xe_params_dump(const struct xe_params *params, struct
> > drm_printer *p)
> > +{
> > +#define PRINT(T, x, ...) _param_print(p, #x, params->x);
> > +       XE_PARAMS_FOR_EACH(PRINT);
> > +#undef PRINT
> > +}
> > +
> > +static void _param_dup_charp(char **valp)
> > +{
> > +       *valp = kstrdup(*valp, GFP_ATOMIC);
> > +}
> > +
> > +static void _param_nop(void *valp)
> > +{
> > +}
> > +
> > +#define _param_dup(valp)                               \
> > +       _Generic(valp,                                  \
> > +                char **: _param_dup_charp,             \
> > +                default: _param_nop)(valp)
> > +
> > +void xe_params_copy(struct xe_params *dest, const struct xe_params
> > *src)
> > +{
> > +       *dest = *src;
> > +#define DUP(T, x, ...) _param_dup(&dest->x);
> > +       XE_PARAMS_FOR_EACH(DUP);
> > +#undef DUP
> > +}
> > +
> > +static void _param_free_charp(char **valp)
> > +{
> > +       kfree(*valp);
> > +       *valp = NULL;
> > +}
> > +
> > +#define _param_free(valp)                              \
> > +       _Generic(valp,                                  \
> > +                char **: _param_free_charp,            \
> > +                default: _param_nop)(valp)
> > +
> > +/* free the allocated members, *not* the passed in params itself
> > */
> > +void xe_params_free(struct xe_params *params)
> > +{
> > +#define FREE(T, x, ...) _param_free(&params->x);
> > +       XE_PARAMS_FOR_EACH(FREE);
> > +#undef FREE
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_params.h
> > b/drivers/gpu/drm/xe/xe_params.h
> > new file mode 100644
> > index 000000000000..26fb592e9cd6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_params.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#ifndef _XE_PARAMS_H_
> > +#define _XE_PARAMS_H_
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/cache.h> /* for __read_mostly */
> > +
> > +struct drm_printer;
> > +
> > +/*
> > + * Invoke param, a function-like macro, for each xe param, with
> > arguments:
> > + *
> > + * param(type, name, value, mode)
> > + *
> > + * type: parameter type, one of {bool, int, unsigned int, unsigned
> > long, char *}
> > + * name: name of the parameter
> > + * value: initial/default value of the parameter
> > + * mode: debugfs file permissions, one of {0400, 0600, 0}, use 0
> > to not create
> > + *       debugfs file
> > + */
> > +#define XE_PARAMS_FOR_EACH(param) \
> > +       param(bool, dummy, true, 0444)
> > +
> > +#define MEMBER(T, member, ...) T member;
> > +struct xe_params {
> > +       XE_PARAMS_FOR_EACH(MEMBER);
> > +};
> > +#undef MEMBER
> > +
> > +extern struct xe_params xe_modparams __read_mostly;
> > +
> > +void xe_params_dump(const struct xe_params *params, struct
> > drm_printer *p);
> > +void xe_params_copy(struct xe_params *dest, const struct xe_params
> > *src);
> > +void xe_params_free(struct xe_params *params);
> > +
> > +#endif
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c
> > b/drivers/gpu/drm/xe/xe_pci.c
> > index f8ca179254fa..11da5abe587f 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -22,6 +22,8 @@
> >  #include "xe_module.h"
> >  #include "xe_pm.h"
> >  #include "xe_step.h"
> > +#include "xe_params.h"
> > +#include "xe_debugfs.h"
> >  
> >  #define DEV_INFO_FOR_EACH_FLAG(func) \
> >         func(require_force_probe); \
> > @@ -370,6 +372,8 @@ static void xe_pci_remove(struct pci_dev *pdev)
> >         if (!xe) /* driver load aborted, nothing to cleanup */
> >                 return;
> >  
> > +       xe_params_free(&xe->info.params);
> > +
> >         xe_device_remove(xe);
> >         xe_pm_runtime_fini(xe);
> >         pci_set_drvdata(pdev, NULL);
> > @@ -406,6 +410,8 @@ static int xe_pci_probe(struct pci_dev *pdev,
> > const struct pci_device_id *ent)
> >         if (IS_ERR(xe))
> >                 return PTR_ERR(xe);
> >  
> > +       xe_params_copy(&xe->info.params, &xe_modparams);
> > +
> >         xe->info.graphics_verx100 = desc->graphics_ver * 100 +
> >                                     desc->graphics_rel;
> >         xe->info.media_verx100 = desc->media_ver * 100 +
> > -- 
> > 2.38.1.143.g1fc3c0ad40
> >