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