[Intel-xe] [PATCH 0/6] Disabling display with modparam.

[Intel-xe] [PATCH 0/6] Disabling display with modparam.

[Rodrigo Vivi]

These patches here are already reviewed and ready for
getting merged. But I will hold on the pending ones
so we don't have to deal with rebase conflicts.

Mauro Carvalho Chehab (6):
  drm/xe: allow disabling display at modprobe time
  drm/xe: place all modprobe parameters at the same place
  drm/xe: move XE_DISPLAY to the Kconfig
  drm/xe: move xe device display logic to a separate file
  drm/xe: move xe IRQ-related display logic to xe_display.c
  drm/xe: move xe PM-related display logic to xe_display.c

 drivers/gpu/drm/xe/Kconfig           |   8 +
 drivers/gpu/drm/xe/Kconfig.debug     |   7 -
 drivers/gpu/drm/xe/Makefile          |   1 +
 drivers/gpu/drm/xe/xe_device.c       | 184 ++-------------
 drivers/gpu/drm/xe/xe_device_types.h |   2 +
 drivers/gpu/drm/xe/xe_display.c      | 323 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_display.h      |  85 +++++++
 drivers/gpu/drm/xe/xe_guc_log.c      |   5 +-
 drivers/gpu/drm/xe/xe_irq.c          |  36 +--
 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          |  13 +-
 drivers/gpu/drm/xe/xe_pm.c           | 109 +--------
 14 files changed, 490 insertions(+), 323 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_display.c
 create mode 100644 drivers/gpu/drm/xe/xe_display.h
 create mode 100644 drivers/gpu/drm/xe/xe_module.h

-- 
2.39.1

[Intel-xe] [PATCH 1/6] drm/xe: allow disabling display at modprobe time

[Rodrigo Vivi]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

Offer an alternative besides Kconfig parameter to disable display
when loading the module.

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>
---
 drivers/gpu/drm/xe/xe_device.c       | 73 ++++++++++++++++++++++------
 drivers/gpu/drm/xe/xe_device_types.h |  2 +
 drivers/gpu/drm/xe/xe_irq.c          | 11 +++--
 drivers/gpu/drm/xe/xe_pci.c          |  6 ---
 drivers/gpu/drm/xe/xe_pm.c           | 14 +++++-
 5 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 62f54b4806dc..2e1f4beba9b0 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -47,6 +47,10 @@ 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;
@@ -138,15 +142,12 @@ static void xe_driver_release(struct drm_device *dev)
 	pci_set_drvdata(to_pci_dev(xe->drm.dev), NULL);
 }
 
