[PATCH 05/31] drm/xe: At shutdown disable commit helpers instead of flushing

[Rodrigo Vivi]

On Mon, Oct 07, 2024 at 03:42:34PM -0400, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Rodrigo Vivi
> Sent: Tuesday, September 24, 2024 1:35 PM
> To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Maarten Lankhort <maarten.lankhorst at linux.intel.com>
> 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 at linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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 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 at intel.com>
> -Jonathan Cavitt
> 
> >  	intel_dp_mst_suspend(xe);
> >  	intel_hpd_cancel_work(xe);
> >  
> > -- 
> > 2.46.0
> > 
> >