* [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2024-11-14 20:22 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
@ 2024-11-14 20:22 ` Rodrigo Vivi
0 siblings, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2024-11-14 20:22 UTC (permalink / raw)
To: intel-gfx, intel-xe; +Cc: Rodrigo Vivi, Jonathan Cavitt
Start the xe-i915-display reconciliation by using the same
shutdown sequences.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/xe/display/xe_display.c | 46 +++++++------------------
drivers/gpu/drm/xe/display/xe_display.h | 5 +--
drivers/gpu/drm/xe/xe_device.c | 4 ++-
3 files changed, 19 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 4759c3cf5915..29676652bc54 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -10,7 +10,6 @@
#include <drm/drm_drv.h>
#include <drm/drm_managed.h>
-#include <drm/drm_atomic_helper.h>
#include <drm/drm_probe_helper.h>
#include <uapi/drm/xe_drm.h>
@@ -357,32 +356,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
void xe_display_pm_shutdown(struct xe_device *xe)
{
- struct intel_display *display = &xe->display;
-
if (!xe->info.probe_display)
return;
- intel_power_domains_disable(xe);
- intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
- if (has_display(xe)) {
- drm_kms_helper_poll_disable(&xe->drm);
- intel_display_driver_disable_user_access(xe);
-
- drm_atomic_helper_shutdown(&xe->drm);
- }
-
- intel_dp_mst_suspend(xe);
- intel_hpd_cancel_work(xe);
+ intel_display_driver_shutdown(xe);
+}
- if (has_display(xe))
- intel_display_driver_suspend_access(xe);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_encoder_suspend_all(display);
- intel_encoder_shutdown_all(display);
+ intel_display_driver_shutdown_noirq(xe);
+}
- intel_opregion_suspend(display, PCI_D3cold);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_dmc_suspend(display);
+ intel_display_driver_shutdown_nogem(xe);
}
void xe_display_pm_runtime_suspend(struct xe_device *xe)
@@ -408,19 +401,6 @@ void xe_display_pm_suspend_late(struct xe_device *xe)
intel_display_power_suspend_late(xe, s2idle);
}
-void xe_display_pm_shutdown_late(struct xe_device *xe)
-{
- if (!xe->info.probe_display)
- return;
-
- /*
- * The only requirement is to reboot with display DC states disabled,
- * for now leaving all display power wells in the INIT power domain
- * enabled.
- */
- intel_power_domains_driver_remove(xe);
-}
-
void xe_display_pm_resume_early(struct xe_device *xe)
{
if (!xe->info.probe_display)
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 17afa537aee5..a801db19b64f 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
void xe_display_pm_suspend_late(struct xe_device *xe);
-void xe_display_pm_shutdown_late(struct xe_device *xe);
+void xe_display_pm_shutdown(struct xe_device *xe);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
void xe_display_pm_resume_early(struct xe_device *xe);
void xe_display_pm_resume(struct xe_device *xe);
void xe_display_pm_runtime_suspend(struct xe_device *xe);
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 0e2dd691bdae..f4ae8e11ac53 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -819,10 +819,12 @@ void xe_device_shutdown(struct xe_device *xe)
xe_irq_suspend(xe);
+ xe_display_pm_shutdown_noirq(xe);
+
for_each_gt(gt, xe, id)
xe_gt_shutdown(gt);
- xe_display_pm_shutdown_late(xe);
+ xe_display_pm_shutdown_noaccel(xe);
} else {
/* BOOM! */
__xe_driver_flr(xe);
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver
@ 2025-01-17 22:09 Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 2/3] drm/xe: At shutdown disable commit helpers instead of flushing Rodrigo Vivi
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-17 22:09 UTC (permalink / raw)
To: intel-gfx, intel-xe; +Cc: Rodrigo Vivi, Imre Deak, Jonathan Cavitt
Move display related shutdown sequences from i915_driver to
intel_display_driver.
No functional change. Just taking the right ownership and
start some reconciliation of them between i915 and Xe.
v2: - Add missing _nogem caller (Imre)
- Fix comment style (Jonathan)
v3: rebase
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> #v1
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
.../drm/i915/display/intel_display_driver.c | 40 +++++++++++++++++++
.../drm/i915/display/intel_display_driver.h | 3 ++
drivers/gpu/drm/i915/i915_driver.c | 27 ++-----------
3 files changed, 47 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 73ea0e906014..718b8b92276f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -39,6 +39,7 @@
#include "intel_dp_tunnel.h"
#include "intel_dpll.h"
#include "intel_dpll_mgr.h"
+#include "intel_encoder.h"
#include "intel_fb.h"
#include "intel_fbc.h"
#include "intel_fbdev.h"
@@ -769,3 +770,42 @@ void intel_display_driver_resume(struct intel_display *display)
if (state)
drm_atomic_state_put(state);
}
+
+void intel_display_driver_shutdown(struct intel_display *display)
+{
+ intel_power_domains_disable(display);
+
+ intel_fbdev_set_suspend(display->drm, FBINFO_STATE_SUSPENDED, true);
+ if (HAS_DISPLAY(display)) {
+ drm_kms_helper_poll_disable(display->drm);
+ intel_display_driver_disable_user_access(display);
+
+ drm_atomic_helper_shutdown(display->drm);
+ }
+
+ intel_dp_mst_suspend(display);
+}
+
+void intel_display_driver_shutdown_noirq(struct intel_display *display)
+{
+ intel_hpd_cancel_work(display);
+
+ if (HAS_DISPLAY(display))
+ intel_display_driver_suspend_access(display);
+
+ intel_encoder_suspend_all(display);
+ intel_encoder_shutdown_all(display);
+
+ intel_dmc_suspend(display);
+}
+
+void intel_display_driver_shutdown_nogem(struct intel_display *display)
+{
+ /*
+ * The only requirement is to reboot with display DC states disabled,
+ * for now leaving all display power wells in the INIT power domain
+ * enabled.
+ */
+
+ intel_power_domains_driver_remove(display);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h b/drivers/gpu/drm/i915/display/intel_display_driver.h
index 2966ff91b219..f155a43e2377 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.h
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.h
@@ -26,6 +26,9 @@ void intel_display_driver_remove_nogem(struct intel_display *display);
void intel_display_driver_unregister(struct intel_display *display);
int intel_display_driver_suspend(struct intel_display *display);
void intel_display_driver_resume(struct intel_display *display);
+void intel_display_driver_shutdown(struct intel_display *display);
+void intel_display_driver_shutdown_noirq(struct intel_display *display);
+void intel_display_driver_shutdown_nogem(struct intel_display *display);
/* interface for intel_display_reset.c */
int __intel_display_driver_resume(struct intel_display *display,
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c2ae37d6b94d..cb7453393a21 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -971,43 +971,24 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
disable_rpm_wakeref_asserts(&i915->runtime_pm);
intel_runtime_pm_disable(&i915->runtime_pm);
- intel_power_domains_disable(display);
-
- intel_fbdev_set_suspend(&i915->drm, FBINFO_STATE_SUSPENDED, true);
- if (HAS_DISPLAY(i915)) {
- drm_kms_helper_poll_disable(&i915->drm);
- intel_display_driver_disable_user_access(display);
-
- drm_atomic_helper_shutdown(&i915->drm);
- }
- intel_dp_mst_suspend(display);
+ intel_display_driver_shutdown(display);
intel_irq_suspend(i915);
- intel_hpd_cancel_work(i915);
- if (HAS_DISPLAY(i915))
- intel_display_driver_suspend_access(display);
-
- intel_encoder_suspend_all(&i915->display);
- intel_encoder_shutdown_all(&i915->display);
-
- intel_dmc_suspend(&i915->display);
+ intel_display_driver_shutdown_noirq(display);
i915_gem_suspend(i915);
/*
- * The only requirement is to reboot with display DC states disabled,
- * for now leaving all display power wells in the INIT power domain
- * enabled.
- *
* TODO:
* - unify the pci_driver::shutdown sequence here with the
* pci_driver.driver.pm.poweroff,poweroff_late sequence.
* - unify the driver remove and system/runtime suspend sequences with
* the above unified shutdown/poweroff sequence.
*/
- intel_power_domains_driver_remove(display);
+ intel_display_driver_shutdown_nogem(display);
+
enable_rpm_wakeref_asserts(&i915->runtime_pm);
intel_runtime_pm_driver_last_release(&i915->runtime_pm);
--
2.48.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/xe: At shutdown disable commit helpers instead of flushing
2025-01-17 22:09 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
@ 2025-01-17 22:09 ` Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
2025-01-17 23:23 ` ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915/display: Move shutdown sequences under display driver Patchwork
2 siblings, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-17 22:09 UTC (permalink / raw)
To: intel-gfx, intel-xe; +Cc: Rodrigo Vivi, Maarten Lankhort, Jonathan Cavitt
This aligns with the current i915 display sequence.
Cc: Maarten Lankhort <maarten.lankhorst@linux.intel.com>
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/xe/display/xe_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 96ba9595bf2a..4f60d7bd7742 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -10,6 +10,7 @@
#include <drm/drm_drv.h>
#include <drm/drm_managed.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_probe_helper.h>
#include <uapi/drm/xe_drm.h>
@@ -378,10 +379,10 @@ void xe_display_pm_shutdown(struct xe_device *xe)
if (has_display(xe)) {
drm_kms_helper_poll_disable(&xe->drm);
intel_display_driver_disable_user_access(display);
- intel_display_driver_suspend(display);
+
+ drm_atomic_helper_shutdown(display->drm);
}
- xe_display_flush_cleanup_work(xe);
intel_dp_mst_suspend(display);
intel_hpd_cancel_work(xe);
--
2.48.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2025-01-17 22:09 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 2/3] drm/xe: At shutdown disable commit helpers instead of flushing Rodrigo Vivi
@ 2025-01-17 22:09 ` Rodrigo Vivi
2025-01-20 13:42 ` Maarten Lankhorst
2025-01-17 23:23 ` ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915/display: Move shutdown sequences under display driver Patchwork
2 siblings, 1 reply; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-17 22:09 UTC (permalink / raw)
To: intel-gfx, intel-xe; +Cc: Rodrigo Vivi, Jonathan Cavitt
Start the xe-i915-display reconciliation by using the same
shutdown sequences.
v2: include the stubs for !CONFIG_DRM_XE_DISPLAY (Kunit)
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/xe/display/xe_display.c | 48 +++++++------------------
drivers/gpu/drm/xe/display/xe_display.h | 10 +++---
drivers/gpu/drm/xe/xe_device.c | 4 ++-
3 files changed, 22 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 4f60d7bd7742..e1ce9eb3332d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -10,7 +10,6 @@
#include <drm/drm_drv.h>
#include <drm/drm_managed.h>
-#include <drm/drm_atomic_helper.h>
#include <drm/drm_probe_helper.h>
#include <uapi/drm/xe_drm.h>
@@ -369,32 +368,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
void xe_display_pm_shutdown(struct xe_device *xe)
{
- struct intel_display *display = &xe->display;
-
if (!xe->info.probe_display)
return;
- intel_power_domains_disable(display);
- intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
- if (has_display(xe)) {
- drm_kms_helper_poll_disable(&xe->drm);
- intel_display_driver_disable_user_access(display);
-
- drm_atomic_helper_shutdown(display->drm);
- }
-
- intel_dp_mst_suspend(display);
- intel_hpd_cancel_work(xe);
+ intel_display_driver_shutdown(&xe->display);
+}
- if (has_display(xe))
- intel_display_driver_suspend_access(display);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_encoder_suspend_all(display);
- intel_encoder_shutdown_all(display);
+ intel_display_driver_shutdown_noirq(&xe->display);
+}
- intel_opregion_suspend(display, PCI_D3cold);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_dmc_suspend(display);
+ intel_display_driver_shutdown_nogem(&xe->display);
}
void xe_display_pm_runtime_suspend(struct xe_device *xe)
@@ -439,21 +432,6 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
intel_dmc_wl_flush_release_work(display);
}
-void xe_display_pm_shutdown_late(struct xe_device *xe)
-{
- struct intel_display *display = &xe->display;
-
- if (!xe->info.probe_display)
- return;
-
- /*
- * The only requirement is to reboot with display DC states disabled,
- * for now leaving all display power wells in the INIT power domain
- * enabled.
- */
- intel_power_domains_driver_remove(display);
-}
-
void xe_display_pm_resume_early(struct xe_device *xe)
{
struct intel_display *display = &xe->display;
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 233f81a26c25..a15ec29b862b 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
void xe_display_pm_suspend_late(struct xe_device *xe);
-void xe_display_pm_shutdown_late(struct xe_device *xe);
+void xe_display_pm_shutdown(struct xe_device *xe);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
void xe_display_pm_resume_early(struct xe_device *xe);
void xe_display_pm_resume(struct xe_device *xe);
void xe_display_pm_runtime_suspend(struct xe_device *xe);
@@ -69,9 +70,10 @@ 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_shutdown(struct xe_device *xe) {}
static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
-static inline void xe_display_pm_shutdown_late(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown_noirq(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown_noaccel(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) {}
static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 0966d9697caf..53cac055a2a9 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -934,10 +934,12 @@ void xe_device_shutdown(struct xe_device *xe)
xe_irq_suspend(xe);
+ xe_display_pm_shutdown_noirq(xe);
+
for_each_gt(gt, xe, id)
xe_gt_shutdown(gt);
- xe_display_pm_shutdown_late(xe);
+ xe_display_pm_shutdown_noaccel(xe);
} else {
/* BOOM! */
__xe_driver_flr(xe);
--
2.48.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915/display: Move shutdown sequences under display driver
2025-01-17 22:09 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 2/3] drm/xe: At shutdown disable commit helpers instead of flushing Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
@ 2025-01-17 23:23 ` Patchwork
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2025-01-17 23:23 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/display: Move shutdown sequences under display driver
URL : https://patchwork.freedesktop.org/series/143698/
State : failure
== Summary ==
Error: make failed
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CC [M] drivers/gpu/drm/i915/display/intel_display_driver.o
drivers/gpu/drm/i915/display/intel_display_driver.c: In function ‘intel_display_driver_shutdown_noirq’:
drivers/gpu/drm/i915/display/intel_display_driver.c:791:31: error: passing argument 1 of ‘intel_hpd_cancel_work’ from incompatible pointer type [-Werror=incompatible-pointer-types]
791 | intel_hpd_cancel_work(display);
| ^~~~~~~
| |
| struct intel_display *
In file included from drivers/gpu/drm/i915/display/intel_display_driver.c:49:
drivers/gpu/drm/i915/display/intel_hotplug.h:26:53: note: expected ‘struct drm_i915_private *’ but argument is of type ‘struct intel_display *’
26 | void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
| ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
cc1: all warnings being treated as errors
make[6]: *** [scripts/Makefile.build:194: drivers/gpu/drm/i915/display/intel_display_driver.o] Error 1
make[5]: *** [scripts/Makefile.build:440: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:440: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:440: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:440: drivers] Error 2
make[1]: *** [/home/kbuild/kernel/Makefile:1989: .] Error 2
make: *** [Makefile:251: __sub-make] Error 2
Build failed, no error log produced
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2025-01-17 22:09 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
@ 2025-01-20 13:42 ` Maarten Lankhorst
2025-01-21 19:37 ` Rodrigo Vivi
2025-01-28 23:27 ` Rodrigo Vivi
0 siblings, 2 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2025-01-20 13:42 UTC (permalink / raw)
To: Rodrigo Vivi, intel-gfx, intel-xe; +Cc: Jonathan Cavitt
Hey,
Den 2025-01-17 kl. 23:09, skrev Rodrigo Vivi:
> Start the xe-i915-display reconciliation by using the same
> shutdown sequences.
>
> v2: include the stubs for !CONFIG_DRM_XE_DISPLAY (Kunit)
>
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/xe/display/xe_display.c | 48 +++++++------------------
> drivers/gpu/drm/xe/display/xe_display.h | 10 +++---
> drivers/gpu/drm/xe/xe_device.c | 4 ++-
> 3 files changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 4f60d7bd7742..e1ce9eb3332d 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -10,7 +10,6 @@
>
> #include <drm/drm_drv.h>
> #include <drm/drm_managed.h>
> -#include <drm/drm_atomic_helper.h>
> #include <drm/drm_probe_helper.h>
> #include <uapi/drm/xe_drm.h>
>
> @@ -369,32 +368,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
>
> void xe_display_pm_shutdown(struct xe_device *xe)
> {
> - struct intel_display *display = &xe->display;
> -
> if (!xe->info.probe_display)
> return;
>
> - intel_power_domains_disable(display);
> - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> - if (has_display(xe)) {
> - drm_kms_helper_poll_disable(&xe->drm);
> - intel_display_driver_disable_user_access(display);
> -
> - drm_atomic_helper_shutdown(display->drm);
> - }
> -
> - intel_dp_mst_suspend(display);
> - intel_hpd_cancel_work(xe);
> + intel_display_driver_shutdown(&xe->display);
> +}
>
> - if (has_display(xe))
> - intel_display_driver_suspend_access(display);
> +void xe_display_pm_shutdown_noirq(struct xe_device *xe)
> +{
> + if (!xe->info.probe_display)
> + return;
>
> - intel_encoder_suspend_all(display);
> - intel_encoder_shutdown_all(display);
> + intel_display_driver_shutdown_noirq(&xe->display);
> +}
>
> - intel_opregion_suspend(display, PCI_D3cold);
> +void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
> +{
> + if (!xe->info.probe_display)
> + return;
>
> - intel_dmc_suspend(display);
> + intel_display_driver_shutdown_nogem(&xe->display);
> }
>
> void xe_display_pm_runtime_suspend(struct xe_device *xe)
> @@ -439,21 +432,6 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> intel_dmc_wl_flush_release_work(display);
> }
>
> -void xe_display_pm_shutdown_late(struct xe_device *xe)
> -{
> - struct intel_display *display = &xe->display;
> -
> - if (!xe->info.probe_display)
> - return;
> -
> - /*
> - * The only requirement is to reboot with display DC states disabled,
> - * for now leaving all display power wells in the INIT power domain
> - * enabled.
> - */
> - intel_power_domains_driver_remove(display);
> -}
> -
> void xe_display_pm_resume_early(struct xe_device *xe)
> {
> struct intel_display *display = &xe->display;
> diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> index 233f81a26c25..a15ec29b862b 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.h
> +++ b/drivers/gpu/drm/xe/display/xe_display.h
> @@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
> void xe_display_pm_suspend_late(struct xe_device *xe);
> -void xe_display_pm_shutdown_late(struct xe_device *xe);
> +void xe_display_pm_shutdown(struct xe_device *xe);
> +void xe_display_pm_shutdown_noirq(struct xe_device *xe);
> +void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
> void xe_display_pm_resume_early(struct xe_device *xe);
> void xe_display_pm_resume(struct xe_device *xe);
> void xe_display_pm_runtime_suspend(struct xe_device *xe);
> @@ -69,9 +70,10 @@ 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_shutdown(struct xe_device *xe) {}
> static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
> -static inline void xe_display_pm_shutdown_late(struct xe_device *xe) {}
> +static inline void xe_display_pm_shutdown(struct xe_device *xe) {}
> +static inline void xe_display_pm_shutdown_noirq(struct xe_device *xe) {}
> +static inline void xe_display_pm_shutdown_noaccel(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) {}
> static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 0966d9697caf..53cac055a2a9 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -934,10 +934,12 @@ void xe_device_shutdown(struct xe_device *xe)
>
> xe_irq_suspend(xe);
>
> + xe_display_pm_shutdown_noirq(xe);
> +
> for_each_gt(gt, xe, id)
> xe_gt_shutdown(gt);
>
> - xe_display_pm_shutdown_late(xe);
> + xe_display_pm_shutdown_noaccel(xe);
From the xe point of view, it shouldn't matter whether we call the
noirq part before or after gt shutdown. I like the integration into
xe_device to be as simple as possible, so could we keep the single
xe_display_pm_shutdown_late() call?
Which reminds me to send out xe_display simplification once more to do
the same for init..
> } else {
> /* BOOM! */
> __xe_driver_flr(xe);
Completely unrelated, do you happen to know if we need to call
encoder_suspend/shutdown in the FLR path, whether it affects logic not
on the chip itself?
Cheers,
~Maarten
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2025-01-20 13:42 ` Maarten Lankhorst
@ 2025-01-21 19:37 ` Rodrigo Vivi
2025-01-28 23:27 ` Rodrigo Vivi
1 sibling, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-21 19:37 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, intel-xe, Jonathan Cavitt
On Mon, Jan 20, 2025 at 02:42:14PM +0100, Maarten Lankhorst wrote:
> Hey,
>
> Den 2025-01-17 kl. 23:09, skrev Rodrigo Vivi:
> > Start the xe-i915-display reconciliation by using the same
> > shutdown sequences.
> >
> > v2: include the stubs for !CONFIG_DRM_XE_DISPLAY (Kunit)
> >
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/xe/display/xe_display.c | 48 +++++++------------------
> > drivers/gpu/drm/xe/display/xe_display.h | 10 +++---
> > drivers/gpu/drm/xe/xe_device.c | 4 ++-
> > 3 files changed, 22 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > index 4f60d7bd7742..e1ce9eb3332d 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -10,7 +10,6 @@
> > #include <drm/drm_drv.h>
> > #include <drm/drm_managed.h>
> > -#include <drm/drm_atomic_helper.h>
> > #include <drm/drm_probe_helper.h>
> > #include <uapi/drm/xe_drm.h>
> > @@ -369,32 +368,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
> > void xe_display_pm_shutdown(struct xe_device *xe)
> > {
> > - struct intel_display *display = &xe->display;
> > -
> > if (!xe->info.probe_display)
> > return;
> > - intel_power_domains_disable(display);
> > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> > - if (has_display(xe)) {
> > - drm_kms_helper_poll_disable(&xe->drm);
> > - intel_display_driver_disable_user_access(display);
> > -
> > - drm_atomic_helper_shutdown(display->drm);
> > - }
> > -
> > - intel_dp_mst_suspend(display);
> > - intel_hpd_cancel_work(xe);
> > + intel_display_driver_shutdown(&xe->display);
> > +}
> > - if (has_display(xe))
> > - intel_display_driver_suspend_access(display);
> > +void xe_display_pm_shutdown_noirq(struct xe_device *xe)
> > +{
> > + if (!xe->info.probe_display)
> > + return;
> > - intel_encoder_suspend_all(display);
> > - intel_encoder_shutdown_all(display);
> > + intel_display_driver_shutdown_noirq(&xe->display);
> > +}
> > - intel_opregion_suspend(display, PCI_D3cold);
> > +void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
> > +{
> > + if (!xe->info.probe_display)
> > + return;
> > - intel_dmc_suspend(display);
> > + intel_display_driver_shutdown_nogem(&xe->display);
> > }
> > void xe_display_pm_runtime_suspend(struct xe_device *xe)
> > @@ -439,21 +432,6 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> > intel_dmc_wl_flush_release_work(display);
> > }
> > -void xe_display_pm_shutdown_late(struct xe_device *xe)
> > -{
> > - struct intel_display *display = &xe->display;
> > -
> > - if (!xe->info.probe_display)
> > - return;
> > -
> > - /*
> > - * The only requirement is to reboot with display DC states disabled,
> > - * for now leaving all display power wells in the INIT power domain
> > - * enabled.
> > - */
> > - intel_power_domains_driver_remove(display);
> > -}
> > -
> > void xe_display_pm_resume_early(struct xe_device *xe)
> > {
> > struct intel_display *display = &xe->display;
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> > index 233f81a26c25..a15ec29b862b 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.h
> > +++ b/drivers/gpu/drm/xe/display/xe_display.h
> > @@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
> > void xe_display_pm_suspend_late(struct xe_device *xe);
> > -void xe_display_pm_shutdown_late(struct xe_device *xe);
> > +void xe_display_pm_shutdown(struct xe_device *xe);
> > +void xe_display_pm_shutdown_noirq(struct xe_device *xe);
> > +void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
> > void xe_display_pm_resume_early(struct xe_device *xe);
> > void xe_display_pm_resume(struct xe_device *xe);
> > void xe_display_pm_runtime_suspend(struct xe_device *xe);
> > @@ -69,9 +70,10 @@ 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_shutdown(struct xe_device *xe) {}
> > static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
> > -static inline void xe_display_pm_shutdown_late(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown_noirq(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown_noaccel(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) {}
> > static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 0966d9697caf..53cac055a2a9 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -934,10 +934,12 @@ void xe_device_shutdown(struct xe_device *xe)
> > xe_irq_suspend(xe);
> > + xe_display_pm_shutdown_noirq(xe);
> > +
> > for_each_gt(gt, xe, id)
> > xe_gt_shutdown(gt);
> > - xe_display_pm_shutdown_late(xe);
> > + xe_display_pm_shutdown_noaccel(xe);
> From the xe point of view, it shouldn't matter whether we call the noirq
> part before or after gt shutdown. I like the integration into xe_device to
> be as simple as possible, so could we keep the single
> xe_display_pm_shutdown_late() call?
I prefer that we make the xe_display entirely an wrapper to i915/display,
It should only check for the Xe's display module parameter, and then call
the equivalent function there directly.
Then, whatever differences we might have we move to xe_device itself.
And in this case _noaccel is just a generic name for _nogem which is a name
that would works for both i915 and xe...
>
> Which reminds me to send out xe_display simplification once more to do the
> same for init..
On that too, I know I reviewed, but Jani also had concerns with that on the
sense that that deviates from removing display differences between drivers.
Although I still believe it is possible to take that patch in, but later
move the differences out of xe_display ?!
> > } else {
> > /* BOOM! */
> > __xe_driver_flr(xe);
> Completely unrelated, do you happen to know if we need to call
> encoder_suspend/shutdown in the FLR path, whether it affects logic not on
> the chip itself?
>
> Cheers,
> ~Maarten
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2025-01-22 10:40 [PATCH 1/3] " Rodrigo Vivi
@ 2025-01-22 10:40 ` Rodrigo Vivi
0 siblings, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-22 10:40 UTC (permalink / raw)
To: intel-gfx, intel-xe; +Cc: Rodrigo Vivi, Jonathan Cavitt
Start the xe-i915-display reconciliation by using the same
shutdown sequences.
v2: include the stubs for !CONFIG_DRM_XE_DISPLAY (Kunit)
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/xe/display/xe_display.c | 48 +++++++------------------
drivers/gpu/drm/xe/display/xe_display.h | 10 +++---
drivers/gpu/drm/xe/xe_device.c | 4 ++-
3 files changed, 22 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 4f60d7bd7742..e1ce9eb3332d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -10,7 +10,6 @@
#include <drm/drm_drv.h>
#include <drm/drm_managed.h>
-#include <drm/drm_atomic_helper.h>
#include <drm/drm_probe_helper.h>
#include <uapi/drm/xe_drm.h>
@@ -369,32 +368,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
void xe_display_pm_shutdown(struct xe_device *xe)
{
- struct intel_display *display = &xe->display;
-
if (!xe->info.probe_display)
return;
- intel_power_domains_disable(display);
- intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
- if (has_display(xe)) {
- drm_kms_helper_poll_disable(&xe->drm);
- intel_display_driver_disable_user_access(display);
-
- drm_atomic_helper_shutdown(display->drm);
- }
-
- intel_dp_mst_suspend(display);
- intel_hpd_cancel_work(xe);
+ intel_display_driver_shutdown(&xe->display);
+}
- if (has_display(xe))
- intel_display_driver_suspend_access(display);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_encoder_suspend_all(display);
- intel_encoder_shutdown_all(display);
+ intel_display_driver_shutdown_noirq(&xe->display);
+}
- intel_opregion_suspend(display, PCI_D3cold);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
+{
+ if (!xe->info.probe_display)
+ return;
- intel_dmc_suspend(display);
+ intel_display_driver_shutdown_nogem(&xe->display);
}
void xe_display_pm_runtime_suspend(struct xe_device *xe)
@@ -439,21 +432,6 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
intel_dmc_wl_flush_release_work(display);
}
-void xe_display_pm_shutdown_late(struct xe_device *xe)
-{
- struct intel_display *display = &xe->display;
-
- if (!xe->info.probe_display)
- return;
-
- /*
- * The only requirement is to reboot with display DC states disabled,
- * for now leaving all display power wells in the INIT power domain
- * enabled.
- */
- intel_power_domains_driver_remove(display);
-}
-
void xe_display_pm_resume_early(struct xe_device *xe)
{
struct intel_display *display = &xe->display;
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 233f81a26c25..a15ec29b862b 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
void xe_display_pm_suspend_late(struct xe_device *xe);
-void xe_display_pm_shutdown_late(struct xe_device *xe);
+void xe_display_pm_shutdown(struct xe_device *xe);
+void xe_display_pm_shutdown_noirq(struct xe_device *xe);
+void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
void xe_display_pm_resume_early(struct xe_device *xe);
void xe_display_pm_resume(struct xe_device *xe);
void xe_display_pm_runtime_suspend(struct xe_device *xe);
@@ -69,9 +70,10 @@ 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_shutdown(struct xe_device *xe) {}
static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
-static inline void xe_display_pm_shutdown_late(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown_noirq(struct xe_device *xe) {}
+static inline void xe_display_pm_shutdown_noaccel(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) {}
static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index bd6191e1ed3e..b4b8c6582a20 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -940,10 +940,12 @@ void xe_device_shutdown(struct xe_device *xe)
xe_irq_suspend(xe);
+ xe_display_pm_shutdown_noirq(xe);
+
for_each_gt(gt, xe, id)
xe_gt_shutdown(gt);
- xe_display_pm_shutdown_late(xe);
+ xe_display_pm_shutdown_noaccel(xe);
} else {
/* BOOM! */
__xe_driver_flr(xe);
--
2.48.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly
2025-01-20 13:42 ` Maarten Lankhorst
2025-01-21 19:37 ` Rodrigo Vivi
@ 2025-01-28 23:27 ` Rodrigo Vivi
1 sibling, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2025-01-28 23:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, intel-xe, Jonathan Cavitt
On Mon, Jan 20, 2025 at 02:42:14PM +0100, Maarten Lankhorst wrote:
> Hey,
>
> Den 2025-01-17 kl. 23:09, skrev Rodrigo Vivi:
> > Start the xe-i915-display reconciliation by using the same
> > shutdown sequences.
> >
> > v2: include the stubs for !CONFIG_DRM_XE_DISPLAY (Kunit)
> >
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/xe/display/xe_display.c | 48 +++++++------------------
> > drivers/gpu/drm/xe/display/xe_display.h | 10 +++---
> > drivers/gpu/drm/xe/xe_device.c | 4 ++-
> > 3 files changed, 22 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > index 4f60d7bd7742..e1ce9eb3332d 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -10,7 +10,6 @@
> > #include <drm/drm_drv.h>
> > #include <drm/drm_managed.h>
> > -#include <drm/drm_atomic_helper.h>
> > #include <drm/drm_probe_helper.h>
> > #include <uapi/drm/xe_drm.h>
> > @@ -369,32 +368,26 @@ void xe_display_pm_suspend(struct xe_device *xe)
> > void xe_display_pm_shutdown(struct xe_device *xe)
> > {
> > - struct intel_display *display = &xe->display;
> > -
> > if (!xe->info.probe_display)
> > return;
> > - intel_power_domains_disable(display);
> > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> > - if (has_display(xe)) {
> > - drm_kms_helper_poll_disable(&xe->drm);
> > - intel_display_driver_disable_user_access(display);
> > -
> > - drm_atomic_helper_shutdown(display->drm);
> > - }
> > -
> > - intel_dp_mst_suspend(display);
> > - intel_hpd_cancel_work(xe);
> > + intel_display_driver_shutdown(&xe->display);
> > +}
> > - if (has_display(xe))
> > - intel_display_driver_suspend_access(display);
> > +void xe_display_pm_shutdown_noirq(struct xe_device *xe)
> > +{
> > + if (!xe->info.probe_display)
> > + return;
> > - intel_encoder_suspend_all(display);
> > - intel_encoder_shutdown_all(display);
> > + intel_display_driver_shutdown_noirq(&xe->display);
> > +}
> > - intel_opregion_suspend(display, PCI_D3cold);
> > +void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
> > +{
> > + if (!xe->info.probe_display)
> > + return;
> > - intel_dmc_suspend(display);
> > + intel_display_driver_shutdown_nogem(&xe->display);
> > }
> > void xe_display_pm_runtime_suspend(struct xe_device *xe)
> > @@ -439,21 +432,6 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> > intel_dmc_wl_flush_release_work(display);
> > }
> > -void xe_display_pm_shutdown_late(struct xe_device *xe)
> > -{
> > - struct intel_display *display = &xe->display;
> > -
> > - if (!xe->info.probe_display)
> > - return;
> > -
> > - /*
> > - * The only requirement is to reboot with display DC states disabled,
> > - * for now leaving all display power wells in the INIT power domain
> > - * enabled.
> > - */
> > - intel_power_domains_driver_remove(display);
> > -}
> > -
> > void xe_display_pm_resume_early(struct xe_device *xe)
> > {
> > struct intel_display *display = &xe->display;
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> > index 233f81a26c25..a15ec29b862b 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.h
> > +++ b/drivers/gpu/drm/xe/display/xe_display.h
> > @@ -35,9 +35,10 @@ 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_shutdown(struct xe_device *xe);
> > void xe_display_pm_suspend_late(struct xe_device *xe);
> > -void xe_display_pm_shutdown_late(struct xe_device *xe);
> > +void xe_display_pm_shutdown(struct xe_device *xe);
> > +void xe_display_pm_shutdown_noirq(struct xe_device *xe);
> > +void xe_display_pm_shutdown_noaccel(struct xe_device *xe);
> > void xe_display_pm_resume_early(struct xe_device *xe);
> > void xe_display_pm_resume(struct xe_device *xe);
> > void xe_display_pm_runtime_suspend(struct xe_device *xe);
> > @@ -69,9 +70,10 @@ 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_shutdown(struct xe_device *xe) {}
> > static inline void xe_display_pm_suspend_late(struct xe_device *xe) {}
> > -static inline void xe_display_pm_shutdown_late(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown_noirq(struct xe_device *xe) {}
> > +static inline void xe_display_pm_shutdown_noaccel(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) {}
> > static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 0966d9697caf..53cac055a2a9 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -934,10 +934,12 @@ void xe_device_shutdown(struct xe_device *xe)
> > xe_irq_suspend(xe);
> > + xe_display_pm_shutdown_noirq(xe);
> > +
> > for_each_gt(gt, xe, id)
> > xe_gt_shutdown(gt);
> > - xe_display_pm_shutdown_late(xe);
> > + xe_display_pm_shutdown_noaccel(xe);
> From the xe point of view, it shouldn't matter whether we call the noirq
> part before or after gt shutdown. I like the integration into xe_device to
> be as simple as possible, so could we keep the single
> xe_display_pm_shutdown_late() call?
As I had told you offline I was seeing if we could have a naming in i915
more generic like shutdown, shutdown_late, shutdown_early...
However I didn't like that that much... From the i915/display side, the
names still matters... that function can only be called at shutdown when
there's no irq anymore...
And from the xe side, I'm still working towards make the xe_display only
a thin layer for checking xe.enable_display parameter and nothing else.
So I still pretty much prefer the _noirq and _noaccel naming...
>
> Which reminds me to send out xe_display simplification once more to do the
> same for init..
> > } else {
> > /* BOOM! */
> > __xe_driver_flr(xe);
> Completely unrelated, do you happen to know if we need to call
> encoder_suspend/shutdown in the FLR path, whether it affects logic not on
> the chip itself?
>
> Cheers,
> ~Maarten
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-28 23:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-17 22:09 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 2/3] drm/xe: At shutdown disable commit helpers instead of flushing Rodrigo Vivi
2025-01-17 22:09 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
2025-01-20 13:42 ` Maarten Lankhorst
2025-01-21 19:37 ` Rodrigo Vivi
2025-01-28 23:27 ` Rodrigo Vivi
2025-01-17 23:23 ` ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915/display: Move shutdown sequences under display driver Patchwork
-- strict thread matches above, loose matches on Subject: below --
2025-01-22 10:40 [PATCH 1/3] " Rodrigo Vivi
2025-01-22 10:40 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
2024-11-14 20:22 [PATCH 1/3] drm/i915/display: Move shutdown sequences under display driver Rodrigo Vivi
2024-11-14 20:22 ` [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly Rodrigo Vivi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox