[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 11:14 +0300, Luca Coelho wrote:
> 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. :)

Forgot to say that, with this, you have my:

Reviewed-by: Luca Coelho <luciano.coelho at intel.com>

Other patches in the series still pending.

--
Cheers,
Luca.