On Sun, 2023-10-22 at 20:45 +0300, Luca Coelho wrote:
> 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:
Thank you Luca for your comments. Please check my responses below.
>
> [...]
> > 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...
Yeah, it seems. I will drop it.
>
>
> > +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.
These are device parameters. Values from equivalent module parameters
are copied when probed. Can be later modified via debugfs without
touching other devices parameters.
>
>
> [...]
> > 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...
I wanted to split the patch set in a way that first this framework is
introduced and only after that parameters are added/moved one by one. I
still need to have INTEL_DISPLAY_PARAMS_FOR_EACH defined to avoid build
failure. If you look at patch 03/24 you see when first parameter is
added this gets as:
#define INTEL_DISPLAY_PARAMS_FOR_EACH(param) \
param(int, enable_fbc, -1, 0600)
BR,
Jouni Högander
>
> > +
> > +#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.