-static const struct drm_driver driver = {
+static struct drm_driver driver = {
 	/* Don't use MTRRs here; the Xserver or userspace app should
 	 * deal with them for Intel hardware.
 	 */
 	.driver_features =
 	    DRIVER_GEM |
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	    DRIVER_MODESET | DRIVER_ATOMIC |
-#endif
 	    DRIVER_RENDER | DRIVER_SYNCOBJ |
 	    DRIVER_SYNCOBJ_TIMELINE,
 	.open = xe_file_open,
@@ -159,9 +160,6 @@ static const struct drm_driver driver = {
 
 	.dumb_create = xe_bo_dumb_create,
 	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	.lastclose = intel_fbdev_restore_mode,
-#endif
 	.release = &xe_driver_release,
 
 	.ioctls = xe_ioctls,
@@ -191,6 +189,16 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	struct xe_device *xe;
 	int err;
 
+#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (enable_display) {
+		/* Detect if we need to wait for other drivers early on */
+		if (intel_modeset_probe_defer(pdev))
+			return ERR_PTR(EPROBE_DEFER);
+
+		driver.driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
+		driver.lastclose = intel_fbdev_restore_mode;
+	}
+#endif
 	err = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
 	if (err)
 		return ERR_PTR(err);
@@ -208,6 +216,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	xe->info.devid = pdev->device;
 	xe->info.revid = pdev->revision;
 	xe->info.enable_guc = enable_guc;
+	xe->info.enable_display = enable_display;
 
 	spin_lock_init(&xe->irq.lock);
 
@@ -267,6 +276,9 @@ static void xe_device_fini_display_nommio(struct drm_device *dev, void *dummy)
 {
 	struct xe_device *xe = to_xe_device(dev);
 
+	if (!xe->info.enable_display)
+		return;
+
 	intel_power_domains_cleanup(xe);
 }
 #endif
@@ -276,6 +288,9 @@ static int xe_device_init_display_nommio(struct xe_device *xe)
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 	int err;
 
+	if (!xe->info.enable_display)
+		return 0;
+
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(xe);
 	intel_display_irq_init(xe);
@@ -297,6 +312,9 @@ static void xe_device_fini_display_noirq(struct drm_device *dev, void *dummy)
 {
 	struct xe_device *xe = to_xe_device(dev);
 
+	if (!xe->info.enable_display)
+		return;
+
 	intel_modeset_driver_remove_noirq(xe);
 	intel_power_domains_driver_remove(xe);
 }
@@ -307,6 +325,9 @@ static int xe_device_init_display_noirq(struct xe_device *xe)
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 	int err;
 
+	if (!xe->info.enable_display)
+		return 0;
+
 	/* Early display init.. */
 	intel_opregion_setup(xe);
 
@@ -342,6 +363,9 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
 {
 	struct xe_device *xe = to_xe_device(dev);
 
+	if (!xe->info.enable_display)
+		return;
+
 	intel_modeset_driver_remove_nogem(xe);
 }
 #endif
@@ -349,7 +373,12 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
 static int xe_device_init_display_noaccel(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	int err = intel_modeset_init_nogem(xe);
+	int err;
+
+	if (!xe->info.enable_display)
+		return 0;
+
+	err = intel_modeset_init_nogem(xe);
 	if (err)
 		return err;
 
@@ -362,6 +391,9 @@ static int xe_device_init_display_noaccel(struct xe_device *xe)
 static int xe_device_init_display(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return 0;
+
 	return intel_modeset_init(xe);
 #else
 	return 0;
@@ -371,6 +403,9 @@ static int xe_device_init_display(struct xe_device *xe)
 static void xe_device_unlink_display(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return;
+
 	/* poll work can call into fbdev, hence clean that up afterwards */
 	intel_hpd_poll_fini(xe);
 	intel_fbdev_fini(xe);
@@ -469,9 +504,11 @@ int xe_device_probe(struct xe_device *xe)
 		goto err_irq_shutdown;
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	intel_display_driver_register(xe);
-	intel_register_dsm_handler();
-	intel_power_domains_enable(xe);
+	if (xe->info.enable_display) {
+		intel_display_driver_register(xe);
+		intel_register_dsm_handler();
+		intel_power_domains_enable(xe);
+	}
 #endif
 
 	xe_debugfs_register(xe);
@@ -484,7 +521,8 @@ int xe_device_probe(struct xe_device *xe)
 
 err_fini_display:
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	intel_modeset_driver_remove(xe);
+	if (xe->info.enable_display)
+		intel_modeset_driver_remove(xe);
 #endif
 err_irq_shutdown:
 	xe_irq_shutdown(xe);
@@ -496,14 +534,17 @@ int xe_device_probe(struct xe_device *xe)
 static void xe_device_remove_display(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	intel_unregister_dsm_handler();
-	intel_power_domains_disable(xe);
-	intel_display_driver_unregister(xe);
+	if (xe->info.enable_display) {
+		intel_unregister_dsm_handler();
+		intel_power_domains_disable(xe);
+		intel_display_driver_unregister(xe);
+	}
 #endif
 
 	drm_dev_unplug(&xe->drm);
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	intel_modeset_driver_remove(xe);
+	if (xe->info.enable_display)
+		intel_modeset_driver_remove(xe);
 #endif
 }
 
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index a8d48987b2d8..6c71e1b2dbf4 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -96,6 +96,8 @@ struct xe_device {
 		bool has_4tile;
 		/** @has_range_tlb_invalidation: Has range based TLB invalidations */
 		bool has_range_tlb_invalidation;
+		/** @enable_display: display enabled */
+		bool enable_display;
 
 		struct xe_device_display_info {
 			u8 ver;
diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
index f978bdfda99b..e1cc057f50ba 100644
--- a/drivers/gpu/drm/xe/xe_irq.c
+++ b/drivers/gpu/drm/xe/xe_irq.c
@@ -299,7 +299,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (master_ctl & GEN11_DISPLAY_IRQ)
+	if (xe->info.enable_display && (master_ctl & GEN11_DISPLAY_IRQ))
 		gen11_display_irq_handler(xe);
 #endif
 
@@ -308,7 +308,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	gen11_intr_enable(gt, false);
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (gu_misc_iir & GEN11_GU_MISC_GSE)
+	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
 		intel_opregion_asle_intr(xe);
 #endif
 
@@ -403,7 +403,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
 	dg1_intr_enable(xe, false);
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (gu_misc_iir & GEN11_GU_MISC_GSE)
+	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
 		intel_opregion_asle_intr(xe);
 #endif
 
@@ -488,7 +488,8 @@ void xe_irq_reset(struct xe_device *xe)
 		}
 	}
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	gen11_display_irq_reset(xe);
+	if (xe->info.enable_display)
+		gen11_display_irq_reset(xe);
 #endif
 }
 
@@ -504,7 +505,7 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
 		drm_err(&xe->drm, "No interrupt postinstall hook");
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (gt->info.id == XE_GT0)
+	if (xe->info.enable_display && (gt->info.id == XE_GT0))
 		gen11_display_irq_postinstall(gt_to_xe(gt));
 #endif
 }
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 927a050f3b8f..1d5b6afed2c3 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -476,12 +476,6 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return -ENODEV;
 	}
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	/* Detect if we need to wait for other drivers early on */
-	if (intel_modeset_probe_defer(pdev))
-		return -EPROBE_DEFER;
-#endif
-
 	xe = xe_device_create(pdev, ent);
 	if (IS_ERR(xe))
 		return PTR_ERR(xe);
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index ddfd884796ba..4b3a1d8a8adf 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -52,7 +52,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
 	struct drm_device *dev = &xe->drm;
 	struct intel_encoder *encoder;
 
-	if (!xe->info.display.pipe_mask)
+	if (!xe->info.enable_display || !xe->info.display.pipe_mask)
 		return;
 
 	drm_modeset_lock_all(dev);
@@ -66,6 +66,9 @@ static void intel_suspend_encoders(struct xe_device *xe)
 static void xe_pm_display_suspend(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return;
+
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	intel_power_domains_disable(xe);
@@ -91,6 +94,9 @@ static void xe_pm_display_suspend(struct xe_device *xe)
 static void xe_pm_display_suspend_late(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return;
+
 	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
 
 	intel_display_power_suspend_late(xe);
@@ -100,6 +106,9 @@ static void xe_pm_display_suspend_late(struct xe_device *xe)
 static void xe_pm_display_resume_early(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return;
+
 	intel_display_power_resume_early(xe);
 
 	intel_power_domains_resume(xe);
@@ -109,6 +118,9 @@ static void xe_pm_display_resume_early(struct xe_device *xe)
 static void xe_pm_display_resume(struct xe_device *xe)
 {
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+	if (!xe->info.enable_display)
+		return;
+
 	intel_dmc_ucode_resume(xe);
 
 	if (xe->info.display.pipe_mask)
-- 
2.39.1

Re: [Intel-xe] [PATCH 1/6] drm/xe: allow disabling display at modprobe time

[Jani Nikula]

On Fri, 03 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Mauro Carvalho Chehab <mchehab@kernel.org>
>
> Offer an alternative besides Kconfig parameter to disable display
> when loading the module.

Looks like this display disable stuff ignores the distinction between a)
hardware with display that you want disabled, and b) hardware without
display.

If you have CONFIG_DRM_XE_DISPLAY=n or i915.enabled_display=0 for case
(a), you'll leave the hardware untouched instead of actually disabled,
and you'll keep using power.

See the distinction in i915.

Also, due to the complications, it's likely a good idea to wrap this
stuff:

> +	if (!xe->info.enable_display)
> +		return;
> +

in some macro or function call.

BR,
Jani.



>
> 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>
> ---
>  drivers/gpu/drm/xe/xe_device.c       | 73 ++++++++++++++++++++++------
>  drivers/gpu/drm/xe/xe_device_types.h |  2 +
>  drivers/gpu/drm/xe/xe_irq.c          | 11 +++--
>  drivers/gpu/drm/xe/xe_pci.c          |  6 ---
>  drivers/gpu/drm/xe/xe_pm.c           | 14 +++++-
>  5 files changed, 78 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 62f54b4806dc..2e1f4beba9b0 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -47,6 +47,10 @@ 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;
> @@ -138,15 +142,12 @@ static void xe_driver_release(struct drm_device *dev)
>  	pci_set_drvdata(to_pci_dev(xe->drm.dev), NULL);
>  }
>  
> -static const struct drm_driver driver = {
> +static struct drm_driver driver = {
>  	/* Don't use MTRRs here; the Xserver or userspace app should
>  	 * deal with them for Intel hardware.
>  	 */
>  	.driver_features =
>  	    DRIVER_GEM |
> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	    DRIVER_MODESET | DRIVER_ATOMIC |
> -#endif
>  	    DRIVER_RENDER | DRIVER_SYNCOBJ |
>  	    DRIVER_SYNCOBJ_TIMELINE,
>  	.open = xe_file_open,
> @@ -159,9 +160,6 @@ static const struct drm_driver driver = {
>  
>  	.dumb_create = xe_bo_dumb_create,
>  	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	.lastclose = intel_fbdev_restore_mode,
> -#endif
>  	.release = &xe_driver_release,
>  
>  	.ioctls = xe_ioctls,
> @@ -191,6 +189,16 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	struct xe_device *xe;
>  	int err;
>  
> +#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (enable_display) {
> +		/* Detect if we need to wait for other drivers early on */
> +		if (intel_modeset_probe_defer(pdev))
> +			return ERR_PTR(EPROBE_DEFER);
> +
> +		driver.driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
> +		driver.lastclose = intel_fbdev_restore_mode;
> +	}
> +#endif
>  	err = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>  	if (err)
>  		return ERR_PTR(err);
> @@ -208,6 +216,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	xe->info.devid = pdev->device;
>  	xe->info.revid = pdev->revision;
>  	xe->info.enable_guc = enable_guc;
> +	xe->info.enable_display = enable_display;
>  
>  	spin_lock_init(&xe->irq.lock);
>  
> @@ -267,6 +276,9 @@ static void xe_device_fini_display_nommio(struct drm_device *dev, void *dummy)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
>  
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_power_domains_cleanup(xe);
>  }
>  #endif
> @@ -276,6 +288,9 @@ static int xe_device_init_display_nommio(struct xe_device *xe)
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>  	int err;
>  
> +	if (!xe->info.enable_display)
> +		return 0;
> +
>  	/* This must be called before any calls to HAS_PCH_* */
>  	intel_detect_pch(xe);
>  	intel_display_irq_init(xe);
> @@ -297,6 +312,9 @@ static void xe_device_fini_display_noirq(struct drm_device *dev, void *dummy)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
>  
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_modeset_driver_remove_noirq(xe);
>  	intel_power_domains_driver_remove(xe);
>  }
> @@ -307,6 +325,9 @@ static int xe_device_init_display_noirq(struct xe_device *xe)
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>  	int err;
>  
> +	if (!xe->info.enable_display)
> +		return 0;
> +
>  	/* Early display init.. */
>  	intel_opregion_setup(xe);
>  
> @@ -342,6 +363,9 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
>  
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_modeset_driver_remove_nogem(xe);
>  }
>  #endif
> @@ -349,7 +373,12 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>  static int xe_device_init_display_noaccel(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	int err = intel_modeset_init_nogem(xe);
> +	int err;
> +
> +	if (!xe->info.enable_display)
> +		return 0;
> +
> +	err = intel_modeset_init_nogem(xe);
>  	if (err)
>  		return err;
>  
> @@ -362,6 +391,9 @@ static int xe_device_init_display_noaccel(struct xe_device *xe)
>  static int xe_device_init_display(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return 0;
> +
>  	return intel_modeset_init(xe);
>  #else
>  	return 0;
> @@ -371,6 +403,9 @@ static int xe_device_init_display(struct xe_device *xe)
>  static void xe_device_unlink_display(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	/* poll work can call into fbdev, hence clean that up afterwards */
>  	intel_hpd_poll_fini(xe);
>  	intel_fbdev_fini(xe);
> @@ -469,9 +504,11 @@ int xe_device_probe(struct xe_device *xe)
>  		goto err_irq_shutdown;
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	intel_display_driver_register(xe);
> -	intel_register_dsm_handler();
> -	intel_power_domains_enable(xe);
> +	if (xe->info.enable_display) {
> +		intel_display_driver_register(xe);
> +		intel_register_dsm_handler();
> +		intel_power_domains_enable(xe);
> +	}
>  #endif
>  
>  	xe_debugfs_register(xe);
> @@ -484,7 +521,8 @@ int xe_device_probe(struct xe_device *xe)
>  
>  err_fini_display:
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	intel_modeset_driver_remove(xe);
> +	if (xe->info.enable_display)
> +		intel_modeset_driver_remove(xe);
>  #endif
>  err_irq_shutdown:
>  	xe_irq_shutdown(xe);
> @@ -496,14 +534,17 @@ int xe_device_probe(struct xe_device *xe)
>  static void xe_device_remove_display(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	intel_unregister_dsm_handler();
> -	intel_power_domains_disable(xe);
> -	intel_display_driver_unregister(xe);
> +	if (xe->info.enable_display) {
> +		intel_unregister_dsm_handler();
> +		intel_power_domains_disable(xe);
> +		intel_display_driver_unregister(xe);
> +	}
>  #endif
>  
>  	drm_dev_unplug(&xe->drm);
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	intel_modeset_driver_remove(xe);
> +	if (xe->info.enable_display)
> +		intel_modeset_driver_remove(xe);
>  #endif
>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index a8d48987b2d8..6c71e1b2dbf4 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -96,6 +96,8 @@ struct xe_device {
>  		bool has_4tile;
>  		/** @has_range_tlb_invalidation: Has range based TLB invalidations */
>  		bool has_range_tlb_invalidation;
> +		/** @enable_display: display enabled */
> +		bool enable_display;
>  
>  		struct xe_device_display_info {
>  			u8 ver;
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index f978bdfda99b..e1cc057f50ba 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -299,7 +299,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>  	gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	if (master_ctl & GEN11_DISPLAY_IRQ)
> +	if (xe->info.enable_display && (master_ctl & GEN11_DISPLAY_IRQ))
>  		gen11_display_irq_handler(xe);
>  #endif
>  
> @@ -308,7 +308,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>  	gen11_intr_enable(gt, false);
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>  		intel_opregion_asle_intr(xe);
>  #endif
>  
> @@ -403,7 +403,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>  	dg1_intr_enable(xe, false);
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>  		intel_opregion_asle_intr(xe);
>  #endif
>  
> @@ -488,7 +488,8 @@ void xe_irq_reset(struct xe_device *xe)
>  		}
>  	}
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	gen11_display_irq_reset(xe);
> +	if (xe->info.enable_display)
> +		gen11_display_irq_reset(xe);
>  #endif
>  }
>  
> @@ -504,7 +505,7 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
>  		drm_err(&xe->drm, "No interrupt postinstall hook");
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	if (gt->info.id == XE_GT0)
> +	if (xe->info.enable_display && (gt->info.id == XE_GT0))
>  		gen11_display_irq_postinstall(gt_to_xe(gt));
>  #endif
>  }
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 927a050f3b8f..1d5b6afed2c3 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -476,12 +476,6 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		return -ENODEV;
>  	}
>  
> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> -	/* Detect if we need to wait for other drivers early on */
> -	if (intel_modeset_probe_defer(pdev))
> -		return -EPROBE_DEFER;
> -#endif
> -
>  	xe = xe_device_create(pdev, ent);
>  	if (IS_ERR(xe))
>  		return PTR_ERR(xe);
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index ddfd884796ba..4b3a1d8a8adf 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -52,7 +52,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
>  	struct drm_device *dev = &xe->drm;
>  	struct intel_encoder *encoder;
>  
> -	if (!xe->info.display.pipe_mask)
> +	if (!xe->info.enable_display || !xe->info.display.pipe_mask)
>  		return;
>  
>  	drm_modeset_lock_all(dev);
> @@ -66,6 +66,9 @@ static void intel_suspend_encoders(struct xe_device *xe)
>  static void xe_pm_display_suspend(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	/* We do a lot of poking in a lot of registers, make sure they work
>  	 * properly. */
>  	intel_power_domains_disable(xe);
> @@ -91,6 +94,9 @@ static void xe_pm_display_suspend(struct xe_device *xe)
>  static void xe_pm_display_suspend_late(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
>  
>  	intel_display_power_suspend_late(xe);
> @@ -100,6 +106,9 @@ static void xe_pm_display_suspend_late(struct xe_device *xe)
>  static void xe_pm_display_resume_early(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_display_power_resume_early(xe);
>  
>  	intel_power_domains_resume(xe);
> @@ -109,6 +118,9 @@ static void xe_pm_display_resume_early(struct xe_device *xe)
>  static void xe_pm_display_resume(struct xe_device *xe)
>  {
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> +	if (!xe->info.enable_display)
> +		return;
> +
>  	intel_dmc_ucode_resume(xe);
>  
>  	if (xe->info.display.pipe_mask)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-xe] [PATCH 1/6] drm/xe: allow disabling display at modprobe time

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:19:07PM +0200, Jani Nikula wrote:
>On Fri, 03 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> From: Mauro Carvalho Chehab <mchehab@kernel.org>
>>
>> Offer an alternative besides Kconfig parameter to disable display
>> when loading the module.
>
>Looks like this display disable stuff ignores the distinction between a)
>hardware with display that you want disabled, and b) hardware without
>display.
>
>If you have CONFIG_DRM_XE_DISPLAY=n or i915.enabled_display=0 for case
>(a), you'll leave the hardware untouched instead of actually disabled,
>and you'll keep using power.

CONFIG_DRM_XE_DISPLAY is one thing that we don't have the equivalent in
i915. In this particular case it's actually "xe module doesn't know
anything about display, there is no support for driving it".  So, I
think it's fine to just keep it as is.  Note that I don't have a strong
opinion on whether we should or should not have the config in the long
term.  Short term while the display integration is far from perfect, I
think it's ok.

>See the distinction in i915.

I think in i915 there are more use cases for the module param. Here
it's just a shortcut for "don't touch it because we a) don't trust the
driver to be doing the right thing in this regard or b) something else
is disabling it, i.e firmware or early sku enabling.  For (a),
it may change in future as the display support in xe matures.

To avoid ambiguity this param could be named something else like
"skip_display (default=0)" or "drive_display (default=1)"

In i915 disable_display=1  is an action to be taken by the driver that
can be skipped in case the HW isn't there.

Lucas De Marchi

>
>Also, due to the complications, it's likely a good idea to wrap this
>stuff:
>
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>
>in some macro or function call.
>
>BR,
>Jani.
>
>
>
>>
>> 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>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c       | 73 ++++++++++++++++++++++------
>>  drivers/gpu/drm/xe/xe_device_types.h |  2 +
>>  drivers/gpu/drm/xe/xe_irq.c          | 11 +++--
>>  drivers/gpu/drm/xe/xe_pci.c          |  6 ---
>>  drivers/gpu/drm/xe/xe_pm.c           | 14 +++++-
>>  5 files changed, 78 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 62f54b4806dc..2e1f4beba9b0 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -47,6 +47,10 @@ 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;
>> @@ -138,15 +142,12 @@ static void xe_driver_release(struct drm_device *dev)
>>  	pci_set_drvdata(to_pci_dev(xe->drm.dev), NULL);
>>  }
>>
>> -static const struct drm_driver driver = {
>> +static struct drm_driver driver = {
>>  	/* Don't use MTRRs here; the Xserver or userspace app should
>>  	 * deal with them for Intel hardware.
>>  	 */
>>  	.driver_features =
>>  	    DRIVER_GEM |
>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	    DRIVER_MODESET | DRIVER_ATOMIC |
>> -#endif
>>  	    DRIVER_RENDER | DRIVER_SYNCOBJ |
>>  	    DRIVER_SYNCOBJ_TIMELINE,
>>  	.open = xe_file_open,
>> @@ -159,9 +160,6 @@ static const struct drm_driver driver = {
>>
>>  	.dumb_create = xe_bo_dumb_create,
>>  	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	.lastclose = intel_fbdev_restore_mode,
>> -#endif
>>  	.release = &xe_driver_release,
>>
>>  	.ioctls = xe_ioctls,
>> @@ -191,6 +189,16 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>  	struct xe_device *xe;
>>  	int err;
>>
>> +#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (enable_display) {
>> +		/* Detect if we need to wait for other drivers early on */
>> +		if (intel_modeset_probe_defer(pdev))
>> +			return ERR_PTR(EPROBE_DEFER);
>> +
>> +		driver.driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
>> +		driver.lastclose = intel_fbdev_restore_mode;
>> +	}
>> +#endif
>>  	err = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>  	if (err)
>>  		return ERR_PTR(err);
>> @@ -208,6 +216,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>  	xe->info.devid = pdev->device;
>>  	xe->info.revid = pdev->revision;
>>  	xe->info.enable_guc = enable_guc;
>> +	xe->info.enable_display = enable_display;
>>
>>  	spin_lock_init(&xe->irq.lock);
>>
>> @@ -267,6 +276,9 @@ static void xe_device_fini_display_nommio(struct drm_device *dev, void *dummy)
>>  {
>>  	struct xe_device *xe = to_xe_device(dev);
>>
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_power_domains_cleanup(xe);
>>  }
>>  #endif
>> @@ -276,6 +288,9 @@ static int xe_device_init_display_nommio(struct xe_device *xe)
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>  	int err;
>>
>> +	if (!xe->info.enable_display)
>> +		return 0;
>> +
>>  	/* This must be called before any calls to HAS_PCH_* */
>>  	intel_detect_pch(xe);
>>  	intel_display_irq_init(xe);
>> @@ -297,6 +312,9 @@ static void xe_device_fini_display_noirq(struct drm_device *dev, void *dummy)
>>  {
>>  	struct xe_device *xe = to_xe_device(dev);
>>
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_modeset_driver_remove_noirq(xe);
>>  	intel_power_domains_driver_remove(xe);
>>  }
>> @@ -307,6 +325,9 @@ static int xe_device_init_display_noirq(struct xe_device *xe)
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>  	int err;
>>
>> +	if (!xe->info.enable_display)
>> +		return 0;
>> +
>>  	/* Early display init.. */
>>  	intel_opregion_setup(xe);
>>
>> @@ -342,6 +363,9 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>>  {
>>  	struct xe_device *xe = to_xe_device(dev);
>>
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_modeset_driver_remove_nogem(xe);
>>  }
>>  #endif
>> @@ -349,7 +373,12 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>>  static int xe_device_init_display_noaccel(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	int err = intel_modeset_init_nogem(xe);
>> +	int err;
>> +
>> +	if (!xe->info.enable_display)
>> +		return 0;
>> +
>> +	err = intel_modeset_init_nogem(xe);
>>  	if (err)
>>  		return err;
>>
>> @@ -362,6 +391,9 @@ static int xe_device_init_display_noaccel(struct xe_device *xe)
>>  static int xe_device_init_display(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return 0;
>> +
>>  	return intel_modeset_init(xe);
>>  #else
>>  	return 0;
>> @@ -371,6 +403,9 @@ static int xe_device_init_display(struct xe_device *xe)
>>  static void xe_device_unlink_display(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	/* poll work can call into fbdev, hence clean that up afterwards */
>>  	intel_hpd_poll_fini(xe);
>>  	intel_fbdev_fini(xe);
>> @@ -469,9 +504,11 @@ int xe_device_probe(struct xe_device *xe)
>>  		goto err_irq_shutdown;
>>
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	intel_display_driver_register(xe);
>> -	intel_register_dsm_handler();
>> -	intel_power_domains_enable(xe);
>> +	if (xe->info.enable_display) {
>> +		intel_display_driver_register(xe);
>> +		intel_register_dsm_handler();
>> +		intel_power_domains_enable(xe);
>> +	}
>>  #endif
>>
>>  	xe_debugfs_register(xe);
>> @@ -484,7 +521,8 @@ int xe_device_probe(struct xe_device *xe)
>>
>>  err_fini_display:
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	intel_modeset_driver_remove(xe);
>> +	if (xe->info.enable_display)
>> +		intel_modeset_driver_remove(xe);
>>  #endif
>>  err_irq_shutdown:
>>  	xe_irq_shutdown(xe);
>> @@ -496,14 +534,17 @@ int xe_device_probe(struct xe_device *xe)
>>  static void xe_device_remove_display(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	intel_unregister_dsm_handler();
>> -	intel_power_domains_disable(xe);
>> -	intel_display_driver_unregister(xe);
>> +	if (xe->info.enable_display) {
>> +		intel_unregister_dsm_handler();
>> +		intel_power_domains_disable(xe);
>> +		intel_display_driver_unregister(xe);
>> +	}
>>  #endif
>>
>>  	drm_dev_unplug(&xe->drm);
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	intel_modeset_driver_remove(xe);
>> +	if (xe->info.enable_display)
>> +		intel_modeset_driver_remove(xe);
>>  #endif
>>  }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index a8d48987b2d8..6c71e1b2dbf4 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -96,6 +96,8 @@ struct xe_device {
>>  		bool has_4tile;
>>  		/** @has_range_tlb_invalidation: Has range based TLB invalidations */
>>  		bool has_range_tlb_invalidation;
>> +		/** @enable_display: display enabled */
>> +		bool enable_display;
>>
>>  		struct xe_device_display_info {
>>  			u8 ver;
>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>> index f978bdfda99b..e1cc057f50ba 100644
>> --- a/drivers/gpu/drm/xe/xe_irq.c
>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>> @@ -299,7 +299,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>>  	gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
>>
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	if (master_ctl & GEN11_DISPLAY_IRQ)
>> +	if (xe->info.enable_display && (master_ctl & GEN11_DISPLAY_IRQ))
>>  		gen11_display_irq_handler(xe);
>>  #endif
>>
>> @@ -308,7 +308,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>>  	gen11_intr_enable(gt, false);
>>
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
>> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>>  		intel_opregion_asle_intr(xe);
>>  #endif
>>
>> @@ -403,7 +403,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>>  	dg1_intr_enable(xe, false);
>>
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
>> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>>  		intel_opregion_asle_intr(xe);
>>  #endif
>>
>> @@ -488,7 +488,8 @@ void xe_irq_reset(struct xe_device *xe)
>>  		}
>>  	}
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	gen11_display_irq_reset(xe);
>> +	if (xe->info.enable_display)
>> +		gen11_display_irq_reset(xe);
>>  #endif
>>  }
>>
>> @@ -504,7 +505,7 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
>>  		drm_err(&xe->drm, "No interrupt postinstall hook");
>>
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	if (gt->info.id == XE_GT0)
>> +	if (xe->info.enable_display && (gt->info.id == XE_GT0))
>>  		gen11_display_irq_postinstall(gt_to_xe(gt));
>>  #endif
>>  }
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 927a050f3b8f..1d5b6afed2c3 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -476,12 +476,6 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		return -ENODEV;
>>  	}
>>
>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> -	/* Detect if we need to wait for other drivers early on */
>> -	if (intel_modeset_probe_defer(pdev))
>> -		return -EPROBE_DEFER;
>> -#endif
>> -
>>  	xe = xe_device_create(pdev, ent);
>>  	if (IS_ERR(xe))
>>  		return PTR_ERR(xe);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index ddfd884796ba..4b3a1d8a8adf 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -52,7 +52,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
>>  	struct drm_device *dev = &xe->drm;
>>  	struct intel_encoder *encoder;
>>
>> -	if (!xe->info.display.pipe_mask)
>> +	if (!xe->info.enable_display || !xe->info.display.pipe_mask)
>>  		return;
>>
>>  	drm_modeset_lock_all(dev);
>> @@ -66,6 +66,9 @@ static void intel_suspend_encoders(struct xe_device *xe)
>>  static void xe_pm_display_suspend(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	/* We do a lot of poking in a lot of registers, make sure they work
>>  	 * properly. */
>>  	intel_power_domains_disable(xe);
>> @@ -91,6 +94,9 @@ static void xe_pm_display_suspend(struct xe_device *xe)
>>  static void xe_pm_display_suspend_late(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
>>
>>  	intel_display_power_suspend_late(xe);
>> @@ -100,6 +106,9 @@ static void xe_pm_display_suspend_late(struct xe_device *xe)
>>  static void xe_pm_display_resume_early(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_display_power_resume_early(xe);
>>
>>  	intel_power_domains_resume(xe);
>> @@ -109,6 +118,9 @@ static void xe_pm_display_resume_early(struct xe_device *xe)
>>  static void xe_pm_display_resume(struct xe_device *xe)
>>  {
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>>  	intel_dmc_ucode_resume(xe);
>>
>>  	if (xe->info.display.pipe_mask)
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-xe] [PATCH 1/6] drm/xe: allow disabling display at modprobe time

[Jani Nikula]

On Fri, 17 Feb 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Feb 16, 2023 at 03:19:07PM +0200, Jani Nikula wrote:
>>On Fri, 03 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>>> From: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>
>>> Offer an alternative besides Kconfig parameter to disable display
>>> when loading the module.
>>
>>Looks like this display disable stuff ignores the distinction between a)
>>hardware with display that you want disabled, and b) hardware without
>>display.
>>
>>If you have CONFIG_DRM_XE_DISPLAY=n or i915.enabled_display=0 for case
>>(a), you'll leave the hardware untouched instead of actually disabled,
>>and you'll keep using power.
>
> CONFIG_DRM_XE_DISPLAY is one thing that we don't have the equivalent in
> i915. In this particular case it's actually "xe module doesn't know
> anything about display, there is no support for driving it".  So, I
> think it's fine to just keep it as is.  Note that I don't have a strong
> opinion on whether we should or should not have the config in the long
> term.  Short term while the display integration is far from perfect, I
> think it's ok.

People will scream sooner than you think about display drawing power
when left untouched!

Just try to have a lot of clarity in what things actually mean and what
the implications are. People will want some ambiguous "headless mode"
and they think "no display support or code whatsoever" will satisfy
their use case, but they'll also think it'll DTRT for power...

BR,
Jani.


>
>>See the distinction in i915.
>
> I think in i915 there are more use cases for the module param. Here
> it's just a shortcut for "don't touch it because we a) don't trust the
> driver to be doing the right thing in this regard or b) something else
> is disabling it, i.e firmware or early sku enabling.  For (a),
> it may change in future as the display support in xe matures.
>
> To avoid ambiguity this param could be named something else like
> "skip_display (default=0)" or "drive_display (default=1)"
>
> In i915 disable_display=1  is an action to be taken by the driver that
> can be skipped in case the HW isn't there.
>
> Lucas De Marchi
>
>>
>>Also, due to the complications, it's likely a good idea to wrap this
>>stuff:
>>
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>
>>in some macro or function call.
>>
>>BR,
>>Jani.
>>
>>
>>
>>>
>>> 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>
>>> ---
>>>  drivers/gpu/drm/xe/xe_device.c       | 73 ++++++++++++++++++++++------
>>>  drivers/gpu/drm/xe/xe_device_types.h |  2 +
>>>  drivers/gpu/drm/xe/xe_irq.c          | 11 +++--
>>>  drivers/gpu/drm/xe/xe_pci.c          |  6 ---
>>>  drivers/gpu/drm/xe/xe_pm.c           | 14 +++++-
>>>  5 files changed, 78 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>>> index 62f54b4806dc..2e1f4beba9b0 100644
>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>> @@ -47,6 +47,10 @@ 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;
>>> @@ -138,15 +142,12 @@ static void xe_driver_release(struct drm_device *dev)
>>>  	pci_set_drvdata(to_pci_dev(xe->drm.dev), NULL);
>>>  }
>>>
>>> -static const struct drm_driver driver = {
>>> +static struct drm_driver driver = {
>>>  	/* Don't use MTRRs here; the Xserver or userspace app should
>>>  	 * deal with them for Intel hardware.
>>>  	 */
>>>  	.driver_features =
>>>  	    DRIVER_GEM |
>>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	    DRIVER_MODESET | DRIVER_ATOMIC |
>>> -#endif
>>>  	    DRIVER_RENDER | DRIVER_SYNCOBJ |
>>>  	    DRIVER_SYNCOBJ_TIMELINE,
>>>  	.open = xe_file_open,
>>> @@ -159,9 +160,6 @@ static const struct drm_driver driver = {
>>>
>>>  	.dumb_create = xe_bo_dumb_create,
>>>  	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
>>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	.lastclose = intel_fbdev_restore_mode,
>>> -#endif
>>>  	.release = &xe_driver_release,
>>>
>>>  	.ioctls = xe_ioctls,
>>> @@ -191,6 +189,16 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>>  	struct xe_device *xe;
>>>  	int err;
>>>
>>> +#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (enable_display) {
>>> +		/* Detect if we need to wait for other drivers early on */
>>> +		if (intel_modeset_probe_defer(pdev))
>>> +			return ERR_PTR(EPROBE_DEFER);
>>> +
>>> +		driver.driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
>>> +		driver.lastclose = intel_fbdev_restore_mode;
>>> +	}
>>> +#endif
>>>  	err = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>>  	if (err)
>>>  		return ERR_PTR(err);
>>> @@ -208,6 +216,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>>  	xe->info.devid = pdev->device;
>>>  	xe->info.revid = pdev->revision;
>>>  	xe->info.enable_guc = enable_guc;
>>> +	xe->info.enable_display = enable_display;
>>>
>>>  	spin_lock_init(&xe->irq.lock);
>>>
>>> @@ -267,6 +276,9 @@ static void xe_device_fini_display_nommio(struct drm_device *dev, void *dummy)
>>>  {
>>>  	struct xe_device *xe = to_xe_device(dev);
>>>
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_power_domains_cleanup(xe);
>>>  }
>>>  #endif
>>> @@ -276,6 +288,9 @@ static int xe_device_init_display_nommio(struct xe_device *xe)
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>>  	int err;
>>>
>>> +	if (!xe->info.enable_display)
>>> +		return 0;
>>> +
>>>  	/* This must be called before any calls to HAS_PCH_* */
>>>  	intel_detect_pch(xe);
>>>  	intel_display_irq_init(xe);
>>> @@ -297,6 +312,9 @@ static void xe_device_fini_display_noirq(struct drm_device *dev, void *dummy)
>>>  {
>>>  	struct xe_device *xe = to_xe_device(dev);
>>>
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_modeset_driver_remove_noirq(xe);
>>>  	intel_power_domains_driver_remove(xe);
>>>  }
>>> @@ -307,6 +325,9 @@ static int xe_device_init_display_noirq(struct xe_device *xe)
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>>  	int err;
>>>
>>> +	if (!xe->info.enable_display)
>>> +		return 0;
>>> +
>>>  	/* Early display init.. */
>>>  	intel_opregion_setup(xe);
>>>
>>> @@ -342,6 +363,9 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>>>  {
>>>  	struct xe_device *xe = to_xe_device(dev);
>>>
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_modeset_driver_remove_nogem(xe);
>>>  }
>>>  #endif
>>> @@ -349,7 +373,12 @@ static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
>>>  static int xe_device_init_display_noaccel(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	int err = intel_modeset_init_nogem(xe);
>>> +	int err;
>>> +
>>> +	if (!xe->info.enable_display)
>>> +		return 0;
>>> +
>>> +	err = intel_modeset_init_nogem(xe);
>>>  	if (err)
>>>  		return err;
>>>
>>> @@ -362,6 +391,9 @@ static int xe_device_init_display_noaccel(struct xe_device *xe)
>>>  static int xe_device_init_display(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return 0;
>>> +
>>>  	return intel_modeset_init(xe);
>>>  #else
>>>  	return 0;
>>> @@ -371,6 +403,9 @@ static int xe_device_init_display(struct xe_device *xe)
>>>  static void xe_device_unlink_display(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	/* poll work can call into fbdev, hence clean that up afterwards */
>>>  	intel_hpd_poll_fini(xe);
>>>  	intel_fbdev_fini(xe);
>>> @@ -469,9 +504,11 @@ int xe_device_probe(struct xe_device *xe)
>>>  		goto err_irq_shutdown;
>>>
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	intel_display_driver_register(xe);
>>> -	intel_register_dsm_handler();
>>> -	intel_power_domains_enable(xe);
>>> +	if (xe->info.enable_display) {
>>> +		intel_display_driver_register(xe);
>>> +		intel_register_dsm_handler();
>>> +		intel_power_domains_enable(xe);
>>> +	}
>>>  #endif
>>>
>>>  	xe_debugfs_register(xe);
>>> @@ -484,7 +521,8 @@ int xe_device_probe(struct xe_device *xe)
>>>
>>>  err_fini_display:
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	intel_modeset_driver_remove(xe);
>>> +	if (xe->info.enable_display)
>>> +		intel_modeset_driver_remove(xe);
>>>  #endif
>>>  err_irq_shutdown:
>>>  	xe_irq_shutdown(xe);
>>> @@ -496,14 +534,17 @@ int xe_device_probe(struct xe_device *xe)
>>>  static void xe_device_remove_display(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	intel_unregister_dsm_handler();
>>> -	intel_power_domains_disable(xe);
>>> -	intel_display_driver_unregister(xe);
>>> +	if (xe->info.enable_display) {
>>> +		intel_unregister_dsm_handler();
>>> +		intel_power_domains_disable(xe);
>>> +		intel_display_driver_unregister(xe);
>>> +	}
>>>  #endif
>>>
>>>  	drm_dev_unplug(&xe->drm);
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	intel_modeset_driver_remove(xe);
>>> +	if (xe->info.enable_display)
>>> +		intel_modeset_driver_remove(xe);
>>>  #endif
>>>  }
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>> index a8d48987b2d8..6c71e1b2dbf4 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -96,6 +96,8 @@ struct xe_device {
>>>  		bool has_4tile;
>>>  		/** @has_range_tlb_invalidation: Has range based TLB invalidations */
>>>  		bool has_range_tlb_invalidation;
>>> +		/** @enable_display: display enabled */
>>> +		bool enable_display;
>>>
>>>  		struct xe_device_display_info {
>>>  			u8 ver;
>>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>> index f978bdfda99b..e1cc057f50ba 100644
>>> --- a/drivers/gpu/drm/xe/xe_irq.c
>>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>>> @@ -299,7 +299,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>>>  	gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
>>>
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	if (master_ctl & GEN11_DISPLAY_IRQ)
>>> +	if (xe->info.enable_display && (master_ctl & GEN11_DISPLAY_IRQ))
>>>  		gen11_display_irq_handler(xe);
>>>  #endif
>>>
>>> @@ -308,7 +308,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>>>  	gen11_intr_enable(gt, false);
>>>
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
>>> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>>>  		intel_opregion_asle_intr(xe);
>>>  #endif
>>>
>>> @@ -403,7 +403,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>>>  	dg1_intr_enable(xe, false);
>>>
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	if (gu_misc_iir & GEN11_GU_MISC_GSE)
>>> +	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
>>>  		intel_opregion_asle_intr(xe);
>>>  #endif
>>>
>>> @@ -488,7 +488,8 @@ void xe_irq_reset(struct xe_device *xe)
>>>  		}
>>>  	}
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	gen11_display_irq_reset(xe);
>>> +	if (xe->info.enable_display)
>>> +		gen11_display_irq_reset(xe);
>>>  #endif
>>>  }
>>>
>>> @@ -504,7 +505,7 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
>>>  		drm_err(&xe->drm, "No interrupt postinstall hook");
>>>
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	if (gt->info.id == XE_GT0)
>>> +	if (xe->info.enable_display && (gt->info.id == XE_GT0))
>>>  		gen11_display_irq_postinstall(gt_to_xe(gt));
>>>  #endif
>>>  }
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index 927a050f3b8f..1d5b6afed2c3 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -476,12 +476,6 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>  		return -ENODEV;
>>>  	}
>>>
>>> -#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> -	/* Detect if we need to wait for other drivers early on */
>>> -	if (intel_modeset_probe_defer(pdev))
>>> -		return -EPROBE_DEFER;
>>> -#endif
>>> -
>>>  	xe = xe_device_create(pdev, ent);
>>>  	if (IS_ERR(xe))
>>>  		return PTR_ERR(xe);
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>> index ddfd884796ba..4b3a1d8a8adf 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>> @@ -52,7 +52,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
>>>  	struct drm_device *dev = &xe->drm;
>>>  	struct intel_encoder *encoder;
>>>
>>> -	if (!xe->info.display.pipe_mask)
>>> +	if (!xe->info.enable_display || !xe->info.display.pipe_mask)
>>>  		return;
>>>
>>>  	drm_modeset_lock_all(dev);
>>> @@ -66,6 +66,9 @@ static void intel_suspend_encoders(struct xe_device *xe)
>>>  static void xe_pm_display_suspend(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	/* We do a lot of poking in a lot of registers, make sure they work
>>>  	 * properly. */
>>>  	intel_power_domains_disable(xe);
>>> @@ -91,6 +94,9 @@ static void xe_pm_display_suspend(struct xe_device *xe)
>>>  static void xe_pm_display_suspend_late(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
>>>
>>>  	intel_display_power_suspend_late(xe);
>>> @@ -100,6 +106,9 @@ static void xe_pm_display_suspend_late(struct xe_device *xe)
>>>  static void xe_pm_display_resume_early(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_display_power_resume_early(xe);
>>>
>>>  	intel_power_domains_resume(xe);
>>> @@ -109,6 +118,9 @@ static void xe_pm_display_resume_early(struct xe_device *xe)
>>>  static void xe_pm_display_resume(struct xe_device *xe)
>>>  {
>>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> +	if (!xe->info.enable_display)
>>> +		return;
>>> +
>>>  	intel_dmc_ucode_resume(xe);
>>>
>>>  	if (xe->info.display.pipe_mask)
>>
>>-- 
>>Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Rodrigo Vivi]

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>
---
 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); \
-- 
2.39.1

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

[Jani Nikula]

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.

> ---
>  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

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

[Rodrigo Vivi]

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

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

[Jani Nikula]

On Fri, 17 Feb 2023, Rodrigo Vivi <rodrigo.vivi@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@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.

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

[Intel-xe] [PATCH 3/6] drm/xe: move XE_DISPLAY to the Kconfig

[Rodrigo Vivi]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

XE_DISPLAY symbol is not related to debug, but, instead, to a
Xe feature. Move it to the driver's main Kconfig file.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/Kconfig       | 8 ++++++++
 drivers/gpu/drm/xe/Kconfig.debug | 7 -------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 22e47918bfcf..4684e99549d3 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -40,6 +40,14 @@ config DRM_XE
 
 	  If "M" is selected, the module will be called xe.
 
+
+config DRM_XE_DISPLAY
+	bool "Enable display support"
+	depends on DRM_XE && EXPERT
+	default y
+	help
+	  Disable this option only if you want to compile out display support.
+
 config DRM_XE_FORCE_PROBE
 	string "Force probe xe for selected Intel hardware IDs"
 	depends on DRM_XE
diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
index 6726caa63acc..9c773dd74cbd 100644
--- a/drivers/gpu/drm/xe/Kconfig.debug
+++ b/drivers/gpu/drm/xe/Kconfig.debug
@@ -95,10 +95,3 @@ config DRM_XE_USERPTR_INVAL_INJECT
 
 	 Recomended for driver developers only.
 	 If in doubt, say "N".
-
-config DRM_XE_DISPLAY
-	bool "Enable display support"
-	depends on DRM_XE && EXPERT
-	default y
-	help
-	  Disable this option only if you want to compile out display support.
-- 
2.39.1

[Intel-xe] [PATCH 4/6] drm/xe: move xe device display logic to a separate file

[Rodrigo Vivi]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

It is very ugly to read code with lots of ifdefs. Also, it makes
harder when the display part can be disabled in runtime.

So, move all xe_device display-dependent logic to a new
file: xe_display.c, placing the ifdefs inside the corresponding
header file.

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>
---
 drivers/gpu/drm/xe/Makefile     |   1 +
 drivers/gpu/drm/xe/xe_device.c  | 209 +++-----------------------------
 drivers/gpu/drm/xe/xe_display.c | 190 +++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_display.h |  59 +++++++++
 4 files changed, 265 insertions(+), 194 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_display.c
 create mode 100644 drivers/gpu/drm/xe/xe_display.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index d36d0b12f6ff..ffaa3795d9dc 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -132,6 +132,7 @@ $(obj)/display/intel_%.o: $(srctree)/drivers/gpu/drm/i915/display/intel_%.c FORC
 
 # Display..
 xe-$(CONFIG_DRM_XE_DISPLAY) += \
+	xe_display.o \
 	display/icl_dsi.o \
 	display/intel_atomic.o \
 	display/intel_atomic_plane.o \
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 60938c2deee2..a028efa8eebe 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -14,6 +14,7 @@
 
 #include "xe_bo.h"
 #include "xe_debugfs.h"
+#include "xe_display.h"
 #include "xe_dma_buf.h"
 #include "xe_drv.h"
 #include "xe_engine.h"
@@ -30,19 +31,6 @@
 #include "xe_vm_madvise.h"
 #include "xe_wait_user_fence.h"
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-#include "display/intel_acpi.h"
-#include "display/intel_audio.h"
-#include "display/intel_bw.h"
-#include "display/intel_display.h"
-#include "display/intel_fbdev.h"
-#include "display/intel_hdcp.h"
-#include "display/intel_opregion.h"
-#include "display/ext/i915_irq.h"
-#include "display/ext/intel_dram.h"
-#include "display/ext/intel_pm.h"
-#endif
-
 static int xe_file_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct xe_file *xef;
@@ -181,16 +169,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	struct xe_device *xe;
 	int err;
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (enable_display) {
-		/* Detect if we need to wait for other drivers early on */
-		if (intel_modeset_probe_defer(pdev))
-			return ERR_PTR(EPROBE_DEFER);
+	err = xe_display_enable(pdev, &driver);
+	if (err)
+		return ERR_PTR(err);
 
-		driver.driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
-		driver.lastclose = intel_fbdev_restore_mode;
-	}
-#endif
 	err = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
 	if (err)
 		return ERR_PTR(err);
@@ -263,150 +245,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 	return ERR_PTR(err);
 }
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-static void xe_device_fini_display_nommio(struct drm_device *dev, void *dummy)
-{
-	struct xe_device *xe = to_xe_device(dev);
-
-	if (!xe->info.enable_display)
-		return;
-
-	intel_power_domains_cleanup(xe);
-}
-#endif
-
-static int xe_device_init_display_nommio(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	int err;
-
-	if (!xe->info.enable_display)
-		return 0;
-
-	/* This must be called before any calls to HAS_PCH_* */
-	intel_detect_pch(xe);
-	intel_display_irq_init(xe);
-
-	err = intel_power_domains_init(xe);
-	if (err)
-		return err;
-
-	intel_init_display_hooks(xe);
-
-	return drmm_add_action_or_reset(&xe->drm, xe_device_fini_display_nommio, xe);
-#else
-	return 0;
-#endif
-}
-
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-static void xe_device_fini_display_noirq(struct drm_device *dev, void *dummy)
-{
-	struct xe_device *xe = to_xe_device(dev);
-
-	if (!xe->info.enable_display)
-		return;
-
-	intel_modeset_driver_remove_noirq(xe);
-	intel_power_domains_driver_remove(xe);
-}
-#endif
-
-static int xe_device_init_display_noirq(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	int err;
-
-	if (!xe->info.enable_display)
-		return 0;
-
-	/* Early display init.. */
-	intel_opregion_setup(xe);
-
-	/*
-	 * Fill the dram structure to get the system dram info. This will be
-	 * used for memory latency calculation.
-	 */
-	intel_dram_detect(xe);
-
-	intel_bw_init_hw(xe);
-
-	intel_device_info_runtime_init(xe);
-
-	err = drm_aperture_remove_conflicting_pci_framebuffers(to_pci_dev(xe->drm.dev),
-							       xe->drm.driver);
-	if (err)
-		return err;
-
-	err = intel_modeset_init_noirq(xe);
-	if (err)
-		return err;
-
-	return drmm_add_action_or_reset(&xe->drm, xe_device_fini_display_noirq, NULL);
-#else
-	if (xe->info.display.pipe_mask != 0)
-		drm_warn(&xe->drm, "CONFIG_DRM_XE_DISPLAY is unset, but device is display capable\n");
-	return 0;
-#endif
-}
-
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-static void xe_device_fini_display_noaccel(struct drm_device *dev, void *dummy)
-{
-	struct xe_device *xe = to_xe_device(dev);
-
-	if (!xe->info.enable_display)
-		return;
-
-	intel_modeset_driver_remove_nogem(xe);
-}
-#endif
-
-static int xe_device_init_display_noaccel(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	int err;
-
-	if (!xe->info.enable_display)
-		return 0;
-
-	err = intel_modeset_init_nogem(xe);
-	if (err)
-		return err;
-
-	return drmm_add_action_or_reset(&xe->drm, xe_device_fini_display_noaccel, NULL);
-#else
-	return 0;
-#endif
-}
-
-static int xe_device_init_display(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return 0;
-
-	return intel_modeset_init(xe);
-#else
-	return 0;
-#endif
-}
-
-static void xe_device_unlink_display(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return;
-
-	/* poll work can call into fbdev, hence clean that up afterwards */
-	intel_hpd_poll_fini(xe);
-	intel_fbdev_fini(xe);
-
-	intel_hdcp_component_fini(xe);
-	intel_audio_deinit(xe);
-#endif
-}
-
 static void xe_device_sanitize(struct drm_device *drm, void *arg)
 {
 	struct xe_device *xe = arg;
@@ -424,7 +262,7 @@ int xe_device_probe(struct xe_device *xe)
 	u8 id;
 
 	xe->info.mem_region_mask = 1;
-	err = xe_device_init_display_nommio(xe);
+	err = xe_display_init_nommio(xe);
 	if (err)
 		return err;
 
@@ -444,7 +282,7 @@ int xe_device_probe(struct xe_device *xe)
 			return err;
 	}
 
-	err = xe_device_init_display_noirq(xe);
+	err = xe_display_init_noirq(xe);
 	if (err)
 		return err;
 
@@ -477,7 +315,7 @@ int xe_device_probe(struct xe_device *xe)
 	 * This is the reason the first allocation needs to be done
 	 * inside display.
 	 */
-	err = xe_device_init_display_noaccel(xe);
+	err = xe_display_init_noaccel(xe);
 	if (err)
 		goto err_irq_shutdown;
 
@@ -487,7 +325,7 @@ int xe_device_probe(struct xe_device *xe)
 			goto err_irq_shutdown;
 	}
 
-	err = xe_device_init_display(xe);
+	err = xe_display_init(xe);
 	if (err)
 		goto err_fini_display;
 
@@ -495,13 +333,7 @@ int xe_device_probe(struct xe_device *xe)
 	if (err)
 		goto err_irq_shutdown;
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display) {
-		intel_display_driver_register(xe);
-		intel_register_dsm_handler();
-		intel_power_domains_enable(xe);
-	}
-#endif
+	xe_display_register(xe);
 
 	xe_debugfs_register(xe);
 
@@ -512,32 +344,21 @@ int xe_device_probe(struct xe_device *xe)
 	return 0;
 
 err_fini_display:
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display)
-		intel_modeset_driver_remove(xe);
-#endif
+	xe_display_modset_driver_remove(xe);
+
 err_irq_shutdown:
 	xe_irq_shutdown(xe);
 err:
-	xe_device_unlink_display(xe);
+	xe_display_unlink(xe);
 	return err;
 }
 
 static void xe_device_remove_display(struct xe_device *xe)
 {
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display) {
-		intel_unregister_dsm_handler();
-		intel_power_domains_disable(xe);
-		intel_display_driver_unregister(xe);
-	}
-#endif
+	xe_display_unregister(xe);
 
 	drm_dev_unplug(&xe->drm);
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display)
-		intel_modeset_driver_remove(xe);
-#endif
+	xe_display_modset_driver_remove(xe);
 }
 
 void xe_device_remove(struct xe_device *xe)
