[Intel-gfx] [PATCH v2 01/24] drm/i915/display: Add framework to add parameters specific to display

[Luca Coelho]

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 at intel.com>
> ---

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.