[1/2] drm/i915/params: add i915 parameters to debugfs

Commit Message

Jani Nikula Dec. 5, 2019, 3:43 p.m. UTC

Add a debugfs subdirectory i915_params with all the i915 module
parameters. This is a first step, with lots of boilerplate, and not much
benefit yet.

This will result in a new device specific debugfs directory at
/sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
sysfs directory at /sys/module/i915/parameters/. Going forward, all
users of the parameters should use the debugfs, with the module
parameters being phased out.

Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
mode with module parameter sysfs, but the goal is to make the module
parameters read-only initial values for device specific parameters.

0 mode will bypass debugfs creation. Use it for verbose_state_checks
which will need special attention in follow-up work.

Signed-off-by: Jani Nikula <[e-mail]>
---
 drivers/gpu/drm/i915/Makefile              |   5 +-
 drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
 drivers/gpu/drm/i915/i915_debugfs_params.c | 233 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs_params.h |  14 ++
 drivers/gpu/drm/i915/i915_params.c         |   2 +-
 drivers/gpu/drm/i915/i915_params.h         |  76 +++----
 6 files changed, 294 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.c
 create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.h

Comments

Tvrtko Ursulin Dec. 5, 2019, 5:19 p.m. UTC | #1

On 05/12/2019 15:43, Jani Nikula wrote:
> Add a debugfs subdirectory i915_params with all the i915 module
> parameters. This is a first step, with lots of boilerplate, and not much
> benefit yet.
> 
> This will result in a new device specific debugfs directory at
> /sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
> sysfs directory at /sys/module/i915/parameters/. Going forward, all
> users of the parameters should use the debugfs, with the module
> parameters being phased out.

Does it perhaps needs to stay in sysfs (as modparams are)? Unless 
thinking is there will be no need ever to use one of them in production?

With phasing out of modparams - you will not keep a mechanism to pass in 
i915_params at boot time, perhaps targeting cards using pci ids?

Regards,

Tvrtko

> Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
> mode with module parameter sysfs, but the goal is to make the module
> parameters read-only initial values for device specific parameters.
> 
> 0 mode will bypass debugfs creation. Use it for verbose_state_checks
> which will need special attention in follow-up work.
> 
> Signed-off-by: Jani Nikula <[e-mail]>
> ---
>   drivers/gpu/drm/i915/Makefile              |   5 +-
>   drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>   drivers/gpu/drm/i915/i915_debugfs_params.c | 233 +++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_debugfs_params.h |  14 ++
>   drivers/gpu/drm/i915/i915_params.c         |   2 +-
>   drivers/gpu/drm/i915/i915_params.h         |  76 +++----
>   6 files changed, 294 insertions(+), 40 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.c
>   create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e0fd10c0cfb8..db4bf6ae9127 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -69,7 +69,10 @@ i915-y += \
>   	i915_user_extensions.o
>   
>   i915-$(CONFIG_COMPAT)   += i915_ioc32.o
> -i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o display/intel_pipe_crc.o
> +i915-$(CONFIG_DEBUG_FS) += \
> +	i915_debugfs.o \
> +	i915_debugfs_params.o \
> +	display/intel_pipe_crc.o
>   i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
>   
>   # "Graphics Technology" (aka we talk to the gpu)
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index eb80a2c4b55b..b50af2f7865f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -48,6 +48,7 @@
>   #include "gt/uc/intel_guc_submission.h"
>   
>   #include "i915_debugfs.h"
> +#include "i915_debugfs_params.h"
>   #include "i915_irq.h"
>   #include "i915_trace.h"
>   #include "intel_csr.h"
> @@ -4352,9 +4353,10 @@ int i915_debugfs_register(struct drm_i915_private *dev_priv)
>   	struct drm_minor *minor = dev_priv->drm.primary;
>   	int i;
>   
> +	i915_debugfs_params(dev_priv);
> +
>   	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
>   			    to_i915(minor->dev), &i915_forcewake_fops);
> -
>   	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
>   		debugfs_create_file(i915_debugfs_files[i].name,
>   				    S_IRUGO | S_IWUSR,
> diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.c b/drivers/gpu/drm/i915/i915_debugfs_params.c
> new file mode 100644
> index 000000000000..7f1af5a35ca1
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_debugfs_params.c
> @@ -0,0 +1,233 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +
> +#include "i915_drv.h"
> +#include "i915_params.h"
> +
> +/* int param */
> +static int i915_param_int_show(struct seq_file *m, void *data)
> +{
> +	int *value = m->private;
> +
> +	seq_printf(m, "%d\n", *value);
> +
> +	return 0;
> +}
> +
> +static int i915_param_int_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, i915_param_int_show, inode->i_private);
> +}
> +
> +static ssize_t i915_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);
> +
> +	return ret ?: len;
> +}
> +
> +static const struct file_operations i915_param_int_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_int_open,
> +	.read = seq_read,
> +	.write = i915_param_int_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations i915_param_int_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_int_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +/* unsigned int param */
> +static int i915_param_uint_show(struct seq_file *m, void *data)
> +{
> +	unsigned int *value = m->private;
> +
> +	seq_printf(m, "%u\n", *value);
> +
> +	return 0;
> +}
> +
> +static int i915_param_uint_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, i915_param_uint_show, inode->i_private);
> +}
> +
> +static ssize_t i915_param_uint_write(struct file *file,
> +				     const char __user *ubuf, size_t len,
> +				     loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	unsigned int *value = m->private;
> +	int ret;
> +
> +	ret = kstrtouint_from_user(ubuf, len, 0, value);
> +
> +	return ret ?: len;
> +}
> +
> +static const struct file_operations i915_param_uint_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_uint_open,
> +	.read = seq_read,
> +	.write = i915_param_uint_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations i915_param_uint_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_uint_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +/* char * param */
> +static int i915_param_charp_show(struct seq_file *m, void *data)
> +{
> +	const char **s = m->private;
> +
> +	seq_printf(m, "%s\n", *s);
> +
> +	return 0;
> +}
> +
> +static int i915_param_charp_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, i915_param_charp_show, inode->i_private);
> +}
> +
> +static ssize_t i915_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;
> +
> +	/* FIXME: remove locking after params aren't the module params */
> +	kernel_param_lock(THIS_MODULE);
> +
> +	old = *s;
> +	new = strndup_user(ubuf, PAGE_SIZE);
> +	if (IS_ERR(new)) {
> +		len = PTR_ERR(new);
> +		goto out;
> +	}
> +
> +	*s = new;
> +
> +	kfree(old);
> +out:
> +	kernel_param_unlock(THIS_MODULE);
> +
> +	return len;
> +}
> +
> +static const struct file_operations i915_param_charp_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_charp_open,
> +	.read = seq_read,
> +	.write = i915_param_charp_write,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +static const struct file_operations i915_param_charp_fops_ro = {
> +	.owner = THIS_MODULE,
> +	.open = i915_param_charp_open,
> +	.read = seq_read,
> +	.llseek = default_llseek,
> +	.release = single_release,
> +};
> +
> +#define RO(mode) (((mode) & 0222) == 0)
> +
> +static struct dentry *
> +i915_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) ? &i915_param_int_fops_ro :
> +					  &i915_param_int_fops);
> +}
> +
> +static struct dentry *
> +i915_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) ? &i915_param_uint_fops_ro :
> +					  &i915_param_uint_fops);
> +}
> +
> +static struct dentry *
> +i915_debugfs_create_charp(const char *name, umode_t mode,
> +			  struct dentry *parent, char **value)
> +{
> +	return debugfs_create_file(name, mode, parent, value,
> +				   RO(mode) ? &i915_param_charp_fops_ro :
> +				   &i915_param_charp_fops);
> +}
> +
> +static __always_inline void
> +_i915_param_create_file(struct dentry *parent, const char *name,
> +			const char *type, int mode, void *value)
> +{
> +	if (!mode)
> +		return;
> +
> +	if (!__builtin_strcmp(type, "bool"))
> +		debugfs_create_bool(name, mode, parent, value);
> +	else if (!__builtin_strcmp(type, "int"))
> +		i915_debugfs_create_int(name, mode, parent, value);
> +	else if (!__builtin_strcmp(type, "unsigned int"))
> +		i915_debugfs_create_uint(name, mode, parent, value);
> +	else if (!__builtin_strcmp(type, "unsigned long"))
> +		debugfs_create_ulong(name, mode, parent, value);
> +	else if (!__builtin_strcmp(type, "char *"))
> +		i915_debugfs_create_charp(name, mode, parent, value);
> +	else
> +		WARN(1, "no debugfs fops defined for param type %s (i915.%s)\n",
> +		     type, name);
> +}
> +
> +/* add a subdirectory with files for each i915 param */
> +struct dentry *i915_debugfs_params(struct drm_i915_private *i915)
> +{
> +	struct drm_minor *minor = i915->drm.primary;
> +	struct i915_params *params = &i915_modparams;
> +	struct dentry *dir;
> +
> +	dir = debugfs_create_dir("i915_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, ...) _i915_param_create_file(dir, #x, #T, mode, &params->x);
> +	I915_PARAMS_FOR_EACH(REGISTER);
> +#undef REGISTER
> +
> +	return dir;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.h b/drivers/gpu/drm/i915/i915_debugfs_params.h
> new file mode 100644
> index 000000000000..66567076546b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_debugfs_params.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __I915_DEBUGFS_PARAMS__
> +#define __I915_DEBUGFS_PARAMS__
> +
> +struct dentry;
> +struct drm_i915_private;
> +
> +struct dentry *i915_debugfs_params(struct drm_i915_private *i915);
> +
> +#endif /* __I915_DEBUGFS_PARAMS__ */
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 1dd1f3652795..64009e99073d 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -35,7 +35,7 @@
>   	MODULE_PARM_DESC(name, desc)
>   
>   struct i915_params i915_modparams __read_mostly = {
> -#define MEMBER(T, member, value) .member = (value),
> +#define MEMBER(T, member, value, ...) .member = (value),
>   	I915_PARAMS_FOR_EACH(MEMBER)
>   #undef MEMBER
>   };
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 31b88f297fbc..be6089e4f9e6 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -36,49 +36,51 @@ struct drm_printer;
>   /*
>    * Invoke param, a function-like macro, for each i915 param, with arguments:
>    *
> - * param(type, name, value)
> + * param(type, name, value, mode)
>    *
> - * type: parameter type, one of {bool, int, unsigned int, char *}
> + * 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 I915_PARAMS_FOR_EACH(param) \
> -	param(char *, vbt_firmware, NULL) \
> -	param(int, modeset, -1) \
> -	param(int, lvds_channel_mode, 0) \
> -	param(int, panel_use_ssc, -1) \
> -	param(int, vbt_sdvo_panel_type, -1) \
> -	param(int, enable_dc, -1) \
> -	param(int, enable_fbc, -1) \
> -	param(int, enable_psr, -1) \
> -	param(int, disable_power_well, -1) \
> -	param(int, enable_ips, 1) \
> -	param(int, invert_brightness, 0) \
> -	param(int, enable_guc, 0) \
> -	param(int, guc_log_level, -1) \
> -	param(char *, guc_firmware_path, NULL) \
> -	param(char *, huc_firmware_path, NULL) \
> -	param(char *, dmc_firmware_path, NULL) \
> -	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO)) \
> -	param(int, edp_vswing, 0) \
> -	param(int, reset, 3) \
> -	param(unsigned int, inject_probe_failure, 0) \
> -	param(int, fastboot, -1) \
> -	param(int, enable_dpcd_backlight, 0) \
> -	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE) \
> -	param(unsigned long, fake_lmem_start, 0) \
> +	param(char *, vbt_firmware, NULL, 0400) \
> +	param(int, modeset, -1, 0400) \
> +	param(int, lvds_channel_mode, 0, 0400) \
> +	param(int, panel_use_ssc, -1, 0600) \
> +	param(int, vbt_sdvo_panel_type, -1, 0400) \
> +	param(int, enable_dc, -1, 0400) \
> +	param(int, enable_fbc, -1, 0600) \
> +	param(int, enable_psr, -1, 0600) \
> +	param(int, disable_power_well, -1, 0400) \
> +	param(int, enable_ips, 1, 0600) \
> +	param(int, invert_brightness, 0, 0600) \
> +	param(int, enable_guc, 0, 0400) \
> +	param(int, guc_log_level, -1, 0400) \
> +	param(char *, guc_firmware_path, NULL, 0400) \
> +	param(char *, huc_firmware_path, NULL, 0400) \
> +	param(char *, dmc_firmware_path, NULL, 0400) \
> +	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
> +	param(int, edp_vswing, 0, 0400) \
> +	param(int, reset, 3, 0600) \
> +	param(unsigned int, inject_probe_failure, 0, 0600) \
> +	param(int, fastboot, -1, 0600) \
> +	param(int, enable_dpcd_backlight, 0, 0600) \
> +	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE, 0400) \
> +	param(unsigned long, fake_lmem_start, 0, 0400) \
>   	/* 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) \
> -	param(bool, prefault_disable, false) \
> -	param(bool, load_detect_test, false) \
> -	param(bool, force_reset_modeset_test, false) \
> -	param(bool, error_capture, true) \
> -	param(bool, disable_display, false) \
> -	param(bool, verbose_state_checks, true) \
> -	param(bool, nuclear_pageflip, false) \
> -	param(bool, enable_dp_mst, true) \
> -	param(bool, enable_gvt, false)
> +	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT), 0400) \
> +	param(bool, enable_hangcheck, true, 0600) \
> +	param(bool, prefault_disable, false, 0600) \
> +	param(bool, load_detect_test, false, 0600) \
> +	param(bool, force_reset_modeset_test, false, 0600) \
> +	param(bool, error_capture, true, 0600) \
> +	param(bool, disable_display, false, 0400) \
> +	param(bool, verbose_state_checks, true, 0) \
> +	param(bool, nuclear_pageflip, false, 0400) \
> +	param(bool, enable_dp_mst, true, 0600) \
> +	param(bool, enable_gvt, false, 0400)
>   
>   #define MEMBER(T, member, ...) T member;
>   struct i915_params {
>

Jani Nikula Dec. 9, 2019, 9:53 a.m. UTC | #2

On Thu, 05 Dec 2019, Tvrtko Ursulin <[e-mail]> wrote:
> On 05/12/2019 15:43, Jani Nikula wrote:
>> Add a debugfs subdirectory i915_params with all the i915 module
>> parameters. This is a first step, with lots of boilerplate, and not much
>> benefit yet.
>> 
>> This will result in a new device specific debugfs directory at
>> /sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
>> sysfs directory at /sys/module/i915/parameters/. Going forward, all
>> users of the parameters should use the debugfs, with the module
>> parameters being phased out.
>
> Does it perhaps needs to stay in sysfs (as modparams are)? Unless 
> thinking is there will be no need ever to use one of them in production?
>
> With phasing out of modparams - you will not keep a mechanism to pass in 
> i915_params at boot time, perhaps targeting cards using pci ids?

Okay, so we may need to retain *some* module parameters as a module
specific way to set initial values for *some* of the device specific
parameters.

But the interface for modifying the device specific parameters should
really be debugfs. For starters, 26 out of the 35 parameters are
"unsafe" and taint the kernel. (Maybe the new debugfs interface should
taint the kernel too.) They are purely for debugging by the developers,
and they have no place in production or as an ABI.

The remaining "non-unsafe" ones are arguably more or less debugging too:

- disable_display
- enable_dpcd_backlight
- enable_gvt
- error_capture
- fastboot
- guc_log_level
- mmio_debug
- modeset
- verbose_state_checks

I can't really make a strong argument for any of those to be part of the
ABI in sysfs. Maybe there are some exceptions, but the vast majority of
the current module parameters are clearly for debugging only.

BR,
Jani.


>
> Regards,
>
> Tvrtko
>
>> Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
>> mode with module parameter sysfs, but the goal is to make the module
>> parameters read-only initial values for device specific parameters.
>> 
>> 0 mode will bypass debugfs creation. Use it for verbose_state_checks
>> which will need special attention in follow-up work.
>> 
>> Signed-off-by: Jani Nikula <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/Makefile              |   5 +-
>>   drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>>   drivers/gpu/drm/i915/i915_debugfs_params.c | 233 +++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_debugfs_params.h |  14 ++
>>   drivers/gpu/drm/i915/i915_params.c         |   2 +-
>>   drivers/gpu/drm/i915/i915_params.h         |  76 +++----
>>   6 files changed, 294 insertions(+), 40 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.c
>>   create mode 100644 drivers/gpu/drm/i915/i915_debugfs_params.h
>> 
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e0fd10c0cfb8..db4bf6ae9127 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -69,7 +69,10 @@ i915-y += \
>>   	i915_user_extensions.o
>>   
>>   i915-$(CONFIG_COMPAT)   += i915_ioc32.o
>> -i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o display/intel_pipe_crc.o
>> +i915-$(CONFIG_DEBUG_FS) += \
>> +	i915_debugfs.o \
>> +	i915_debugfs_params.o \
>> +	display/intel_pipe_crc.o
>>   i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
>>   
>>   # "Graphics Technology" (aka we talk to the gpu)
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index eb80a2c4b55b..b50af2f7865f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -48,6 +48,7 @@
>>   #include "gt/uc/intel_guc_submission.h"
>>   
>>   #include "i915_debugfs.h"
>> +#include "i915_debugfs_params.h"
>>   #include "i915_irq.h"
>>   #include "i915_trace.h"
>>   #include "intel_csr.h"
>> @@ -4352,9 +4353,10 @@ int i915_debugfs_register(struct drm_i915_private *dev_priv)
>>   	struct drm_minor *minor = dev_priv->drm.primary;
>>   	int i;
>>   
>> +	i915_debugfs_params(dev_priv);
>> +
>>   	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
>>   			    to_i915(minor->dev), &i915_forcewake_fops);
>> -
>>   	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
>>   		debugfs_create_file(i915_debugfs_files[i].name,
>>   				    S_IRUGO | S_IWUSR,
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.c b/drivers/gpu/drm/i915/i915_debugfs_params.c
>> new file mode 100644
>> index 000000000000..7f1af5a35ca1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_debugfs_params.c
>> @@ -0,0 +1,233 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2019 Intel Corporation
>> + */
>> +
>> +#include <linux/kernel.h>
>> +
>> +#include "i915_drv.h"
>> +#include "i915_params.h"
>> +
>> +/* int param */
>> +static int i915_param_int_show(struct seq_file *m, void *data)
>> +{
>> +	int *value = m->private;
>> +
>> +	seq_printf(m, "%d\n", *value);
>> +
>> +	return 0;
>> +}
>> +
>> +static int i915_param_int_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, i915_param_int_show, inode->i_private);
>> +}
>> +
>> +static ssize_t i915_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);
>> +
>> +	return ret ?: len;
>> +}
>> +
>> +static const struct file_operations i915_param_int_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_int_open,
>> +	.read = seq_read,
>> +	.write = i915_param_int_write,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +static const struct file_operations i915_param_int_fops_ro = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_int_open,
>> +	.read = seq_read,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +/* unsigned int param */
>> +static int i915_param_uint_show(struct seq_file *m, void *data)
>> +{
>> +	unsigned int *value = m->private;
>> +
>> +	seq_printf(m, "%u\n", *value);
>> +
>> +	return 0;
>> +}
>> +
>> +static int i915_param_uint_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, i915_param_uint_show, inode->i_private);
>> +}
>> +
>> +static ssize_t i915_param_uint_write(struct file *file,
>> +				     const char __user *ubuf, size_t len,
>> +				     loff_t *offp)
>> +{
>> +	struct seq_file *m = file->private_data;
>> +	unsigned int *value = m->private;
>> +	int ret;
>> +
>> +	ret = kstrtouint_from_user(ubuf, len, 0, value);
>> +
>> +	return ret ?: len;
>> +}
>> +
>> +static const struct file_operations i915_param_uint_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_uint_open,
>> +	.read = seq_read,
>> +	.write = i915_param_uint_write,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +static const struct file_operations i915_param_uint_fops_ro = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_uint_open,
>> +	.read = seq_read,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +/* char * param */
>> +static int i915_param_charp_show(struct seq_file *m, void *data)
>> +{
>> +	const char **s = m->private;
>> +
>> +	seq_printf(m, "%s\n", *s);
>> +
>> +	return 0;
>> +}
>> +
>> +static int i915_param_charp_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, i915_param_charp_show, inode->i_private);
>> +}
>> +
>> +static ssize_t i915_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;
>> +
>> +	/* FIXME: remove locking after params aren't the module params */
>> +	kernel_param_lock(THIS_MODULE);
>> +
>> +	old = *s;
>> +	new = strndup_user(ubuf, PAGE_SIZE);
>> +	if (IS_ERR(new)) {
>> +		len = PTR_ERR(new);
>> +		goto out;
>> +	}
>> +
>> +	*s = new;
>> +
>> +	kfree(old);
>> +out:
>> +	kernel_param_unlock(THIS_MODULE);
>> +
>> +	return len;
>> +}
>> +
>> +static const struct file_operations i915_param_charp_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_charp_open,
>> +	.read = seq_read,
>> +	.write = i915_param_charp_write,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +static const struct file_operations i915_param_charp_fops_ro = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_param_charp_open,
>> +	.read = seq_read,
>> +	.llseek = default_llseek,
>> +	.release = single_release,
>> +};
>> +
>> +#define RO(mode) (((mode) & 0222) == 0)
>> +
>> +static struct dentry *
>> +i915_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) ? &i915_param_int_fops_ro :
>> +					  &i915_param_int_fops);
>> +}
>> +
>> +static struct dentry *
>> +i915_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) ? &i915_param_uint_fops_ro :
>> +					  &i915_param_uint_fops);
>> +}
>> +
>> +static struct dentry *
>> +i915_debugfs_create_charp(const char *name, umode_t mode,
>> +			  struct dentry *parent, char **value)
>> +{
>> +	return debugfs_create_file(name, mode, parent, value,
>> +				   RO(mode) ? &i915_param_charp_fops_ro :
>> +				   &i915_param_charp_fops);
>> +}
>> +
>> +static __always_inline void
>> +_i915_param_create_file(struct dentry *parent, const char *name,
>> +			const char *type, int mode, void *value)
>> +{
>> +	if (!mode)
>> +		return;
>> +
>> +	if (!__builtin_strcmp(type, "bool"))
>> +		debugfs_create_bool(name, mode, parent, value);
>> +	else if (!__builtin_strcmp(type, "int"))
>> +		i915_debugfs_create_int(name, mode, parent, value);
>> +	else if (!__builtin_strcmp(type, "unsigned int"))
>> +		i915_debugfs_create_uint(name, mode, parent, value);
>> +	else if (!__builtin_strcmp(type, "unsigned long"))
>> +		debugfs_create_ulong(name, mode, parent, value);
>> +	else if (!__builtin_strcmp(type, "char *"))
>> +		i915_debugfs_create_charp(name, mode, parent, value);
>> +	else
>> +		WARN(1, "no debugfs fops defined for param type %s (i915.%s)\n",
>> +		     type, name);
>> +}
>> +
>> +/* add a subdirectory with files for each i915 param */
>> +struct dentry *i915_debugfs_params(struct drm_i915_private *i915)
>> +{
>> +	struct drm_minor *minor = i915->drm.primary;
>> +	struct i915_params *params = &i915_modparams;
>> +	struct dentry *dir;
>> +
>> +	dir = debugfs_create_dir("i915_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, ...) _i915_param_create_file(dir, #x, #T, mode, &params->x);
>> +	I915_PARAMS_FOR_EACH(REGISTER);
>> +#undef REGISTER
>> +
>> +	return dir;
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.h b/drivers/gpu/drm/i915/i915_debugfs_params.h
>> new file mode 100644
>> index 000000000000..66567076546b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_debugfs_params.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2019 Intel Corporation
>> + */
>> +
>> +#ifndef __I915_DEBUGFS_PARAMS__
>> +#define __I915_DEBUGFS_PARAMS__
>> +
>> +struct dentry;
>> +struct drm_i915_private;
>> +
>> +struct dentry *i915_debugfs_params(struct drm_i915_private *i915);
>> +
>> +#endif /* __I915_DEBUGFS_PARAMS__ */
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 1dd1f3652795..64009e99073d 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -35,7 +35,7 @@
>>   	MODULE_PARM_DESC(name, desc)
>>   
>>   struct i915_params i915_modparams __read_mostly = {
>> -#define MEMBER(T, member, value) .member = (value),
>> +#define MEMBER(T, member, value, ...) .member = (value),
>>   	I915_PARAMS_FOR_EACH(MEMBER)
>>   #undef MEMBER
>>   };
>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>> index 31b88f297fbc..be6089e4f9e6 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -36,49 +36,51 @@ struct drm_printer;
>>   /*
>>    * Invoke param, a function-like macro, for each i915 param, with arguments:
>>    *
>> - * param(type, name, value)
>> + * param(type, name, value, mode)
>>    *
>> - * type: parameter type, one of {bool, int, unsigned int, char *}
>> + * 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 I915_PARAMS_FOR_EACH(param) \
>> -	param(char *, vbt_firmware, NULL) \
>> -	param(int, modeset, -1) \
>> -	param(int, lvds_channel_mode, 0) \
>> -	param(int, panel_use_ssc, -1) \
>> -	param(int, vbt_sdvo_panel_type, -1) \
>> -	param(int, enable_dc, -1) \
>> -	param(int, enable_fbc, -1) \
>> -	param(int, enable_psr, -1) \
>> -	param(int, disable_power_well, -1) \
>> -	param(int, enable_ips, 1) \
>> -	param(int, invert_brightness, 0) \
>> -	param(int, enable_guc, 0) \
>> -	param(int, guc_log_level, -1) \
>> -	param(char *, guc_firmware_path, NULL) \
>> -	param(char *, huc_firmware_path, NULL) \
>> -	param(char *, dmc_firmware_path, NULL) \
>> -	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO)) \
>> -	param(int, edp_vswing, 0) \
>> -	param(int, reset, 3) \
>> -	param(unsigned int, inject_probe_failure, 0) \
>> -	param(int, fastboot, -1) \
>> -	param(int, enable_dpcd_backlight, 0) \
>> -	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE) \
>> -	param(unsigned long, fake_lmem_start, 0) \
>> +	param(char *, vbt_firmware, NULL, 0400) \
>> +	param(int, modeset, -1, 0400) \
>> +	param(int, lvds_channel_mode, 0, 0400) \
>> +	param(int, panel_use_ssc, -1, 0600) \
>> +	param(int, vbt_sdvo_panel_type, -1, 0400) \
>> +	param(int, enable_dc, -1, 0400) \
>> +	param(int, enable_fbc, -1, 0600) \
>> +	param(int, enable_psr, -1, 0600) \
>> +	param(int, disable_power_well, -1, 0400) \
>> +	param(int, enable_ips, 1, 0600) \
>> +	param(int, invert_brightness, 0, 0600) \
>> +	param(int, enable_guc, 0, 0400) \
>> +	param(int, guc_log_level, -1, 0400) \
>> +	param(char *, guc_firmware_path, NULL, 0400) \
>> +	param(char *, huc_firmware_path, NULL, 0400) \
>> +	param(char *, dmc_firmware_path, NULL, 0400) \
>> +	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
>> +	param(int, edp_vswing, 0, 0400) \
>> +	param(int, reset, 3, 0600) \
>> +	param(unsigned int, inject_probe_failure, 0, 0600) \
>> +	param(int, fastboot, -1, 0600) \
>> +	param(int, enable_dpcd_backlight, 0, 0600) \
>> +	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE, 0400) \
>> +	param(unsigned long, fake_lmem_start, 0, 0400) \
>>   	/* 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) \
>> -	param(bool, prefault_disable, false) \
>> -	param(bool, load_detect_test, false) \
>> -	param(bool, force_reset_modeset_test, false) \
>> -	param(bool, error_capture, true) \
>> -	param(bool, disable_display, false) \
>> -	param(bool, verbose_state_checks, true) \
>> -	param(bool, nuclear_pageflip, false) \
>> -	param(bool, enable_dp_mst, true) \
>> -	param(bool, enable_gvt, false)
>> +	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT), 0400) \
>> +	param(bool, enable_hangcheck, true, 0600) \
>> +	param(bool, prefault_disable, false, 0600) \
>> +	param(bool, load_detect_test, false, 0600) \
>> +	param(bool, force_reset_modeset_test, false, 0600) \
>> +	param(bool, error_capture, true, 0600) \
>> +	param(bool, disable_display, false, 0400) \
>> +	param(bool, verbose_state_checks, true, 0) \
>> +	param(bool, nuclear_pageflip, false, 0400) \
>> +	param(bool, enable_dp_mst, true, 0600) \
>> +	param(bool, enable_gvt, false, 0400)
>>   
>>   #define MEMBER(T, member, ...) T member;
>>   struct i915_params {
>> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson Jan. 8, 2020, 2:45 p.m. UTC | #3

Quoting Jani Nikula (2019-12-05 15:43:40)
> +static int i915_param_int_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, i915_param_int_show, inode->i_private);

What I've always wanted with this style of approach was a means that the
parameter is only set while the debugfs remained open.

	fd = open("/debug/my_parameter", O_WRONLY | O_EXCL);
	write(fd, "1", 1);

	... run test ..

System reverts to default on process termination, or explicit close(fd).

I'd make the open implicitly O_EXCL, i.e. return -EBUSY if something
else already holds the parameter set. Or, you can use the O_EXCL to
select between the different modes of operation.

Moving the parameters to debugfs is more than worth it imo if we can
enable this mode of operation.
-Chris

Jani Nikula Jan. 8, 2020, 3:07 p.m. UTC | #4

On Wed, 08 Jan 2020, Chris Wilson <[e-mail]> wrote:
> Quoting Jani Nikula (2019-12-05 15:43:40)
>> +static int i915_param_int_open(struct inode *inode, struct file *file)
>> +{
>> +       return single_open(file, i915_param_int_show, inode->i_private);
>
> What I've always wanted with this style of approach was a means that the
> parameter is only set while the debugfs remained open.
>
> 	fd = open("/debug/my_parameter", O_WRONLY | O_EXCL);
> 	write(fd, "1", 1);
>
> 	... run test ..
>
> System reverts to default on process termination, or explicit close(fd).
>
> I'd make the open implicitly O_EXCL, i.e. return -EBUSY if something
> else already holds the parameter set. Or, you can use the O_EXCL to
> select between the different modes of operation.
>
> Moving the parameters to debugfs is more than worth it imo if we can
> enable this mode of operation.

I understand the use case, and I'd like something like that. But
(obviously?) I think the regular use case of 'echo 1 >
/debug/my_parameter' needs to work as one would expect.

Reading open(2) man page, feels like using O_EXCL for this would be a
hack. Granted, it's debugfs to begin with, but still. Makes me a bit
hesitant about abusing O_EXCL for this. (Or am I missing something?)

BR,
Jani.

Chris Wilson Jan. 15, 2020, 10:04 a.m. UTC | #5

Quoting Jani Nikula (2019-12-05 15:43:40)
> Add a debugfs subdirectory i915_params with all the i915 module
> parameters. This is a first step, with lots of boilerplate, and not much
> benefit yet.

Right, creates a mirror [more or less] of /sys/module/i915/parameters. I
probably would have used parameters/ rather than i915_params/, but that
is immaterial.

> 
> This will result in a new device specific debugfs directory at
> /sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
> sysfs directory at /sys/module/i915/parameters/. Going forward, all
> users of the parameters should use the debugfs, with the module
> parameters being phased out.
> 
> Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
> mode with module parameter sysfs, but the goal is to make the module
> parameters read-only initial values for device specific parameters.
> 
> 0 mode will bypass debugfs creation. Use it for verbose_state_checks
> which will need special attention in follow-up work.

The patch does what you say, hopefully a local entropy maxima.
Reviewed-by: Chris Wilson <[e-mail]>
-Chris

Jani Nikula Jan. 15, 2020, 10:39 a.m. UTC | #6

On Wed, 15 Jan 2020, Chris Wilson <[e-mail]> wrote:
> Quoting Jani Nikula (2019-12-05 15:43:40)
>> Add a debugfs subdirectory i915_params with all the i915 module
>> parameters. This is a first step, with lots of boilerplate, and not much
>> benefit yet.
>
> Right, creates a mirror [more or less] of /sys/module/i915/parameters. I
> probably would have used parameters/ rather than i915_params/, but that
> is immaterial.

I guess I used i915_ because it's sort of a shared namespace with drm
core, which in itself is, IMO, a historical mistake. Perhaps there
should've been a driver/ debugfs directory for driver specific debug
files.

What if someone wanted to add a drm core level parameters/ file or
directory? Would be kind of embarrassing to say, don't do that, i915
already uses it...

>> 
>> This will result in a new device specific debugfs directory at
>> /sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
>> sysfs directory at /sys/module/i915/parameters/. Going forward, all
>> users of the parameters should use the debugfs, with the module
>> parameters being phased out.
>> 
>> Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
>> mode with module parameter sysfs, but the goal is to make the module
>> parameters read-only initial values for device specific parameters.
>> 
>> 0 mode will bypass debugfs creation. Use it for verbose_state_checks
>> which will need special attention in follow-up work.
>
> The patch does what you say, hopefully a local entropy maxima.
> Reviewed-by: Chris Wilson <[e-mail]>

Many thanks,
Jani.

Jani Nikula Jan. 15, 2020, 1:12 p.m. UTC | #7

On Wed, 15 Jan 2020, Jani Nikula <[e-mail]> wrote:
> On Wed, 15 Jan 2020, Chris Wilson <[e-mail]> wrote:
>> Quoting Jani Nikula (2019-12-05 15:43:40)
>>> Add a debugfs subdirectory i915_params with all the i915 module
>>> parameters. This is a first step, with lots of boilerplate, and not much
>>> benefit yet.
>>
>> Right, creates a mirror [more or less] of /sys/module/i915/parameters. I
>> probably would have used parameters/ rather than i915_params/, but that
>> is immaterial.
>
> I guess I used i915_ because it's sort of a shared namespace with drm
> core, which in itself is, IMO, a historical mistake. Perhaps there
> should've been a driver/ debugfs directory for driver specific debug
> files.
>
> What if someone wanted to add a drm core level parameters/ file or
> directory? Would be kind of embarrassing to say, don't do that, i915
> already uses it...
>
>>> 
>>> This will result in a new device specific debugfs directory at
>>> /sys/kernel/debug/dri/<N>/i915_params duplicating the module specific
>>> sysfs directory at /sys/module/i915/parameters/. Going forward, all
>>> users of the parameters should use the debugfs, with the module
>>> parameters being phased out.
>>> 
>>> Add debugfs permissions to I915_PARAMS_FOR_EACH(). This duplicates the
>>> mode with module parameter sysfs, but the goal is to make the module
>>> parameters read-only initial values for device specific parameters.
>>> 
>>> 0 mode will bypass debugfs creation. Use it for verbose_state_checks
>>> which will need special attention in follow-up work.
>>
>> The patch does what you say, hopefully a local entropy maxima.
>> Reviewed-by: Chris Wilson <[e-mail]>
>
> Many thanks,
> Jani.

And pushed both to dinq, thanks again for the review.

BR,
Jani.

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e0fd10c0cfb8..db4bf6ae9127 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -69,7 +69,10 @@  i915-y += \
 	i915_user_extensions.o
 
 i915-$(CONFIG_COMPAT)   += i915_ioc32.o
-i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o display/intel_pipe_crc.o
+i915-$(CONFIG_DEBUG_FS) += \
+	i915_debugfs.o \
+	i915_debugfs_params.o \
+	display/intel_pipe_crc.o
 i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
 
 # "Graphics Technology" (aka we talk to the gpu)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index eb80a2c4b55b..b50af2f7865f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -48,6 +48,7 @@ 
 #include "gt/uc/intel_guc_submission.h"
 
 #include "i915_debugfs.h"
+#include "i915_debugfs_params.h"
 #include "i915_irq.h"
 #include "i915_trace.h"
 #include "intel_csr.h"
@@ -4352,9 +4353,10 @@  int i915_debugfs_register(struct drm_i915_private *dev_priv)
 	struct drm_minor *minor = dev_priv->drm.primary;
 	int i;
 
+	i915_debugfs_params(dev_priv);
+
 	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
 			    to_i915(minor->dev), &i915_forcewake_fops);
-
 	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
 		debugfs_create_file(i915_debugfs_files[i].name,
 				    S_IRUGO | S_IWUSR,
diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.c b/drivers/gpu/drm/i915/i915_debugfs_params.c
new file mode 100644
index 000000000000..7f1af5a35ca1
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_debugfs_params.c
@@ -0,0 +1,233 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/kernel.h>
+
+#include "i915_drv.h"
+#include "i915_params.h"
+
+/* int param */
+static int i915_param_int_show(struct seq_file *m, void *data)
+{
+	int *value = m->private;
+
+	seq_printf(m, "%d\n", *value);
+
+	return 0;
+}
+
+static int i915_param_int_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, i915_param_int_show, inode->i_private);
+}
+
+static ssize_t i915_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);
+
+	return ret ?: len;
+}
+
+static const struct file_operations i915_param_int_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_param_int_open,
+	.read = seq_read,
+	.write = i915_param_int_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations i915_param_int_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = i915_param_int_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+/* unsigned int param */
+static int i915_param_uint_show(struct seq_file *m, void *data)
+{
+	unsigned int *value = m->private;
+
+	seq_printf(m, "%u\n", *value);
+
+	return 0;
+}
+
+static int i915_param_uint_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, i915_param_uint_show, inode->i_private);
+}
+
+static ssize_t i915_param_uint_write(struct file *file,
+				     const char __user *ubuf, size_t len,
+				     loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	unsigned int *value = m->private;
+	int ret;
+
+	ret = kstrtouint_from_user(ubuf, len, 0, value);
+
+	return ret ?: len;
+}
+
+static const struct file_operations i915_param_uint_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_param_uint_open,
+	.read = seq_read,
+	.write = i915_param_uint_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations i915_param_uint_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = i915_param_uint_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+/* char * param */
+static int i915_param_charp_show(struct seq_file *m, void *data)
+{
+	const char **s = m->private;
+
+	seq_printf(m, "%s\n", *s);
+
+	return 0;
+}
+
+static int i915_param_charp_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, i915_param_charp_show, inode->i_private);
+}
+
+static ssize_t i915_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;
+
+	/* FIXME: remove locking after params aren't the module params */
+	kernel_param_lock(THIS_MODULE);
+
+	old = *s;
+	new = strndup_user(ubuf, PAGE_SIZE);
+	if (IS_ERR(new)) {
+		len = PTR_ERR(new);
+		goto out;
+	}
+
+	*s = new;
+
+	kfree(old);
+out:
+	kernel_param_unlock(THIS_MODULE);
+
+	return len;
+}
+
+static const struct file_operations i915_param_charp_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_param_charp_open,
+	.read = seq_read,
+	.write = i915_param_charp_write,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+static const struct file_operations i915_param_charp_fops_ro = {
+	.owner = THIS_MODULE,
+	.open = i915_param_charp_open,
+	.read = seq_read,
+	.llseek = default_llseek,
+	.release = single_release,
+};
+
+#define RO(mode) (((mode) & 0222) == 0)
+
+static struct dentry *
+i915_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) ? &i915_param_int_fops_ro :
+					  &i915_param_int_fops);
+}
+
+static struct dentry *
+i915_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) ? &i915_param_uint_fops_ro :
+					  &i915_param_uint_fops);
+}
+
+static struct dentry *
+i915_debugfs_create_charp(const char *name, umode_t mode,
+			  struct dentry *parent, char **value)
+{
+	return debugfs_create_file(name, mode, parent, value,
+				   RO(mode) ? &i915_param_charp_fops_ro :
+				   &i915_param_charp_fops);
+}
+
+static __always_inline void
+_i915_param_create_file(struct dentry *parent, const char *name,
+			const char *type, int mode, void *value)
+{
+	if (!mode)
+		return;
+
+	if (!__builtin_strcmp(type, "bool"))
+		debugfs_create_bool(name, mode, parent, value);
+	else if (!__builtin_strcmp(type, "int"))
+		i915_debugfs_create_int(name, mode, parent, value);
+	else if (!__builtin_strcmp(type, "unsigned int"))
+		i915_debugfs_create_uint(name, mode, parent, value);
+	else if (!__builtin_strcmp(type, "unsigned long"))
+		debugfs_create_ulong(name, mode, parent, value);
+	else if (!__builtin_strcmp(type, "char *"))
+		i915_debugfs_create_charp(name, mode, parent, value);
+	else
+		WARN(1, "no debugfs fops defined for param type %s (i915.%s)\n",
+		     type, name);
+}
+
+/* add a subdirectory with files for each i915 param */
+struct dentry *i915_debugfs_params(struct drm_i915_private *i915)
+{
+	struct drm_minor *minor = i915->drm.primary;
+	struct i915_params *params = &i915_modparams;
+	struct dentry *dir;
+
+	dir = debugfs_create_dir("i915_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, ...) _i915_param_create_file(dir, #x, #T, mode, &params->x);
+	I915_PARAMS_FOR_EACH(REGISTER);
+#undef REGISTER
+
+	return dir;
+}
diff --git a/drivers/gpu/drm/i915/i915_debugfs_params.h b/drivers/gpu/drm/i915/i915_debugfs_params.h
new file mode 100644
index 000000000000..66567076546b
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_debugfs_params.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __I915_DEBUGFS_PARAMS__
+#define __I915_DEBUGFS_PARAMS__
+
+struct dentry;
+struct drm_i915_private;
+
+struct dentry *i915_debugfs_params(struct drm_i915_private *i915);
+
+#endif /* __I915_DEBUGFS_PARAMS__ */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1dd1f3652795..64009e99073d 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -35,7 +35,7 @@ 
 	MODULE_PARM_DESC(name, desc)
 
 struct i915_params i915_modparams __read_mostly = {
-#define MEMBER(T, member, value) .member = (value),
+#define MEMBER(T, member, value, ...) .member = (value),
 	I915_PARAMS_FOR_EACH(MEMBER)
 #undef MEMBER
 };
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 31b88f297fbc..be6089e4f9e6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -36,49 +36,51 @@  struct drm_printer;
 /*
  * Invoke param, a function-like macro, for each i915 param, with arguments:
  *
- * param(type, name, value)
+ * param(type, name, value, mode)
  *
- * type: parameter type, one of {bool, int, unsigned int, char *}
+ * 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 I915_PARAMS_FOR_EACH(param) \
-	param(char *, vbt_firmware, NULL) \
-	param(int, modeset, -1) \
-	param(int, lvds_channel_mode, 0) \
-	param(int, panel_use_ssc, -1) \
-	param(int, vbt_sdvo_panel_type, -1) \
-	param(int, enable_dc, -1) \
-	param(int, enable_fbc, -1) \
-	param(int, enable_psr, -1) \
-	param(int, disable_power_well, -1) \
-	param(int, enable_ips, 1) \
-	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
-	param(int, guc_log_level, -1) \
-	param(char *, guc_firmware_path, NULL) \
-	param(char *, huc_firmware_path, NULL) \
-	param(char *, dmc_firmware_path, NULL) \
-	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO)) \
-	param(int, edp_vswing, 0) \
-	param(int, reset, 3) \
-	param(unsigned int, inject_probe_failure, 0) \
-	param(int, fastboot, -1) \
-	param(int, enable_dpcd_backlight, 0) \
-	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE) \
-	param(unsigned long, fake_lmem_start, 0) \
+	param(char *, vbt_firmware, NULL, 0400) \
+	param(int, modeset, -1, 0400) \
+	param(int, lvds_channel_mode, 0, 0400) \
+	param(int, panel_use_ssc, -1, 0600) \
+	param(int, vbt_sdvo_panel_type, -1, 0400) \
+	param(int, enable_dc, -1, 0400) \
+	param(int, enable_fbc, -1, 0600) \
+	param(int, enable_psr, -1, 0600) \
+	param(int, disable_power_well, -1, 0400) \
+	param(int, enable_ips, 1, 0600) \
+	param(int, invert_brightness, 0, 0600) \
+	param(int, enable_guc, 0, 0400) \
+	param(int, guc_log_level, -1, 0400) \
+	param(char *, guc_firmware_path, NULL, 0400) \
+	param(char *, huc_firmware_path, NULL, 0400) \
+	param(char *, dmc_firmware_path, NULL, 0400) \
+	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
+	param(int, edp_vswing, 0, 0400) \
+	param(int, reset, 3, 0600) \
+	param(unsigned int, inject_probe_failure, 0, 0600) \
+	param(int, fastboot, -1, 0600) \
+	param(int, enable_dpcd_backlight, 0, 0600) \
+	param(char *, force_probe, CONFIG_DRM_I915_FORCE_PROBE, 0400) \
+	param(unsigned long, fake_lmem_start, 0, 0400) \
 	/* 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) \
-	param(bool, prefault_disable, false) \
-	param(bool, load_detect_test, false) \
-	param(bool, force_reset_modeset_test, false) \
-	param(bool, error_capture, true) \
-	param(bool, disable_display, false) \
-	param(bool, verbose_state_checks, true) \
-	param(bool, nuclear_pageflip, false) \
-	param(bool, enable_dp_mst, true) \
-	param(bool, enable_gvt, false)
+	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT), 0400) \
+	param(bool, enable_hangcheck, true, 0600) \
+	param(bool, prefault_disable, false, 0600) \
+	param(bool, load_detect_test, false, 0600) \
+	param(bool, force_reset_modeset_test, false, 0600) \
+	param(bool, error_capture, true, 0600) \
+	param(bool, disable_display, false, 0400) \
+	param(bool, verbose_state_checks, true, 0) \
+	param(bool, nuclear_pageflip, false, 0400) \
+	param(bool, enable_dp_mst, true, 0600) \
+	param(bool, enable_gvt, false, 0400)
 
 #define MEMBER(T, member, ...) T member;
 struct i915_params {