@@ -546,7 +367,7 @@ void xe_device_remove(struct xe_device *xe)
 
 	xe_irq_shutdown(xe);
 
-	xe_device_unlink_display(xe);
+	xe_display_unlink(xe);
 }
 
 void xe_device_shutdown(struct xe_device *xe)
diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
new file mode 100644
index 000000000000..8d83b7c34ad6
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_display.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+
+#include "xe_device.h"
+#include "xe_display.h"
+#include "xe_module.h"
+
+#include <drm/drm_aperture.h>
+#include <drm/drm_managed.h>
+#include <drm/xe_drm.h>
+
+#include "display/intel_acpi.h"
+#include "display/intel_audio.h"
+#include "display/intel_bw.h"
+#include "display/intel_display.h"
+#include "display/intel_fbdev.h"
+#include "display/intel_hdcp.h"
+#include "display/intel_opregion.h"
+#include "display/ext/i915_irq.h"
+#include "display/ext/intel_dram.h"
+#include "display/ext/intel_pm.h"
+
+/* Xe device functions */
+
+int xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver)
+{
+	if (!enable_display)
+		return 0;
+
+	/* Detect if we need to wait for other drivers early on */
+	if (intel_modeset_probe_defer(pdev))
+		return EPROBE_DEFER;
+
+	driver->driver_features |= DRIVER_MODESET | DRIVER_ATOMIC;
+	driver->lastclose = intel_fbdev_restore_mode;
+
+	return 0;
+}
+
+void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
+{
+	struct xe_device *xe = to_xe_device(dev);
+
+	if (!xe->info.enable_display)
+		return;
+
+	intel_power_domains_cleanup(xe);
+}
+
+int xe_display_init_nommio(struct xe_device *xe)
+{
+	int err;
+
+	if (!xe->info.enable_display)
+		return 0;
+
+	/* This must be called before any calls to HAS_PCH_* */
+	intel_detect_pch(xe);
+	intel_display_irq_init(xe);
+
+	err = intel_power_domains_init(xe);
+	if (err)
+		return err;
+
+	intel_init_display_hooks(xe);
+
+	return drmm_add_action_or_reset(&xe->drm, xe_display_fini_nommio, xe);
+}
+
+void xe_display_fini_noirq(struct drm_device *dev, void *dummy)
+{
+	struct xe_device *xe = to_xe_device(dev);
+
+	if (!xe->info.enable_display)
+		return;
+
+	intel_modeset_driver_remove_noirq(xe);
+	intel_power_domains_driver_remove(xe);
+}
+
+int xe_display_init_noirq(struct xe_device *xe)
+{
+	int err;
+
+	if (!xe->info.enable_display)
+		return 0;
+
+	/* Early display init.. */
+	intel_opregion_setup(xe);
+
+	/*
+	 * Fill the dram structure to get the system dram info. This will be
+	 * used for memory latency calculation.
+	 */
+	intel_dram_detect(xe);
+
+	intel_bw_init_hw(xe);
+
+	intel_device_info_runtime_init(xe);
+
+	err = drm_aperture_remove_conflicting_pci_framebuffers(to_pci_dev(xe->drm.dev),
+							       xe->drm.driver);
+	if (err)
+		return err;
+
+	err = intel_modeset_init_noirq(xe);
+	if (err)
+		return err;
+
+	return drmm_add_action_or_reset(&xe->drm, xe_display_fini_noirq, NULL);
+}
+
+void xe_display_fini_noaccel(struct drm_device *dev, void *dummy)
+{
+	struct xe_device *xe = to_xe_device(dev);
+
+	if (!xe->info.enable_display)
+		return;
+
+	intel_modeset_driver_remove_nogem(xe);
+}
+
+int xe_display_init_noaccel(struct xe_device *xe)
+{
+	int err;
+
+	if (!xe->info.enable_display)
+		return 0;
+
+	err = intel_modeset_init_nogem(xe);
+	if (err)
+		return err;
+
+	return drmm_add_action_or_reset(&xe->drm, xe_display_fini_noaccel, NULL);
+}
+
+int xe_display_init(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return 0;
+
+	return intel_modeset_init(xe);
+}
+
+void xe_display_unlink(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	/* poll work can call into fbdev, hence clean that up afterwards */
+	intel_hpd_poll_fini(xe);
+	intel_fbdev_fini(xe);
+
+	intel_hdcp_component_fini(xe);
+	intel_audio_deinit(xe);
+}
+
+void xe_display_register(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_display_driver_register(xe);
+	intel_register_dsm_handler();
+	intel_power_domains_enable(xe);
+}
+
+void xe_display_unregister(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_unregister_dsm_handler();
+	intel_power_domains_disable(xe);
+	intel_display_driver_unregister(xe);
+}
+
+void xe_display_modset_driver_remove(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_modeset_driver_remove(xe);
+}
+
+#endif
diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/xe_display.h
new file mode 100644
index 000000000000..79aba827b254
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_display.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _XE_DISPLAY_H_
+#define _XE_DISPLAY_H_
+
+#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+#include <drm/drm_drv.h>
+
+int xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver);
+
+int xe_display_init_nommio(struct xe_device *xe);
+void xe_display_fini_nommio(struct drm_device *dev, void *dummy);
+
+int xe_display_init_noirq(struct xe_device *xe);
+void xe_display_fini_noirq(struct drm_device *dev, void *dummy);
+
+int xe_display_init_noaccel(struct xe_device *xe);
+void xe_display_fini_noaccel(struct drm_device *dev, void *dummy);
+
+int xe_display_init(struct xe_device *xe);
+void xe_display_unlink(struct xe_device *xe);
+
+void xe_display_register(struct xe_device *xe);
+void xe_display_unregister(struct xe_device *xe);
+void xe_display_modset_driver_remove(struct xe_device *xe);
+
+#else
+static inline int
+xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver) { return 0; };
+
+static inline int
+xe_display_init_nommio(struct xe_device *xe) { return 0; };
+static inline void xe_display_fini_nommio(struct drm_device *dev, void *dummy) {};
+
+static inline int xe_display_init_noirq(struct xe_device *xe)
+{
+	if (xe->info.display.pipe_mask != 0)
+		drm_warn(&xe->drm, "CONFIG_DRM_XE_DISPLAY is unset, but device is display capable\n");
+	return 0;
+}
+
+static inline void
+xe_display_fini_noirq(struct drm_device *dev, void *dummy) {};
+
+static inline int xe_display_init_noaccel(struct xe_device *xe) { return 0; };
+static inline void xe_display_fini_noaccel(struct drm_device *dev, void *dummy) {};
+
+static inline int xe_display_init(struct xe_device *xe) { return 0; };
+static inline void xe_display_unlink(struct xe_device *xe) {};
+
+static inline void xe_display_register(struct xe_device *xe) {};
+static inline void xe_display_unregister(struct xe_device *xe) {};
+static inline void xe_display_modset_driver_remove(struct xe_device *xe) {};
+
+#endif /* CONFIG_DRM_XE_DISPLAY */
+#endif /* _XE_DISPLAY_H_ */
-- 
2.39.1

