Re: [PATCH 3/3] drm/xe: Use i915-display shutdown sequence directly

[Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
>   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