On Thu, 05 Oct 2023, Jouni Högander <jouni.hogander@xxxxxxxxx> wrote:
> Currently all module parameters are handled by i915_param.c/h. This
> is a problem for display parameters when Xe driver is used. Add
> a mechanism to add parameters specific to the display. This is mainly
> copied from i915_[debugfs]_params.[ch]. Parameters are not yet moved. This
> is done by subsequent patches.
>
> Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 2 +
> .../gpu/drm/i915/display/intel_display_core.h | 2 +
> .../drm/i915/display/intel_display_debugfs.c | 2 +
> .../display/intel_display_debugfs_params.c | 239 ++++++++++++++++++
> .../display/intel_display_debugfs_params.h | 14 +
> .../drm/i915/display/intel_display_device.c | 8 +
> .../drm/i915/display/intel_display_device.h | 1 +
> .../drm/i915/display/intel_display_params.c | 95 +++++++
> .../drm/i915/display/intel_display_params.h | 58 +++++
> drivers/gpu/drm/i915/i915_driver.c | 2 +
> 10 files changed, 423 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_display_debugfs_params.h
> create mode 100644 drivers/gpu/drm/i915/display/intel_display_params.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_display_params.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index dec78efa452a..3c26e9ae3722 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -85,6 +85,7 @@ i915-$(CONFIG_DEBUG_FS) += \
> i915_debugfs.o \
> i915_debugfs_params.o \
> display/intel_display_debugfs.o \
> + display/intel_display_debugfs_params.o \
> display/intel_pipe_crc.o
> i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
>
> @@ -247,6 +248,7 @@ i915-y += \
> display/intel_display.o \
> display/intel_display_driver.o \
> display/intel_display_irq.o \
> + display/intel_display_params.o \
> display/intel_display_power.o \
> display/intel_display_power_map.o \
> display/intel_display_power_well.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 53e5c33e08c3..f2c84ae52217 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -19,6 +19,7 @@
> #include "intel_cdclk.h"
> #include "intel_display_device.h"
> #include "intel_display_limits.h"
> +#include "intel_display_params.h"
> #include "intel_display_power.h"
> #include "intel_dpll_mgr.h"
> #include "intel_fbc.h"
> @@ -517,6 +518,7 @@ struct intel_display {
> struct intel_hotplug hotplug;
> struct intel_opregion opregion;
> struct intel_overlay *overlay;
> + struct intel_display_params params;
> struct intel_vbt_data vbt;
> struct intel_wm wm;
> };
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index f05b52381a83..e929a8bf0555 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -17,6 +17,7 @@
> #include "intel_de.h"
> #include "intel_crtc_state_dump.h"
> #include "intel_display_debugfs.h"
> +#include "intel_display_debugfs_params.h"
> #include "intel_display_power.h"
> #include "intel_display_power_well.h"
> #include "intel_display_types.h"
> @@ -1100,6 +1101,7 @@ void intel_display_debugfs_register(struct drm_i915_private *i915)
> intel_hpd_debugfs_register(i915);
> intel_psr_debugfs_register(i915);
> intel_wm_debugfs_register(i915);
> + intel_display_debugfs_params(i915);
> }
>
> static int i915_panel_show(struct seq_file *m, void *data)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> new file mode 100644
> index 000000000000..62bd61e8ea37
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +
> +#include <drm/drm_drv.h>
> +
> +#include "intel_display_debugfs_params.h"
> +#include "i915_drv.h"
> +#include "intel_display_params.h"
> +
> +/* int param */
> +static int intel_display_param_int_show(struct seq_file *m, void *data)
> +{
> + int *value = m->private;
> +
> + seq_printf(m, "%d\n", *value);
> +
> + return 0;
> +}
> +
> +static int intel_display_param_int_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, intel_display_param_int_show, inode->i_private);
> +}
> +
> +static ssize_t intel_display_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 intel_display_param_int_fops = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_int_open,
> + .read = seq_read,
> + .write = intel_display_param_int_write,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +static const struct file_operations intel_display_param_int_fops_ro = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_int_open,
> + .read = seq_read,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +/* unsigned int param */
> +static int intel_display_param_uint_show(struct seq_file *m, void *data)
> +{
> + unsigned int *value = m->private;
> +
> + seq_printf(m, "%u\n", *value);
> +
> + return 0;
> +}
> +
> +static int intel_display_param_uint_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, intel_display_param_uint_show, inode->i_private);
> +}
> +
> +static ssize_t intel_display_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);
> + 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 intel_display_param_uint_fops = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_uint_open,
> + .read = seq_read,
> + .write = intel_display_param_uint_write,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +static const struct file_operations intel_display_param_uint_fops_ro = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_uint_open,
> + .read = seq_read,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +/* char * param */
> +static int intel_display_param_charp_show(struct seq_file *m, void *data)
> +{
> + const char **s = m->private;
> +
> + seq_printf(m, "%s\n", *s);
> +
> + return 0;
> +}
> +
> +static int intel_display_param_charp_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, intel_display_param_charp_show, inode->i_private);
> +}
> +
> +static ssize_t intel_display_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 intel_display_param_charp_fops = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_charp_open,
> + .read = seq_read,
> + .write = intel_display_param_charp_write,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +static const struct file_operations intel_display_param_charp_fops_ro = {
> + .owner = THIS_MODULE,
> + .open = intel_display_param_charp_open,
> + .read = seq_read,
> + .llseek = default_llseek,
> + .release = single_release,
> +};
> +
> +#define RO(mode) (((mode) & 0222) == 0)
> +
> +static struct dentry *
> +intel_display_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) ? &intel_display_param_int_fops_ro :
> + &intel_display_param_int_fops);
> +}
> +
> +static struct dentry *
> +intel_display_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) ? &intel_display_param_uint_fops_ro :
> + &intel_display_param_uint_fops);
> +}
> +
> +static struct dentry *
> +intel_display_debugfs_create_charp(const char *name, umode_t mode,
> + struct dentry *parent, char **value)
> +{
> + return debugfs_create_file(name, mode, parent, value,
> + RO(mode) ? &intel_display_param_charp_fops_ro :
> + &intel_display_param_charp_fops);
> +}
> +
> +#define _intel_display_param_create_file(parent, name, mode, valp) \
> + do { \
> + if (mode) \
> + _Generic(valp, \
> + bool * : debugfs_create_bool, \
> + int * : intel_display_debugfs_create_int, \
> + unsigned int * : intel_display_debugfs_create_uint, \
> + unsigned long * : debugfs_create_ulong, \
> + char ** : intel_display_debugfs_create_charp) \
> + (name, mode, parent, valp); \
> + } while (0)
I'd try to take the opportunity to convert the param types to ones
supported by debugfs directly, and discard the i915 local crap (yes, I
wrote them) to add more types.
At least debugfs_create_str() has been added since I wrote the charp
thing.
> +
> +/* add a subdirectory with files for each intel display param */
> +void intel_display_debugfs_params(struct drm_i915_private *i915)
> +{
> + struct drm_minor *minor = i915->drm.primary;
> + struct intel_display_params *params = &i915->display.params;
> + struct dentry *dir;
> + char dirname[16];
> +
> + snprintf(dirname, sizeof(dirname), "%s_params", i915->drm.driver->name);
> + dir = debugfs_lookup(dirname, minor->debugfs_root);
> + if (!dir)
> + dir = debugfs_create_dir(dirname, minor->debugfs_root);
> + if (IS_ERR(dir))
> + return;
> +
> + /*
> + * 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, ...) _intel_display_param_create_file(dir, #x, mode, ¶ms->x);
> + INTEL_DISPLAY_PARAMS_FOR_EACH(REGISTER);
> +#undef REGISTER
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.h b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.h
> new file mode 100644
> index 000000000000..0e33f4e90ddc
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_DISPLAY_DEBUGFS_PARAMS__
> +#define __INTEL_DISPLAY_DEBUGFS_PARAMS__
> +
> +struct dentry;
> +struct drm_i915_private;
> +
> +void intel_display_debugfs_params(struct drm_i915_private *i915);
> +
> +#endif /* __INTEL_DISPLAY_DEBUGFS_PARAMS__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index a6a18eae7ae8..7b7d8e41e5be 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -12,6 +12,7 @@
> #include "intel_de.h"
> #include "intel_display.h"
> #include "intel_display_device.h"
> +#include "intel_display_params.h"
> #include "intel_display_power.h"
> #include "intel_display_reg_defs.h"
> #include "intel_fbc.h"
> @@ -937,6 +938,13 @@ void intel_display_device_probe(struct drm_i915_private *i915)
> DISPLAY_RUNTIME_INFO(i915)->ip.rel = rel;
> DISPLAY_RUNTIME_INFO(i915)->ip.step = step;
> }
> +
> + intel_display_params_copy(&i915->display.params, &intel_display_modparams);
> +}
> +
> +void intel_display_device_remove(struct drm_i915_private *i915)
> +{
> + intel_display_params_free(&i915->display.params);
> }
>
> void intel_display_device_info_runtime_init(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 44733c9d5812..f5a614a553a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -151,6 +151,7 @@ struct intel_display_device_info {
> };
>
> void intel_display_device_probe(struct drm_i915_private *i915);
> +void intel_display_device_remove(struct drm_i915_private *i915);
> void intel_display_device_info_runtime_init(struct drm_i915_private *i915);
>
> void intel_display_device_info_print(const struct intel_display_device_info *info,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> new file mode 100644
> index 000000000000..530cc80d0928
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright © 2023 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
Please convert all of these to SPDX.
> +
> +#include <linux/string_helpers.h>
> +
> +#include <drm/drm_print.h>
> +
> +#include "intel_display_params.h"
> +#include "i915_drv.h"
> +
> +#define intel_display_param_named(name, T, perm, desc) \
> + module_param_named(name, intel_display_modparams.name, T, perm); \
> + MODULE_PARM_DESC(name, desc)
> +#define intel_display_param_named_unsafe(name, T, perm, desc) \
> + module_param_named_unsafe(name, intel_display_modparams.name, T, perm); \
> + MODULE_PARM_DESC(name, desc)
> +
> +struct intel_display_params intel_display_modparams __read_mostly = {
Please make this static.
If any place in code really needs access to the original raw module
params rather than the copied device params, then add accessor
functions.
(This has been on my todo list for i915 a long time, but I think here we
could start with all the ones that aren't needed directly.)
> +#define MEMBER(T, member, value, ...) .member = (value),
> + INTEL_DISPLAY_PARAMS_FOR_EACH(MEMBER)
> +#undef MEMBER
> +};
> +/*
> + * Note: As a rule, keep module parameter sysfs permissions read-only
> + * 0400. Runtime changes are only supported through i915 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_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 intel_display_params_copy(struct intel_display_params *dest,
> + const struct intel_display_params *src)
> +{
> + *dest = *src;
> +#define DUP(T, x, ...) _param_dup(&dest->x);
> + INTEL_DISPLAY_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 intel_display_params_free(struct intel_display_params *params)
> +{
> +#define FREE(T, x, ...) _param_free(¶ms->x);
> + INTEL_DISPLAY_PARAMS_FOR_EACH(FREE);
> +#undef FREE
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h
> new file mode 100644
> index 000000000000..4c241f265c10
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright © 2023 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _INTEL_DISPLAY_PARAMS_H_
> +#define _INTEL_DISPLAY_PARAMS_H_
> +
> +#include <linux/types.h>
> +#include <linux/cache.h> /* for __read_mostly */
> +
> +struct drm_printer;
> +
> +/*
> + * Invoke param, a function-like macro, for each intel display 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 INTEL_DISPLAY_PARAMS_FOR_EACH(param)
> +
> +#define MEMBER(T, member, ...) T member;
> +struct intel_display_params {
> + INTEL_DISPLAY_PARAMS_FOR_EACH(MEMBER);
> +};
> +#undef MEMBER
> +
> +extern struct intel_display_params intel_display_modparams __read_mostly;
> +
> +void intel_display_params_copy(struct intel_display_params *dest,
> + const struct intel_display_params *src);
> +void intel_display_params_free(struct intel_display_params *params);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 78501a83ba10..a86b10093dc4 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -907,6 +907,8 @@ static void i915_driver_release(struct drm_device *dev)
> intel_runtime_pm_driver_release(rpm);
>
> i915_driver_late_release(dev_priv);
> +
> + intel_display_device_remove(dev_priv);
> }
>
> static int i915_driver_open(struct drm_device *dev, struct drm_file *file)
--
Jani Nikula, Intel
