On Thu, 05 Dec 2019, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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 <jani.nikula@xxxxxxxxx>
>> ---
>> 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, ¶ms->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
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
