From: Jani Nikula <jani.nikula@linux.intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: intel-xe@lists.freedesktop.org
Subject: Re: [Intel-xe] [PATCH] drm/xe: Encapsulate all the module parameters
Date: Thu, 12 Oct 2023 17:15:06 +0300 [thread overview]
Message-ID: <87o7h40vx1.fsf@intel.com> (raw)
In-Reply-To: <gvcesx3btnumlf6eufzvdjl7u4zjbkdlvnutfz6gd2a4323zha@pmb63j73bnpk>
On Thu, 12 Oct 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Oct 12, 2023 at 12:16:46PM +0300, Jani Nikula wrote:
>>On Wed, 11 Oct 2023, Bommithi Sakeena <bommithi.sakeena@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@linux.intel.com>
>>> Signed-off-by: Bommithi Sakeena <bommithi.sakeena@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.
Personally I don't care either way, as long as it's *either*, not
something in between!
BR,
Jani.
>
>>
>>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
>
> 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
--
Jani Nikula, Intel
next prev parent reply other threads:[~2023-10-12 14:15 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-11 17:42 [Intel-xe] [PATCH] drm/xe: Encapsulate all the module parameters Bommithi Sakeena
2023-10-12 0:05 ` [Intel-xe] ✓ CI.Patch_applied: success for " Patchwork
2023-10-12 0:05 ` [Intel-xe] ✗ CI.checkpatch: warning " Patchwork
2023-10-12 0:06 ` [Intel-xe] ✓ CI.KUnit: success " Patchwork
2023-10-12 0:14 ` [Intel-xe] ✓ CI.Build: " Patchwork
2023-10-12 0:14 ` [Intel-xe] ✓ CI.Hooks: " Patchwork
2023-10-12 0:15 ` [Intel-xe] ✓ CI.checksparse: " Patchwork
2023-10-12 0:37 ` [Intel-xe] ✓ CI.BAT: " Patchwork
2023-10-12 9:16 ` [Intel-xe] [PATCH] " Jani Nikula
2023-10-12 14:08 ` Lucas De Marchi
2023-10-12 14:15 ` Jani Nikula [this message]
2023-10-13 20:03 ` Rodrigo Vivi
2023-10-13 22:36 ` Lucas De Marchi
-- strict thread matches above, loose matches on Subject: below --
2023-11-02 19:51 Bommithi Sakeena
2023-11-03 3:48 ` Lucas De Marchi
2023-11-03 15:55 ` Rodrigo Vivi
2023-11-09 17:47 Bommithi Sakeena
2023-11-13 14:23 ` Lucas De Marchi
2023-11-13 19:52 ` Rodrigo Vivi
2023-11-17 16:06 Bommithi Sakeena
2023-11-28 15:58 ` 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=87o7h40vx1.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=lucas.demarchi@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.