On Mon, Oct 07, 2024 at 03:42:34PM -0400, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Rodrigo Vivi
> Sent: Tuesday, September 24, 2024 1:35 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Maarten Lankhort <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Subject: [PATCH 05/31] drm/xe: At shutdown disable commit helpers instead of flushing
> >
> > This aligns with the current i915 display sequence.
> >
> > Cc: Maarten Lankhort <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > 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 5cbee5040e91..0237d458078b 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>
> >
> > @@ -364,10 +365,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(xe);
> > - intel_display_driver_suspend(xe);
> > +
> > + drm_atomic_helper_shutdown(&xe->drm);
>
> Isn't this functionally equivalent? The only difference AFAICT is that previously we
> set the display.restore.modeset_state = state, where the state was the
> return value for drm_atomic_helper_shutdown.
well, both ends up in disabling all crtc, but with the current
suspend there's unecessary duplication of the current atomic state.
It is a shutdown so we don't need to restore anything later.
>
> > }
> >
> > - xe_display_flush_cleanup_work(xe);
>
> And I'm guessing we're removing this line because it's a duplicate in the new
> execution path now?
all the CRTCs are disabled at this point. This is not doing anything...
>
> I won't block on it.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> -Jonathan Cavitt
>
> > intel_dp_mst_suspend(xe);
> > intel_hpd_cancel_work(xe);
> >
> > --
> > 2.46.0
> >
> >
