| Message ID | 20240924204222.246862-6-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reconcile i915's and xe's display power mgt sequences | expand |
-----Original Message----- From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Rodrigo Vivi Sent: Tuesday, September 24, 2024 1:35 PM To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org Cc: Deak, Imre <imre.deak@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Maarten Lankhort <maarten.lankhorst@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@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. > } > > - 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? I won't block on it. Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > intel_dp_mst_suspend(xe); > intel_hpd_cancel_work(xe); > > -- > 2.46.0 > >
On Mon, Oct 07, 2024 at 03:42:34PM -0400, Cavitt, Jonathan wrote: > -----Original Message----- > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Rodrigo Vivi > Sent: Tuesday, September 24, 2024 1:35 PM > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Maarten Lankhort <maarten.lankhorst@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@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@intel.com> > -Jonathan Cavitt > > > intel_dp_mst_suspend(xe); > > intel_hpd_cancel_work(xe); > > > > -- > > 2.46.0 > > > >
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); } - xe_display_flush_cleanup_work(xe); intel_dp_mst_suspend(xe); intel_hpd_cancel_work(xe);
This aligns with the current i915 display sequence. Cc: Maarten Lankhort <maarten.lankhorst@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/xe/display/xe_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)