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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>