[Intel-xe] [PATCH 5/6] drm/xe: move xe IRQ-related display logic to xe_display.c

[Rodrigo Vivi]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

It is very ugly to read code with lots of ifdefs. Also, it makes
harder when the display part can be disabled in runtime.

Move the IRQ-related core to xe_display.c, making the code
clearer and more modular.

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>
---
 drivers/gpu/drm/xe/xe_display.c | 37 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_display.h | 14 +++++++++++++
 drivers/gpu/drm/xe/xe_irq.c     | 37 +++++++--------------------------
 3 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
index 8d83b7c34ad6..262d8e06b51f 100644
--- a/drivers/gpu/drm/xe/xe_display.c
+++ b/drivers/gpu/drm/xe/xe_display.c
@@ -187,4 +187,41 @@ void xe_display_modset_driver_remove(struct xe_device *xe)
 	intel_modeset_driver_remove(xe);
 }
 
+/* IRQ-related functions */
+
+void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	if (master_ctl & GEN11_DISPLAY_IRQ)
+		gen11_display_irq_handler(xe);
+}
+
+void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	if (gu_misc_iir & GEN11_GU_MISC_GSE)
+		intel_opregion_asle_intr(xe);
+}
+
+void xe_display_irq_reset(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	gen11_display_irq_reset(xe);
+}
+
+void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	if (gt->info.id == XE_GT0)
+		gen11_display_irq_postinstall(xe);
+}
+
 #endif
diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/xe_display.h
index 79aba827b254..11f98ccfaf4e 100644
--- a/drivers/gpu/drm/xe/xe_display.h
+++ b/drivers/gpu/drm/xe/xe_display.h
@@ -9,6 +9,9 @@
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 #include <drm/drm_drv.h>
 
+#include "display/intel_opregion.h"
+#include "display/ext/i915_irq.h"
+
 int xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver);
 
 int xe_display_init_nommio(struct xe_device *xe);
@@ -27,6 +30,12 @@ void xe_display_register(struct xe_device *xe);
 void xe_display_unregister(struct xe_device *xe);
 void xe_display_modset_driver_remove(struct xe_device *xe);
 
+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);
+
+void xe_display_irq_reset(struct xe_device *xe);
+void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt);
+
 #else
 static inline int
 xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver) { return 0; };
@@ -55,5 +64,10 @@ static inline void xe_display_register(struct xe_device *xe) {};
 static inline void xe_display_unregister(struct xe_device *xe) {};
 static inline void xe_display_modset_driver_remove(struct xe_device *xe) {};
 
+static inline void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl) {};
+static inline void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir) {};
+static inline void xe_display_irq_reset(struct xe_device *xe) {};
+static inline void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt) {};
+
 #endif /* CONFIG_DRM_XE_DISPLAY */
 #endif /* _XE_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
