[Intel-xe] [PATCH 2/6] drm/xe: place all modprobe parameters at the same place

Mon Feb 27 12:10:53 UTC 2023

On Fri, 17 Feb 2023, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> On Thu, Feb 16, 2023 at 03:12:13PM +0200, Jani Nikula wrote:
>> On Fri, 03 Feb 2023, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>> > From: Mauro Carvalho Chehab <mchehab at kernel.org>
>> >
>> > It makes easier to identify the module parameters if they're
>> > placed it at the same place.
>> >
>> > Place them together with the module author/description/license.
>> >
>> > While here, fix a checkpatch.pl warning on force_probe description:
>> > 	WARNING: quoted string split across lines
>> >
>> > Signed-off-by: Mauro Carvalho Chehab <mchehab at kernel.org>
>> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> 
>> I don't really see why this needs to reinvent the wheel when we could
>> use the same model as i915. Separate file, all params in a struct that
>> provides a namespace for the params. It's hideous to have them all in
>> the driver local namespace.
>
> I was willing to avoid any special infra in order to discourage the
> addition of many parameters.
>
> But if we start growing we can definitely have the split and all.
>
> Also, I'd like to avoid i915isms. if we need those macros and all like
> we have in i915 we should probably get that into a common kernel
> infra for debugfs because they would likely benefit other drivers as
> well.

Putting all the params in a single file is already an i915-ism.

Doing that without also embedding them in a struct for namespace is
copying the bad parts without the good.

If you don't want to do that, I suggest making all the parameters
*static* in the file they're needed, and if you need access elsewhere,
add accessors.

And, indeed, the best approach would be to reject (almost) all module
paramters.

BR,
Jani.

