[Intel-xe] [PATCH] drm/xe: Encapsulate all the module parameters
Lucas De Marchi
lucas.demarchi at intel.com
Fri Oct 13 22:36:11 UTC 2023
On Fri, Oct 13, 2023 at 04:03:55PM -0400, Rodrigo Vivi wrote:
>On Thu, Oct 12, 2023 at 09:08:41AM -0500, Lucas De Marchi wrote:
>> On Thu, Oct 12, 2023 at 12:16:46PM +0300, Jani Nikula wrote:
>> > On Wed, 11 Oct 2023, Bommithi Sakeena <bommithi.sakeena at intel.com> wrote:
>> > > Encapsulate all the module paramters in one single global struct
>> > > variable inside the new xe_modparam.h.
>> > >
>> > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> > > Signed-off-by: Bommithi Sakeena <bommithi.sakeena at intel.com>
>> > > ---
>> > > drivers/gpu/drm/xe/xe_device.c | 4 ++--
>> > > drivers/gpu/drm/xe/xe_display.c | 6 +++---
>> > > drivers/gpu/drm/xe/xe_guc_log.c | 4 ++--
>> > > drivers/gpu/drm/xe/xe_mmio.c | 4 ++--
>> > > drivers/gpu/drm/xe/xe_modparam.h | 23 +++++++++++++++++++++++
>> > > drivers/gpu/drm/xe/xe_module.c | 32 +++++++++++++++-----------------
>> > > drivers/gpu/drm/xe/xe_pci.c | 8 ++++----
>> > > drivers/gpu/drm/xe/xe_uc_fw.c | 8 ++++----
>> > > 8 files changed, 55 insertions(+), 34 deletions(-)
>> > > create mode 100644 drivers/gpu/drm/xe/xe_modparam.h
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> > > index ef42c33e3055..0cf908089483 100644
>> > > --- a/drivers/gpu/drm/xe/xe_device.c
>> > > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > > @@ -25,7 +25,7 @@
>> > > #include "xe_gt.h"
>> > > #include "xe_irq.h"
>> > > #include "xe_mmio.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > > #include "xe_pat.h"
>> > > #include "xe_pcode.h"
>> > > #include "xe_pm.h"
>> > > @@ -224,7 +224,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>> > >
>> > > xe->info.devid = pdev->device;
>> > > xe->info.revid = pdev->revision;
>> > > - xe->info.force_execlist = force_execlist;
>> > > + xe->info.force_execlist = xe_mod_param.force_execlist;
>> > >
>> > > spin_lock_init(&xe->irq.lock);
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>> > > index 391f08c1caca..0b55037c1da1 100644
>> > > --- a/drivers/gpu/drm/xe/xe_display.c
>> > > +++ b/drivers/gpu/drm/xe/xe_display.c
>> > > @@ -27,7 +27,7 @@
>> > > #include "intel_hdcp.h"
>> > > #include "intel_hotplug.h"
>> > > #include "intel_opregion.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > >
>> > > /* Xe device functions */
>> > >
>> > > @@ -45,7 +45,7 @@ static bool has_display(struct xe_device *xe)
>> > > */
>> > > bool xe_display_driver_probe_defer(struct pci_dev *pdev)
>> > > {
>> > > - if (!enable_display)
>> > > + if (!xe_mod_param.enable_display)
>> > > return 0;
>> > >
>> > > return intel_display_driver_probe_defer(pdev);
>> > > @@ -69,7 +69,7 @@ static void xe_display_last_close(struct drm_device *dev)
>> > > */
>> > > void xe_display_driver_set_hooks(struct drm_driver *driver)
>> > > {
>> > > - if (!enable_display)
>> > > + if (!xe_mod_param.enable_display)
>> > > return;
>> > >
>> > > driver->driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
>> > > diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> > > index 45c60a9c631c..111ca5433c69 100644
>> > > --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> > > +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> > > @@ -10,7 +10,7 @@
>> > > #include "xe_bo.h"
>> > > #include "xe_gt.h"
>> > > #include "xe_map.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > >
>> > > static struct xe_gt *
>> > > log_to_gt(struct xe_guc_log *log)
>> > > @@ -100,7 +100,7 @@ int xe_guc_log_init(struct xe_guc_log *log)
>> > >
>> > > xe_map_memset(xe, &bo->vmap, 0, 0, guc_log_size());
>> > > log->bo = bo;
>> > > - log->level = xe_guc_log_level;
>> > > + log->level = xe_mod_param.xe_guc_log_level;
>> > >
>> > > err = drmm_add_action_or_reset(&xe->drm, guc_log_fini, log);
>> > > if (err)
>> > > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> > > index e4cf9bfec422..717fcdcb5c7d 100644
>> > > --- a/drivers/gpu/drm/xe/xe_mmio.c
>> > > +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> > > @@ -18,7 +18,7 @@
>> > > #include "xe_gt.h"
>> > > #include "xe_gt_mcr.h"
>> > > #include "xe_macros.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > >
>> > > #define XEHP_MTCFG_ADDR XE_REG(0x101800)
>> > > #define TILE_COUNT REG_GENMASK(15, 8)
>> > > @@ -73,7 +73,7 @@ _resize_bar(struct xe_device *xe, int resno, resource_size_t size)
>> > > */
>> > > static void xe_resize_vram_bar(struct xe_device *xe)
>> > > {
>> > > - u64 force_vram_bar_size = xe_force_vram_bar_size;
>> > > + u64 force_vram_bar_size = xe_mod_param.xe_force_vram_bar_size;
>> > > struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> > > struct pci_bus *root = pdev->bus;
>> > > resource_size_t current_size;
>> > > diff --git a/drivers/gpu/drm/xe/xe_modparam.h b/drivers/gpu/drm/xe/xe_modparam.h
>> > > new file mode 100644
>> > > index 000000000000..efa246616d3f
>> > > --- /dev/null
>> > > +++ b/drivers/gpu/drm/xe/xe_modparam.h
>> > > @@ -0,0 +1,23 @@
>> > > +/* SPDX-License-Identifier: MIT */
>> > > +/*
>> > > + * Copyright © 2023 Intel Corporation
>> > > + */
>> > > +
>> > > +#ifndef _XE_MODPARAM_H_
>> > > +#define _XE_MODPARAM_H_
>> > > +
>> > > +#include <linux/types.h>
>> > > +
>> > > +/* Module modprobe variables */
>> > > +struct xe_module_parameters {
>> > > + bool force_execlist;
>> > > + bool enable_display;
>> > > + u32 xe_force_vram_bar_size;
>> > > + int xe_guc_log_level;
>> > > + char *xe_guc_firmware_path;
>> > > + char *xe_huc_firmware_path;
>> > > + char *xe_param_force_probe;
>> >
>> > xe_mod_param.xe_param_force_probe is unnecesary tautology.
>> >
>> > > +};
>> > > +extern struct xe_module_parameters xe_mod_param;
>> >
>> > To me this is half-baked. Either keep all of it in xe_module.[ch] or
>> > move all of it to xe_modparam.[ch], but please don't add a xe_modparam.h
>> > that has an extern for something in xe_module.c.
>>
>> any reason not to just keep using xe_module.h and have a `struct
>> xe_modparam` inside? I don't think we need a separate .h and .c
>> files just for the module parameters... xe_module.[ch] are small enough
>> to absorb that.
>
>agreed
>
>>
>> >
>> > Please also aim for some naming consistency. File xe_modparam.h, struct
>> > xe_module_parameters, variable xe_mod_param. Choose one, and stick with
>> > it.
>>
>> agreed
>
>hmm... how to keep consistency here?
>
>considering it is inside xe_module.h a good name for the struct is
>struct xe_module_parameters
>or
>struct xe_module_params
>for shorten...
>
>but then to follow the consistency with the rest of xe, the variable
>would be something like
>
>struct xe_module_params module_params;
>
>but this is exported symbol, so it should be good (mandatory?) to have
>the "xe_" prefix in it.
>
>It looks like the way to go then is:
>
>struct xe_module_params xe_module_params;
>
>Will any compiler out there complain on this?
No... why whould they? But... the struct and var don't have to be named
following the header. Just like we declare e.g. struct xe_tile, struct
xe_mem_region, etc in xe_device.h. What we shouldn't do is to use
xe_modparam, xe_module_param, xe_module_params, in different places.
so, IMO:
xe_module.h
struct xe_modparam {
bool force_execlist;
bool enable_display;
u32 force_vram_bar_size;
int guc_log_level;
char *guc_firmware_path;
char *huc_firmware_path;
char *force_probe;
};
extern struct xe_modparam xe_modparam;
xe_module.c
struct xe_modparam xe_modparam;
Lucas De Marchi
>
>>
>> Lucas De Marchi
>>
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> > > +
>> > > +#endif
>> > > diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
>> > > index 7194595e7f31..31ed68fcd087 100644
>> > > --- a/drivers/gpu/drm/xe/xe_module.c
>> > > +++ b/drivers/gpu/drm/xe/xe_module.c
>> > > @@ -3,46 +3,44 @@
>> > > * Copyright © 2021 Intel Corporation
>> > > */
>> > >
>> > > -#include "xe_module.h"
>> > > -
>> > > #include <linux/init.h>
>> > > #include <linux/module.h>
>> > >
>> > > #include "xe_drv.h"
>> > > #include "xe_hw_fence.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > > #include "xe_pci.h"
>> > > #include "xe_pmu.h"
>> > > #include "xe_sched_job.h"
>> > >
>> > > -bool force_execlist = false;
>> > > -module_param_named_unsafe(force_execlist, force_execlist, bool, 0444);
>> > > +struct xe_module_parameters xe_mod_param = {
>> > > + .enable_display = true,
>> > > + .xe_guc_log_level = 5,
>> > > + .xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE,
>> > > + /* the rest are 0 by default */
>> > > +};
>> > > +
>> > > +module_param_named_unsafe(force_execlist, xe_mod_param.force_execlist, bool, 0444);
>> > > MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
>> > >
>> > > -bool enable_display = true;
>> > > -module_param_named(enable_display, enable_display, bool, 0444);
>> > > +module_param_named(enable_display, xe_mod_param.enable_display, bool, 0444);
>> > > MODULE_PARM_DESC(enable_display, "Enable display");
>> > >
>> > > -u32 xe_force_vram_bar_size;
>> > > -module_param_named(vram_bar_size, xe_force_vram_bar_size, uint, 0600);
>> > > +module_param_named(vram_bar_size, xe_mod_param.xe_force_vram_bar_size, uint, 0600);
>> > > MODULE_PARM_DESC(vram_bar_size, "Set the vram bar size(in MiB)");
>> > >
>> > > -int xe_guc_log_level = 5;
>> > > -module_param_named(guc_log_level, xe_guc_log_level, int, 0600);
>> > > +module_param_named(guc_log_level, xe_mod_param.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_guc_firmware_path;
>> > > -module_param_named_unsafe(guc_firmware_path, xe_guc_firmware_path, charp, 0400);
>> > > +module_param_named_unsafe(guc_firmware_path, xe_mod_param.xe_guc_firmware_path, charp, 0400);
>> > > MODULE_PARM_DESC(guc_firmware_path,
>> > > "GuC firmware path to use instead of the default one");
>> > >
>> > > -char *xe_huc_firmware_path;
>> > > -module_param_named_unsafe(huc_firmware_path, xe_huc_firmware_path, charp, 0400);
>> > > +module_param_named_unsafe(huc_firmware_path, xe_mod_param.xe_huc_firmware_path, charp, 0400);
>> > > MODULE_PARM_DESC(huc_firmware_path,
>> > > "HuC firmware path to use instead of the default one - empty string disables");
>> > >
>> > > -char *xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE;
>> > > -module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0400);
>> > > +module_param_named_unsafe(force_probe, xe_mod_param.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.");
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> > > index 99ccee07f8cd..29bcf7565741 100644
>> > > --- a/drivers/gpu/drm/xe/xe_pci.c
>> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
>> > > @@ -21,7 +21,7 @@
>> > > #include "xe_drv.h"
>> > > #include "xe_gt.h"
>> > > #include "xe_macros.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > > #include "xe_pci_types.h"
>> > > #include "xe_pm.h"
>> > > #include "xe_step.h"
>> > > @@ -414,12 +414,12 @@ static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
>> > >
>> > > static bool id_forced(u16 device_id)
>> > > {
>> > > - return device_id_in_list(device_id, xe_param_force_probe, false);
>> > > + return device_id_in_list(device_id, xe_mod_param.xe_param_force_probe, false);
>> > > }
>> > >
>> > > static bool id_blocked(u16 device_id)
>> > > {
>> > > - return device_id_in_list(device_id, xe_param_force_probe, true);
>> > > + return device_id_in_list(device_id, xe_mod_param.xe_param_force_probe, true);
>> > > }
>> > >
>> > > static const struct xe_subplatform_desc *
>> > > @@ -587,7 +587,7 @@ static int xe_info_init(struct xe_device *xe,
>> > > xe->info.has_range_tlb_invalidation = graphics_desc->has_range_tlb_invalidation;
>> > >
>> > > xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
>> > > - enable_display &&
>> > > + xe_mod_param.enable_display &&
>> > > desc->has_display;
>> > > /*
>> > > * All platforms have at least one primary GT. Any platform with media
>> > > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > index 138960138872..854e4ba027e1 100644
>> > > --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > @@ -15,7 +15,7 @@
>> > > #include "xe_gt.h"
>> > > #include "xe_map.h"
>> > > #include "xe_mmio.h"
>> > > -#include "xe_module.h"
>> > > +#include "xe_modparam.h"
>> > > #include "xe_uc_fw.h"
>> > >
>> > > /*
>> > > @@ -219,11 +219,11 @@ uc_fw_override(struct xe_uc_fw *uc_fw)
>> > > /* empty string disables, but it's not allowed for GuC */
>> > > switch (uc_fw->type) {
>> > > case XE_UC_FW_TYPE_GUC:
>> > > - if (xe_guc_firmware_path && *xe_guc_firmware_path)
>> > > - path_override = xe_guc_firmware_path;
>> > > + if (xe_mod_param.xe_guc_firmware_path && xe_mod_param.xe_guc_firmware_path)
>> > > + path_override = xe_mod_param.xe_guc_firmware_path;
>> > > break;
>> > > case XE_UC_FW_TYPE_HUC:
>> > > - path_override = xe_huc_firmware_path;
>> > > + path_override = xe_mod_param.xe_huc_firmware_path;
>> > > break;
>> > > default:
>> > > break;
>> >
>> > --
>> > Jani Nikula, Intel
More information about the Intel-xe
mailing list