[PATCH] drm/xe: Rename enable_display module param
Jani Nikula
jani.nikula at linux.intel.com
Fri Aug 9 08:05:13 UTC 2024
On Thu, 08 Aug 2024, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
> The different approach used by xe regarding the initialization of
> display HW has been proved a great additional for early driver bring up:
> core xe can be tested without having all the bits sorted out on the
> display side.
>
> On the other hand, the approach exposed by i915-display is to *actively*
> disable the display by programming it if needed, i.e. if it was left
> enabled by firmware. It also has its use to make sure the HW is actually
> disabled and not wasting power.
>
> However having both the way it is in xe doesn't expose a good interface
> wrt module params. From modinfo:
>
> disable_display:Disable display (default: false) (bool)
> enable_display:Enable display (bool)
>
> Rename enable_display do probe_display to try to convey the message that
*to
> the HW is being touched and improve the module param description. To
> avoid confusion, the enable_display is renamed everywhere, not only in
> the module param. New description for the parameters:
>
> disable_display:Disable display (default: false) (bool)
> probe_display:Probe display HW, otherwise it's left untouched (default: true) (bool)
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
Acked-by: Jani Nikula <jani.nikula at intel.com>
> ---
>
> Now that xe settled in upstream, maybe it's also time to consider
> removing the CONFIG_DRM_XE_DISPLAY as discussed in
> https://lore.kernel.org/all/oedsuayuo3wtdea6cvmzaoesji25bwbqohgemayj5ocybbzgtn@el2d34rwqfsn/
I think CONFIG_DRM_XE_DISPLAY=n is a legit use case for people without
display hardware wanting to use a smaller footprint driver (typically
servers).
> Also relevant: https://lore.kernel.org/all/87sfer1fjj.fsf@intel.com/
>
> Jani, I think at one point you had a patch to remove enable_display
> (which I was against and still am for the reasons above) or rename it
> (what this patch is doing). However I couldn't find it to check if you
> were trying to rename to a similar thing as I'm doing now.
I think I just misunderstood your use case, in part because of the
naming. So thanks for fixing it. :)
> drivers/gpu/drm/xe/display/xe_display.c | 46 ++++++++++++-------------
> drivers/gpu/drm/xe/xe_device.c | 2 +-
> drivers/gpu/drm/xe/xe_device_types.h | 7 ++--
> drivers/gpu/drm/xe/xe_module.c | 6 ++--
> drivers/gpu/drm/xe/xe_module.h | 2 +-
> drivers/gpu/drm/xe/xe_pci.c | 8 ++---
> 6 files changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index ca4468c82078..e30fb482d542 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -46,7 +46,7 @@ static bool has_display(struct xe_device *xe)
> */
> bool xe_display_driver_probe_defer(struct pci_dev *pdev)
> {
> - if (!xe_modparam.enable_display)
> + if (!xe_modparam.probe_display)
> return 0;
>
> return intel_display_driver_probe_defer(pdev);
> @@ -62,7 +62,7 @@ bool xe_display_driver_probe_defer(struct pci_dev *pdev)
> */
> void xe_display_driver_set_hooks(struct drm_driver *driver)
> {
> - if (!xe_modparam.enable_display)
> + if (!xe_modparam.probe_display)
> return;
>
> driver->driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
> @@ -104,7 +104,7 @@ static void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
> {
> struct xe_device *xe = to_xe_device(dev);
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_power_domains_cleanup(xe);
> @@ -112,7 +112,7 @@ static void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
>
> int xe_display_init_nommio(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return 0;
>
> /* Fake uncore lock */
> @@ -128,7 +128,7 @@ static void xe_display_fini_noirq(void *arg)
> {
> struct xe_device *xe = arg;
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_display_driver_remove_noirq(xe);
> @@ -139,7 +139,7 @@ int xe_display_init_noirq(struct xe_device *xe)
> {
> int err;
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return 0;
>
> intel_display_driver_early_probe(xe);
> @@ -170,7 +170,7 @@ static void xe_display_fini_noaccel(void *arg)
> {
> struct xe_device *xe = arg;
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_display_driver_remove_nogem(xe);
> @@ -180,7 +180,7 @@ int xe_display_init_noaccel(struct xe_device *xe)
> {
> int err;
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return 0;
>
> err = intel_display_driver_probe_nogem(xe);
> @@ -192,7 +192,7 @@ int xe_display_init_noaccel(struct xe_device *xe)
>
> int xe_display_init(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return 0;
>
> return intel_display_driver_probe(xe);
> @@ -200,7 +200,7 @@ int xe_display_init(struct xe_device *xe)
>
> void xe_display_fini(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_hpd_poll_fini(xe);
> @@ -211,7 +211,7 @@ void xe_display_fini(struct xe_device *xe)
>
> void xe_display_register(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_display_driver_register(xe);
> @@ -221,7 +221,7 @@ void xe_display_register(struct xe_device *xe)
>
> void xe_display_unregister(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_unregister_dsm_handler();
> @@ -231,7 +231,7 @@ void xe_display_unregister(struct xe_device *xe)
>
> void xe_display_driver_remove(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_display_driver_remove(xe);
> @@ -241,7 +241,7 @@ void xe_display_driver_remove(struct xe_device *xe)
>
> void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> if (master_ctl & DISPLAY_IRQ)
> @@ -250,7 +250,7 @@ void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl)
>
> void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> if (gu_misc_iir & GU_MISC_GSE)
> @@ -259,7 +259,7 @@ void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
>
> void xe_display_irq_reset(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> gen11_display_irq_reset(xe);
> @@ -267,7 +267,7 @@ void xe_display_irq_reset(struct xe_device *xe)
>
> void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> if (gt->info.id == XE_GT0)
> @@ -286,7 +286,7 @@ static bool suspend_to_idle(void)
> void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> {
> bool s2idle = suspend_to_idle();
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> /*
> @@ -316,7 +316,7 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> void xe_display_pm_suspend_late(struct xe_device *xe)
> {
> bool s2idle = suspend_to_idle();
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_power_domains_suspend(xe, s2idle);
> @@ -326,7 +326,7 @@ void xe_display_pm_suspend_late(struct xe_device *xe)
>
> void xe_display_pm_resume_early(struct xe_device *xe)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_display_power_resume_early(xe);
> @@ -336,7 +336,7 @@ void xe_display_pm_resume_early(struct xe_device *xe)
>
> void xe_display_pm_resume(struct xe_device *xe, bool runtime)
> {
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> return;
>
> intel_dmc_resume(xe);
> @@ -374,7 +374,7 @@ int xe_display_probe(struct xe_device *xe)
> {
> int err;
>
> - if (!xe->info.enable_display)
> + if (!xe->info.probe_display)
> goto no_display;
>
> intel_display_device_probe(xe);
> @@ -387,7 +387,7 @@ int xe_display_probe(struct xe_device *xe)
> return 0;
>
> no_display:
> - xe->info.enable_display = false;
> + xe->info.probe_display = false;
> unset_display_features(xe);
> return 0;
> }
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 1aba6f9eaa19..206328387150 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -543,7 +543,7 @@ static void update_device_info(struct xe_device *xe)
> {
> /* disable features that are not available/applicable to VFs */
> if (IS_SRIOV_VF(xe)) {
> - xe->info.enable_display = 0;
> + xe->info.probe_display = 0;
> xe->info.has_heci_gscfi = 0;
> xe->info.skip_guc_pc = 1;
> xe->info.skip_pcode = 1;
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5b7292a9a66d..82e99a3e084e 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -282,8 +282,11 @@ struct xe_device {
> u8 has_sriov:1;
> /** @info.has_usm: Device has unified shared memory support */
> u8 has_usm:1;
> - /** @info.enable_display: display enabled */
> - u8 enable_display:1;
> + /**
> + * @info.probe_display: probe display HW, otherwise it's left
> + * untouched in whatever state the firmware left it
> + */
> + u8 probe_display:1;
> /** @info.skip_mtcfg: skip Multi-Tile configuration from MTCFG register */
> u8 skip_mtcfg:1;
> /** @info.skip_pcode: skip access to PCODE uC */
> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> index 7bb99e451fcc..923460119cec 100644
> --- a/drivers/gpu/drm/xe/xe_module.c
> +++ b/drivers/gpu/drm/xe/xe_module.c
> @@ -15,7 +15,7 @@
> #include "xe_sched_job.h"
>
> struct xe_modparam xe_modparam = {
> - .enable_display = true,
> + .probe_display = true,
> .guc_log_level = 5,
> .force_probe = CONFIG_DRM_XE_FORCE_PROBE,
> #ifdef CONFIG_PCI_IOV
> @@ -28,8 +28,8 @@ struct xe_modparam xe_modparam = {
> module_param_named_unsafe(force_execlist, xe_modparam.force_execlist, bool, 0444);
> MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
>
> -module_param_named(enable_display, xe_modparam.enable_display, bool, 0444);
> -MODULE_PARM_DESC(enable_display, "Enable display");
> +module_param_named(probe_display, xe_modparam.probe_display, bool, 0444);
> +MODULE_PARM_DESC(probe_display, "Probe display HW, otherwise it's left untouched (default: true)");
>
> module_param_named(vram_bar_size, xe_modparam.force_vram_bar_size, uint, 0600);
> MODULE_PARM_DESC(vram_bar_size, "Set the vram bar size(in MiB)");
> diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_module.h
> index 61a0d28a28c8..161a5e6f717f 100644
> --- a/drivers/gpu/drm/xe/xe_module.h
> +++ b/drivers/gpu/drm/xe/xe_module.h
> @@ -11,7 +11,7 @@
> /* Module modprobe variables */
> struct xe_modparam {
> bool force_execlist;
> - bool enable_display;
> + bool probe_display;
> u32 force_vram_bar_size;
> int guc_log_level;
> char *guc_firmware_path;
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index f818aa69f3ca..b304855624ce 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -621,9 +621,9 @@ static int xe_info_init_early(struct xe_device *xe,
> xe->info.skip_mtcfg = desc->skip_mtcfg;
> xe->info.skip_pcode = desc->skip_pcode;
>
> - xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
> - xe_modparam.enable_display &&
> - desc->has_display;
> + xe->info.probe_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
> + xe_modparam.probe_display &&
> + desc->has_display;
>
> err = xe_tile_init_early(xe_device_get_root_tile(xe), xe, 0);
> if (err)
> @@ -834,7 +834,7 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> xe->info.media_name,
> xe->info.media_verx100 / 100,
> xe->info.media_verx100 % 100,
> - str_yes_no(xe->info.enable_display),
> + str_yes_no(xe->info.probe_display),
> xe->info.dma_mask_size, xe->info.tile_count,
> xe->info.has_heci_gscfi, xe->info.has_heci_cscfi);
--
Jani Nikula, Intel
More information about the Intel-xe
mailing list