From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-xe@lists.freedesktop.org
Subject: Re: [Intel-xe] [PATCH 2/6] drm/xe: place all modprobe parameters at the same place
Date: Fri, 17 Feb 2023 11:28:06 -0500 [thread overview]
Message-ID: <Y++rFnBkN5PEvE1U@intel.com> (raw)
In-Reply-To: <87cz6921qq.fsf@intel.com>
On Thu, Feb 16, 2023 at 03:12:13PM +0200, Jani Nikula wrote:
> On Fri, 03 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > From: Mauro Carvalho Chehab <mchehab@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@kernel.org>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.
>
> > ---
> > 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
next prev parent reply other threads:[~2023-02-17 16:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-03 20:27 [Intel-xe] [PATCH 0/6] Disabling display with modparam Rodrigo Vivi
2023-02-03 20:27 ` [Intel-xe] [PATCH 1/6] drm/xe: allow disabling display at modprobe time Rodrigo Vivi
2023-02-16 13:19 ` Jani Nikula
2023-02-17 20:03 ` Lucas De Marchi
2023-02-27 12:03 ` Jani Nikula
2023-02-03 20:27 ` [Intel-xe] [PATCH 2/6] drm/xe: place all modprobe parameters at the same place Rodrigo Vivi
2023-02-16 13:12 ` Jani Nikula
2023-02-17 16:28 ` Rodrigo Vivi [this message]
2023-02-27 12:10 ` Jani Nikula
2023-02-03 20:27 ` [Intel-xe] [PATCH 3/6] drm/xe: move XE_DISPLAY to the Kconfig Rodrigo Vivi
2023-02-03 20:27 ` [Intel-xe] [PATCH 4/6] drm/xe: move xe device display logic to a separate file Rodrigo Vivi
2023-02-03 20:27 ` [Intel-xe] [PATCH 5/6] drm/xe: move xe IRQ-related display logic to xe_display.c Rodrigo Vivi
2023-02-03 20:27 ` [Intel-xe] [PATCH 6/6] drm/xe: move xe PM-related " Rodrigo Vivi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y++rFnBkN5PEvE1U@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox