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

[Luca Coelho]

On Mon, 2023-10-23 at 07:50 +0000, Hogander, Jouni wrote:
> 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 at intel.com>
> > > ---
> > 
> > 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.

Okay, makes sense.


> > [...]
> > > 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)

Thanks for clarifying.  A small comment somewhere here (at least while
it's empty) would be nice. :)

I'll continue reviewing the other patches now.

--
Cheers,
Luca.