>
>> 
>> > ---
>> >  drivers/gpu/drm/xe/xe_device.c  | 10 +---------
>> >  drivers/gpu/drm/xe/xe_guc_log.c |  5 +----
>> >  drivers/gpu/drm/xe/xe_mmio.c    |  5 +----
>> >  drivers/gpu/drm/xe/xe_module.c  | 22 ++++++++++++++++++++++
>> >  drivers/gpu/drm/xe/xe_module.h  | 13 +++++++++++++
>> >  drivers/gpu/drm/xe/xe_pci.c     |  7 +------
>> >  6 files changed, 39 insertions(+), 23 deletions(-)
>> >  create mode 100644 drivers/gpu/drm/xe/xe_module.h
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> > index 2e1f4beba9b0..60938c2deee2 100644
>> > --- a/drivers/gpu/drm/xe/xe_device.c
>> > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > @@ -20,6 +20,7 @@
>> >  #include "xe_exec.h"
>> >  #include "xe_gt.h"
>> >  #include "xe_irq.h"
>> > +#include "xe_module.h"
>> >  #include "xe_mmio.h"
>> >  #include "xe_pcode.h"
>> >  #include "xe_pm.h"
>> > @@ -42,15 +43,6 @@
>> >  #include "display/ext/intel_pm.h"
>> >  #endif
>> >  
>> > -/* FIXME: Move to common param infrastructure */
>> > -static bool enable_guc = true;
>> > -module_param_named_unsafe(enable_guc, enable_guc, bool, 0444);
>> > -MODULE_PARM_DESC(enable_guc, "Enable GuC submission");
>> > -
>> > -static bool enable_display = true;
>> > -module_param_named(enable_display, enable_display, bool, 0444);
>> > -MODULE_PARM_DESC(enable_display, "Enable display");
>> > -
>> >  static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>> >  {
>> >  	struct xe_file *xef;
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> > index 3f19fbf243d1..7ec1b2bb1f8e 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> > @@ -9,10 +9,7 @@
>> >  #include "xe_gt.h"
>> >  #include "xe_guc_log.h"
>> >  #include "xe_map.h"
>> > -
>> > -static int xe_guc_log_level = 5;
>> > -module_param_named(guc_log_level, xe_guc_log_level, int, 0600);
>> > -MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=disable, 1..5=enable with verbosity min..max)");
>> > +#include "xe_module.h"
>> >  
>> >  static struct xe_gt *
>> >  log_to_gt(struct xe_guc_log *log)
>> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> > index f20734cf15ba..8a953df2b468 100644
>> > --- a/drivers/gpu/drm/xe/xe_mmio.c
>> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> > @@ -12,6 +12,7 @@
>> >  #include "xe_gt.h"
>> >  #include "xe_gt_mcr.h"
>> >  #include "xe_macros.h"
>> > +#include "xe_module.h"
>> >  
>> >  #include "i915_reg.h"
>> >  #include "gt/intel_engine_regs.h"
>> > @@ -21,10 +22,6 @@
>> >  #define TILE_COUNT		REG_GENMASK(15, 8)
>> >  #define GEN12_LMEM_BAR		2
>> >  
>> > -static u32 xe_force_lmem_bar_size;
>> > -module_param_named(lmem_bar_size, xe_force_lmem_bar_size, uint, 0600);
>> > -MODULE_PARM_DESC(lmem_bar_size, "Set the lmem bar size(in MiB)");
>> > -
>> >  static int xe_set_dma_info(struct xe_device *xe)
>> >  {
>> >  	unsigned int mask_size = xe->info.dma_mask_size;
>> > diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
>> > index d6b50f1c2a05..9cd1663f83f6 100644
>> > --- a/drivers/gpu/drm/xe/xe_module.c
>> > +++ b/drivers/gpu/drm/xe/xe_module.c
>> > @@ -8,9 +8,31 @@
>> >  
>> >  #include "xe_drv.h"
>> >  #include "xe_hw_fence.h"
>> > +#include "xe_module.h"
>> >  #include "xe_pci.h"
>> >  #include "xe_sched_job.h"
>> >  
>> > +bool enable_guc = true;
>> > +module_param_named_unsafe(enable_guc, enable_guc, bool, 0444);
>> > +MODULE_PARM_DESC(enable_guc, "Enable GuC submission");
>> > +
>> > +bool enable_display = true;
>> > +module_param_named(enable_display, enable_display, bool, 0444);
>> > +MODULE_PARM_DESC(enable_display, "Enable display");
>> > +
>> > +u32 xe_force_lmem_bar_size;
>> > +module_param_named(lmem_bar_size, xe_force_lmem_bar_size, uint, 0600);
>> > +MODULE_PARM_DESC(lmem_bar_size, "Set the lmem bar size(in MiB)");
>> > +
>> > +int xe_guc_log_level = 5;
>> > +module_param_named(guc_log_level, xe_guc_log_level, int, 0600);
>> > +MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=disable, 1..5=enable with verbosity min..max)");
>> > +
>> > +char *xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE;
>> > +module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0400);
>> > +MODULE_PARM_DESC(force_probe,
>> > +		 "Force probe options for specified devices. See CONFIG_DRM_XE_FORCE_PROBE for details.");
>> > +
>> >  struct init_funcs {
>> >  	int (*init)(void);
>> >  	void (*exit)(void);
>> > diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_module.h
>> > new file mode 100644
>> > index 000000000000..2c6ee46f5595
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/xe/xe_module.h
>> > @@ -0,0 +1,13 @@
>> > +/* SPDX-License-Identifier: MIT */
>> > +/*
>> > + * Copyright © 2023 Intel Corporation
>> > + */
>> > +
>> > +#include <linux/init.h>
>> > +
>> > +/* Module modprobe variables */
>> > +extern bool enable_guc;
>> > +extern bool enable_display;
>> > +extern u32 xe_force_lmem_bar_size;
>> > +extern int xe_guc_log_level;
>> > +extern char *xe_param_force_probe;
>> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> > index 1d5b6afed2c3..20aa2b5ca9ac 100644
>> > --- a/drivers/gpu/drm/xe/xe_pci.c
>> > +++ b/drivers/gpu/drm/xe/xe_pci.c
>> > @@ -17,17 +17,12 @@
>> >  #include "xe_drv.h"
>> >  #include "xe_device.h"
>> >  #include "xe_macros.h"
>> > +#include "xe_module.h"
>> >  #include "xe_pm.h"
>> >  #include "xe_step.h"
>> >  
>> >  #include "i915_reg.h"
>> >  
>> > -static char *xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE;
>> > -module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0400);
>> > -MODULE_PARM_DESC(force_probe,
>> > -		 "Force probe options for specified devices. "
>> > -		 "See CONFIG_DRM_XE_FORCE_PROBE for details.");
>> > -
>> >  #define DEV_INFO_FOR_EACH_FLAG(func) \
>> >  	func(require_force_probe); \
>> >  	func(is_dgfx); \
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center