index e1cc057f50ba..7b7ddd11c2b8 100644
--- a/drivers/gpu/drm/xe/xe_irq.c
+++ b/drivers/gpu/drm/xe/xe_irq.c
@@ -8,17 +8,13 @@
 #include <drm/drm_managed.h>
 
 #include "xe_device.h"
+#include "xe_display.h"
 #include "xe_drv.h"
 #include "xe_guc.h"
 #include "xe_gt.h"
 #include "xe_hw_engine.h"
 #include "xe_mmio.h"
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-#include "display/intel_opregion.h"
-#include "display/ext/i915_irq.h"
-#endif
-
 #include "i915_reg.h"
 #include "gt/intel_gt_regs.h"
 
@@ -298,19 +294,13 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display && (master_ctl & GEN11_DISPLAY_IRQ))
-		gen11_display_irq_handler(xe);
-#endif
+	xe_display_irq_handler(xe, master_ctl);
 
 	gu_misc_iir = gen11_gu_misc_irq_ack(gt, master_ctl);
 
 	gen11_intr_enable(gt, false);
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
-		intel_opregion_asle_intr(xe);
-#endif
+	xe_display_irq_enable(xe, gu_misc_iir);
 
 	return IRQ_HANDLED;
 }
@@ -393,19 +383,13 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
 		gen11_gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
 	}
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (master_ctl & GEN11_DISPLAY_IRQ)
-		gen11_display_irq_handler(xe);
-#endif
+	xe_display_irq_handler(xe, master_ctl);
 
 	gu_misc_iir = gen11_gu_misc_irq_ack(gt, master_ctl);
 
 	dg1_intr_enable(xe, false);
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display && (gu_misc_iir & GEN11_GU_MISC_GSE))
-		intel_opregion_asle_intr(xe);
-#endif
+	xe_display_irq_enable(xe, gu_misc_iir);
 
 	return IRQ_HANDLED;
 }
