On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander 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>
> ---
Looks generally good, but I have a couple of comments:
[...]
> 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;
It doesn't seem like you need dentry here...
> +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 2b1ec23ba9c3..e80842d1e7c7 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);
> +}
> +
> +void intel_display_device_remove(struct drm_i915_private *i915)
> +{
> + intel_display_params_free(&i915->display.params);
> }
>
Why can't you just store the parameters as module globals? They are
always associated with the module anyway. Then you don't need to worry
about the lifetime.
[...]
> 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..1b347365988c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _INTEL_DISPLAY_PARAMS_H_
> +#define _INTEL_DISPLAY_PARAMS_H_
> +
> +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)
I don't get this. Here you create a macro that expands to nothing...
> +
> +#define MEMBER(T, member, ...) T member;
> +struct intel_display_params {
> + INTEL_DISPLAY_PARAMS_FOR_EACH(MEMBER);
...so doesn't this become empty in the end?
[...]
--
Cheers,
Luca.