@@ -487,10 +471,8 @@ void xe_irq_reset(struct xe_device *xe)
 			drm_err(&xe->drm, "No interrupt reset hook");
 		}
 	}
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display)
-		gen11_display_irq_reset(xe);
-#endif
+
+	xe_display_irq_reset(xe);
 }
 
 void xe_gt_irq_postinstall(struct xe_gt *gt)
@@ -504,10 +486,7 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
 	else
 		drm_err(&xe->drm, "No interrupt postinstall hook");
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (xe->info.enable_display && (gt->info.id == XE_GT0))
-		gen11_display_irq_postinstall(gt_to_xe(gt));
-#endif
+	xe_display_irq_postinstall(xe, gt);
 }
 
 static void xe_irq_postinstall(struct xe_device *xe)
-- 
2.39.1

[Intel-xe] [PATCH 6/6] drm/xe: move xe PM-related display logic to xe_display.c

[Rodrigo Vivi]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

It is very ugly to read code with lots of ifdefs. Also, it makes
harder when the display part can be disabled in runtime.

Move the PM-related core to xe_display.c, making the code
clearer and more modular.

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>
---
 drivers/gpu/drm/xe/xe_display.c |  98 +++++++++++++++++++++++++-
 drivers/gpu/drm/xe/xe_display.h |  12 ++++
 drivers/gpu/drm/xe/xe_pm.c      | 121 ++------------------------------
 3 files changed, 115 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
index 262d8e06b51f..47a16a3f85fa 100644
--- a/drivers/gpu/drm/xe/xe_display.c
+++ b/drivers/gpu/drm/xe/xe_display.c
@@ -5,7 +5,6 @@
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 
-#include "xe_device.h"
 #include "xe_display.h"
 #include "xe_module.h"
 
@@ -17,13 +16,18 @@
 #include "display/intel_audio.h"
 #include "display/intel_bw.h"
 #include "display/intel_display.h"
+#include "display/intel_display_types.h"
+#include "display/intel_dp.h"
 #include "display/intel_fbdev.h"
 #include "display/intel_hdcp.h"
+#include "display/intel_hotplug.h"
 #include "display/intel_opregion.h"
 #include "display/ext/i915_irq.h"
 #include "display/ext/intel_dram.h"
 #include "display/ext/intel_pm.h"
 
+#include <linux/fb.h>
+
 /* Xe device functions */
 
 int xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver)
@@ -224,4 +228,96 @@ void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
 		gen11_display_irq_postinstall(xe);
 }
 
+static void intel_suspend_encoders(struct xe_device *xe)
+{
+	struct drm_device *dev = &xe->drm;
+	struct intel_encoder *encoder;
+
+	if (!xe->info.display.pipe_mask)
+		return;
+
+	drm_modeset_lock_all(dev);
+	for_each_intel_encoder(dev, encoder)
+		if (encoder->suspend)
+			encoder->suspend(encoder);
+	drm_modeset_unlock_all(dev);
+}
+
+void xe_display_pm_suspend(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	/*
+	 * We do a lot of poking in a lot of registers, make sure they work
+	 * properly.
+	 */
+	intel_power_domains_disable(xe);
+	if (xe->info.display.pipe_mask)
+		drm_kms_helper_poll_disable(&xe->drm);
+
+	intel_display_suspend(&xe->drm);
+
+	intel_dp_mst_suspend(xe);
+
+	intel_hpd_cancel_work(xe);
+
+	intel_suspend_encoders(xe);
+
+	intel_opregion_suspend(xe, PCI_D3cold);
+
+	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
+
+	intel_dmc_ucode_suspend(xe);
+}
+
+void xe_display_pm_suspend_late(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
+
+	intel_display_power_suspend_late(xe);
+}
+
+void xe_display_pm_resume_early(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_display_power_resume_early(xe);
+
+	intel_power_domains_resume(xe);
+}
+
+void xe_display_pm_resume(struct xe_device *xe)
+{
+	if (!xe->info.enable_display)
+		return;
+
+	intel_dmc_ucode_resume(xe);
+
+	if (xe->info.display.pipe_mask)
+		drm_mode_config_reset(&xe->drm);
+
+	intel_modeset_init_hw(xe);
+	intel_init_clock_gating(xe);
+	intel_hpd_init(xe);
+
+	/* MST sideband requires HPD interrupts enabled */
+	intel_dp_mst_resume(xe);
+	intel_display_resume(&xe->drm);
+
+	intel_hpd_poll_disable(xe);
+	if (xe->info.display.pipe_mask)
+		drm_kms_helper_poll_enable(&xe->drm);
+
+	intel_opregion_resume(xe);
+
+	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false);
+
+	intel_power_domains_enable(xe);
+}
+
 #endif
diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/xe_display.h
index 11f98ccfaf4e..03117a7067dc 100644
--- a/drivers/gpu/drm/xe/xe_display.h
+++ b/drivers/gpu/drm/xe/xe_display.h
@@ -9,6 +9,8 @@
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
 #include <drm/drm_drv.h>
 
+#include "xe_device.h"
+
 #include "display/intel_opregion.h"
 #include "display/ext/i915_irq.h"
 
@@ -36,6 +38,11 @@ void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir);
 void xe_display_irq_reset(struct xe_device *xe);
 void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt);
 
+void xe_display_pm_suspend(struct xe_device *xe);
+void xe_display_pm_suspend_late(struct xe_device *xe);
+void xe_display_pm_resume_early(struct xe_device *xe);
+void xe_display_pm_resume(struct xe_device *xe);
+
 #else
 static inline int
 xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver) { return 0; };
@@ -69,5 +76,10 @@ static inline void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
 static inline void xe_display_irq_reset(struct xe_device *xe) {};
 static inline void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt) {};
 
+static inline void xe_display_pm_suspend(struct xe_device *xe) {};
+static inline void xe_display_pm_suspend_late(struct xe_device *xe) {};
+static inline void xe_display_pm_resume_early(struct xe_device *xe) {};
+static inline void xe_display_pm_resume(struct xe_device *xe) {};
+
 #endif /* CONFIG_DRM_XE_DISPLAY */
 #endif /* _XE_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 4b3a1d8a8adf..44c38e670587 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -10,22 +10,13 @@
 #include "xe_bo.h"
 #include "xe_bo_evict.h"
 #include "xe_device.h"
+#include "xe_display.h"
 #include "xe_pm.h"
 #include "xe_gt.h"
 #include "xe_ggtt.h"
 #include "xe_irq.h"
 #include "xe_pcode.h"
 
-#include <linux/fb.h>
-
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-#include "display/intel_display_types.h"
-#include "display/intel_dp.h"
-#include "display/intel_fbdev.h"
-#include "display/intel_hotplug.h"
-#include "display/ext/intel_pm.h"
-#endif
-
 /**
  * DOC: Xe Power Management
  *
@@ -46,106 +37,6 @@
  * and no wait boost. Frequency optimizations should come on a next stage.
  */
 
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-static void intel_suspend_encoders(struct xe_device *xe)
-{
-	struct drm_device *dev = &xe->drm;
-	struct intel_encoder *encoder;
-
-	if (!xe->info.enable_display || !xe->info.display.pipe_mask)
-		return;
-
-	drm_modeset_lock_all(dev);
-	for_each_intel_encoder(dev, encoder)
-		if (encoder->suspend)
-			encoder->suspend(encoder);
-	drm_modeset_unlock_all(dev);
-}
-#endif
-
-static void xe_pm_display_suspend(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return;
-
-	/* We do a lot of poking in a lot of registers, make sure they work
-	 * properly. */
-	intel_power_domains_disable(xe);
-	if (xe->info.display.pipe_mask)
-		drm_kms_helper_poll_disable(&xe->drm);
-
-	intel_display_suspend(&xe->drm);
-
-	intel_dp_mst_suspend(xe);
-
-	intel_hpd_cancel_work(xe);
-
-	intel_suspend_encoders(xe);
-
-	intel_opregion_suspend(xe, PCI_D3cold);
-
-	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
-
-	intel_dmc_ucode_suspend(xe);
-#endif
-}
-
-static void xe_pm_display_suspend_late(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return;
-
-	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
-
-	intel_display_power_suspend_late(xe);
-#endif
-}
-
-static void xe_pm_display_resume_early(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return;
-
-	intel_display_power_resume_early(xe);
-
-	intel_power_domains_resume(xe);
-#endif
-}
-
-static void xe_pm_display_resume(struct xe_device *xe)
-{
-#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-	if (!xe->info.enable_display)
-		return;
-
-	intel_dmc_ucode_resume(xe);
-
-	if (xe->info.display.pipe_mask)
-		drm_mode_config_reset(&xe->drm);
-
-	intel_modeset_init_hw(xe);
-	intel_init_clock_gating(xe);
-	intel_hpd_init(xe);
-
-	/* MST sideband requires HPD interrupts enabled */
-	intel_dp_mst_resume(xe);
-	intel_display_resume(&xe->drm);
-
-	intel_hpd_poll_disable(xe);
-	if (xe->info.display.pipe_mask)
-		drm_kms_helper_poll_enable(&xe->drm);
-
-	intel_opregion_resume(xe);
-
-	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false);
-
-	intel_power_domains_enable(xe);
-#endif
-}
-
 /**
  * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle
  * @xe: xe device instance
@@ -166,19 +57,19 @@ int xe_pm_suspend(struct xe_device *xe)
 	if (err)
 		return err;
 
-	xe_pm_display_suspend(xe);
+	xe_display_pm_suspend(xe);
 
 	for_each_gt(gt, xe, id) {
 		err = xe_gt_suspend(gt);
 		if (err) {
-			xe_pm_display_resume(xe);
+			xe_display_pm_resume(xe);
 			return err;
 		}
 	}
 
 	xe_irq_suspend(xe);
 
-	xe_pm_display_suspend_late(xe);
+	xe_display_pm_suspend_late(xe);
 
 	return 0;
 }
@@ -201,7 +92,7 @@ int xe_pm_resume(struct xe_device *xe)
 			return err;
 	}
 
-	xe_pm_display_resume_early(xe);
+	xe_display_pm_resume_early(xe);
 
 	/*
 	 * This only restores pinned memory which is the memory required for the
@@ -213,7 +104,7 @@ int xe_pm_resume(struct xe_device *xe)
 
 	xe_irq_resume(xe);
 
-	xe_pm_display_resume(xe);
+	xe_display_pm_resume(xe);
 
 	for_each_gt(gt, xe, id)
 		xe_gt_resume(gt);
-- 
2.39.1