mbox series

[00/31] Reconcile i915's and xe's display power mgt sequences

Message ID 20240924204222.246862-1-rodrigo.vivi@intel.com (mailing list archive)
Headers show
Series Reconcile i915's and xe's display power mgt sequences | expand

Message

Rodrigo Vivi Sept. 24, 2024, 8:35 p.m. UTC
Reconcile i915's and xe's display power management sequences.

The main goal of this series is to ensure that the display
power management sequences from both drivers are fully aligned.
So, it needs to live under i915/display/ and xe_display_pm
can only be a wrapper to check for the xe's module
parameter before jumping in the i915/display functions.

During this process, it was clear that the Xe's runtime pm
sequences for integrated gfx (non d3cold capable) was totally
misaligned with i915 and prune to more bugs. So, this series ends
up fixing this and bringing a full alignment there as well.

The one new thing under i915 is the d3cold sequences. Apparently
that can be reduced, but for now this is what is validated and
stable on both DG2 and BMG. So, let's move that to the
i915/display side as well and continue on the improvements with
the code in the right place.

When I started this work I was hoping that I could run more
conversions to intel_display struct before moving things from
i915_driver.c to intel_display_driver, however that was a deeper
task and this series is already deep enough.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


Rodrigo Vivi (31):
  drm/i915: Remove vga and gmbus seq out of i915_restore_display
  drm/i915/display: Convert i915_suspend into i9xx_display_sr
  drm/i915/display: Move regfile registers intel_display.restore
  drm/i915/display: Move shutdown sequences under display driver
  drm/xe: At shutdown disable commit helpers instead of flushing
  drm/xe: Use i915-display shutdown sequence directly
  drm/{i915,xe}/display: Move DP MST calls to display_driver
  drm/i915/display: Move suspend sequences to intel_display_driver
  drm/xe/display: Delay hpd_init resume
  drm/xe/display: Spin-off xe_display runtime/d3cold sequences
  drm/{i915,xe}: Consolidate display resume functions
  drm/i915: Remove lingering pci_save_state
  drm/{i915,xe}: Consolidate display suspend functions
  drm/i915/display: Move resume sequences to intel_display_driver
  drm/xe/display: Delay dsm handler registration
  drm/{i915,xe}: Move power_domains suspend/resume to display_power
  drm/{i915,xe}: Move remaining intel_power_domains to intel_display
  drm/i915/display: Split resume_noirq calls for now
  drm/xe/display: Align display resume sequence with i915
  drm/xe/display: Align suspend sequence with i915
  drm/{i915,xe}/display: Move dsm registration under intel_driver
  drm/i915/display: Move runtime pm related calls under
    intel_display_driver
  drm/xe/display: Prepare runtime pm functions
  drm/xe/display: Call intel_hpd_init on every runtime resume
  drm/xe/display: Move hpd_poll calls to later runtime stages
  drm/xe/display: Add missing watermark ipc update at runtime resume
  drm/xe/display: Notify opregion upon runtime suspend/resume non-d3cold
  drm/xe/display: Move display runtime suspend to a later point
  drm/xe/display: Kill crtc commit flush
  drm/xe/display: Add missing power display handling on non-d3cold rpm
  drm/{i915,xe}/display: Consolidade entire runtime pm sequence

 drivers/gpu/drm/i915/Makefile                 |   2 +-
 .../gpu/drm/i915/display/i9xx_display_sr.c    |  99 +++++++
 .../gpu/drm/i915/display/i9xx_display_sr.h    |  14 +
 .../gpu/drm/i915/display/intel_display_core.h |   5 +
 .../drm/i915/display/intel_display_driver.c   | 247 ++++++++++++++++++
 .../drm/i915/display/intel_display_driver.h   |  18 ++
 .../drm/i915/display/intel_display_power.c    |   6 +-
 .../drm/i915/display/intel_display_power.h    |   2 +-
 drivers/gpu/drm/i915/i915_driver.c            | 161 ++----------
 drivers/gpu/drm/i915/i915_drv.h               |   9 -
 drivers/gpu/drm/i915/i915_suspend.c           | 141 ----------
 drivers/gpu/drm/i915/i915_suspend.h           |  14 -
 drivers/gpu/drm/xe/display/xe_display.c       | 185 ++++---------
 drivers/gpu/drm/xe/display/xe_display.h       |  11 +-
 drivers/gpu/drm/xe/display/xe_fb_pin.c        |   8 +
 drivers/gpu/drm/xe/xe_device.c                |   4 +-
 drivers/gpu/drm/xe/xe_pm.c                    |  16 +-
 17 files changed, 498 insertions(+), 444 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.c
 create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.h
 delete mode 100644 drivers/gpu/drm/i915/i915_suspend.c
 delete mode 100644 drivers/gpu/drm/i915/i915_suspend.h

Comments

Jani Nikula Sept. 25, 2024, 10:32 a.m. UTC | #1
On Tue, 24 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Reconcile i915's and xe's display power management sequences.
>
> The main goal of this series is to ensure that the display
> power management sequences from both drivers are fully aligned.
> So, it needs to live under i915/display/ and xe_display_pm
> can only be a wrapper to check for the xe's module
> parameter before jumping in the i915/display functions.
>
> During this process, it was clear that the Xe's runtime pm
> sequences for integrated gfx (non d3cold capable) was totally
> misaligned with i915 and prune to more bugs. So, this series ends
> up fixing this and bringing a full alignment there as well.
>
> The one new thing under i915 is the d3cold sequences. Apparently
> that can be reduced, but for now this is what is validated and
> stable on both DG2 and BMG. So, let's move that to the
> i915/display side as well and continue on the improvements with
> the code in the right place.
>
> When I started this work I was hoping that I could run more
> conversions to intel_display struct before moving things from
> i915_driver.c to intel_display_driver, however that was a deeper
> task and this series is already deep enough.

Holy crap this is scary and a PITA to review but exactly what we'll
need! Thanks for doing this. I've been shying away from this too long.

I'll try to put in some review effort, but at the same time just more
eyeballs than mine would be great.

BR,
Jani.
Rodrigo Vivi Sept. 25, 2024, 9 p.m. UTC | #2
On Wed, Sep 25, 2024 at 01:32:03PM +0300, Jani Nikula wrote:
> On Tue, 24 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Reconcile i915's and xe's display power management sequences.
> >
> > The main goal of this series is to ensure that the display
> > power management sequences from both drivers are fully aligned.
> > So, it needs to live under i915/display/ and xe_display_pm
> > can only be a wrapper to check for the xe's module
> > parameter before jumping in the i915/display functions.
> >
> > During this process, it was clear that the Xe's runtime pm
> > sequences for integrated gfx (non d3cold capable) was totally
> > misaligned with i915 and prune to more bugs. So, this series ends
> > up fixing this and bringing a full alignment there as well.
> >
> > The one new thing under i915 is the d3cold sequences. Apparently
> > that can be reduced, but for now this is what is validated and
> > stable on both DG2 and BMG. So, let's move that to the
> > i915/display side as well and continue on the improvements with
> > the code in the right place.
> >
> > When I started this work I was hoping that I could run more
> > conversions to intel_display struct before moving things from
> > i915_driver.c to intel_display_driver, however that was a deeper
> > task and this series is already deep enough.
> 
> Holy crap this is scary and a PITA to review but exactly what we'll
> need! Thanks for doing this. I've been shying away from this too long.

yeap! It was very painful to do this... took me much longer then
I was expecting for a refactor :/

But we need to rip the bandaid off

> 
> I'll try to put in some review effort, but at the same time just more
> eyeballs than mine would be great.

Thank you so much!
Yeap, every help is very much appreciated!

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel
Cavitt, Jonathan Oct. 8, 2024, 3:24 p.m. UTC | #3
-----Original Message-----
From: Intel-gfx <intel-gfx-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>
Subject: [PATCH 00/31] Reconcile i915's and xe's display power mgt sequences
> 
> Reconcile i915's and xe's display power management sequences.
> 
> The main goal of this series is to ensure that the display
> power management sequences from both drivers are fully aligned.
> So, it needs to live under i915/display/ and xe_display_pm
> can only be a wrapper to check for the xe's module
> parameter before jumping in the i915/display functions.
> 
> During this process, it was clear that the Xe's runtime pm
> sequences for integrated gfx (non d3cold capable) was totally
> misaligned with i915 and prune to more bugs. So, this series ends
> up fixing this and bringing a full alignment there as well.
> 
> The one new thing under i915 is the d3cold sequences. Apparently
> that can be reduced, but for now this is what is validated and
> stable on both DG2 and BMG. So, let's move that to the
> i915/display side as well and continue on the improvements with
> the code in the right place.
> 
> When I started this work I was hoping that I could run more
> conversions to intel_display struct before moving things from
> i915_driver.c to intel_display_driver, however that was a deeper
> task and this series is already deep enough.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I did individual reviews of all 31 patches in the series, and I gave my
RB to all of them with some notes.  However, there were just a few
things that I felt I needed to say before we called this series closed.

Don't worry: none of this is blocking.


1. Are we sure that aligning i915 and Xe like this is the correct
course of action?  I've been reprimanded in the past for creating
patches whose overall conceit is "we do 'X' thing in 'Y' way on
i915, so we should also be doing 'X' thing in 'Y' way on Xe".  So, I
figured I should at least point this out.


2. Several patches in the series operate on both i915 and Xe at the
same time.  I don't think there's any strict rule stating that we can't
do something like that, but IMO we should consider splitting those
patches into i915 and Xe components where possible/logical.


3. When all is said and done, this series eventually puts all of the
driver management code into intel_display_driver.c.  So any
changes made to xe_display.c, for example, seem superfluous
given we're intending on centralizing the display code outside of
xe_display.c in the end.  It'd be a lot of work to reorder the patch
series, especially at this point, but if we needed to start again from
scratch for any reason, it might be worth ordering the changes as such:

	- Create all necessary helper functions in intel_display_driver
	   for i915 and Xe, using them in the i915 execution path.

	- Use the new helper functions in the xe execution path.

	- Create and use all necessary helper functions for the xe
	   execution path.


Again, none of this is blocking, so there's no need to upend everything
and start over.  I just felt I needed to state some of these things as a
part of due diligence.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

> 
> 
> Rodrigo Vivi (31):
>   drm/i915: Remove vga and gmbus seq out of i915_restore_display
>   drm/i915/display: Convert i915_suspend into i9xx_display_sr
>   drm/i915/display: Move regfile registers intel_display.restore
>   drm/i915/display: Move shutdown sequences under display driver
>   drm/xe: At shutdown disable commit helpers instead of flushing
>   drm/xe: Use i915-display shutdown sequence directly
>   drm/{i915,xe}/display: Move DP MST calls to display_driver
>   drm/i915/display: Move suspend sequences to intel_display_driver
>   drm/xe/display: Delay hpd_init resume
>   drm/xe/display: Spin-off xe_display runtime/d3cold sequences
>   drm/{i915,xe}: Consolidate display resume functions
>   drm/i915: Remove lingering pci_save_state
>   drm/{i915,xe}: Consolidate display suspend functions
>   drm/i915/display: Move resume sequences to intel_display_driver
>   drm/xe/display: Delay dsm handler registration
>   drm/{i915,xe}: Move power_domains suspend/resume to display_power
>   drm/{i915,xe}: Move remaining intel_power_domains to intel_display
>   drm/i915/display: Split resume_noirq calls for now
>   drm/xe/display: Align display resume sequence with i915
>   drm/xe/display: Align suspend sequence with i915
>   drm/{i915,xe}/display: Move dsm registration under intel_driver
>   drm/i915/display: Move runtime pm related calls under
>     intel_display_driver
>   drm/xe/display: Prepare runtime pm functions
>   drm/xe/display: Call intel_hpd_init on every runtime resume
>   drm/xe/display: Move hpd_poll calls to later runtime stages
>   drm/xe/display: Add missing watermark ipc update at runtime resume
>   drm/xe/display: Notify opregion upon runtime suspend/resume non-d3cold
>   drm/xe/display: Move display runtime suspend to a later point
>   drm/xe/display: Kill crtc commit flush
>   drm/xe/display: Add missing power display handling on non-d3cold rpm
>   drm/{i915,xe}/display: Consolidade entire runtime pm sequence
> 
>  drivers/gpu/drm/i915/Makefile                 |   2 +-
>  .../gpu/drm/i915/display/i9xx_display_sr.c    |  99 +++++++
>  .../gpu/drm/i915/display/i9xx_display_sr.h    |  14 +
>  .../gpu/drm/i915/display/intel_display_core.h |   5 +
>  .../drm/i915/display/intel_display_driver.c   | 247 ++++++++++++++++++
>  .../drm/i915/display/intel_display_driver.h   |  18 ++
>  .../drm/i915/display/intel_display_power.c    |   6 +-
>  .../drm/i915/display/intel_display_power.h    |   2 +-
>  drivers/gpu/drm/i915/i915_driver.c            | 161 ++----------
>  drivers/gpu/drm/i915/i915_drv.h               |   9 -
>  drivers/gpu/drm/i915/i915_suspend.c           | 141 ----------
>  drivers/gpu/drm/i915/i915_suspend.h           |  14 -
>  drivers/gpu/drm/xe/display/xe_display.c       | 185 ++++---------
>  drivers/gpu/drm/xe/display/xe_display.h       |  11 +-
>  drivers/gpu/drm/xe/display/xe_fb_pin.c        |   8 +
>  drivers/gpu/drm/xe/xe_device.c                |   4 +-
>  drivers/gpu/drm/xe/xe_pm.c                    |  16 +-
>  17 files changed, 498 insertions(+), 444 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.c
>  create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.h
>  delete mode 100644 drivers/gpu/drm/i915/i915_suspend.c
>  delete mode 100644 drivers/gpu/drm/i915/i915_suspend.h
> 
> -- 
> 2.46.0
> 
>
Rodrigo Vivi Nov. 11, 2024, 7:23 p.m. UTC | #4
On Tue, Oct 08, 2024 at 11:24:42AM -0400, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-gfx <intel-gfx-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>
> Subject: [PATCH 00/31] Reconcile i915's and xe's display power mgt sequences
> > 
> > Reconcile i915's and xe's display power management sequences.
> > 
> > The main goal of this series is to ensure that the display
> > power management sequences from both drivers are fully aligned.
> > So, it needs to live under i915/display/ and xe_display_pm
> > can only be a wrapper to check for the xe's module
> > parameter before jumping in the i915/display functions.
> > 
> > During this process, it was clear that the Xe's runtime pm
> > sequences for integrated gfx (non d3cold capable) was totally
> > misaligned with i915 and prune to more bugs. So, this series ends
> > up fixing this and bringing a full alignment there as well.
> > 
> > The one new thing under i915 is the d3cold sequences. Apparently
> > that can be reduced, but for now this is what is validated and
> > stable on both DG2 and BMG. So, let's move that to the
> > i915/display side as well and continue on the improvements with
> > the code in the right place.
> > 
> > When I started this work I was hoping that I could run more
> > conversions to intel_display struct before moving things from
> > i915_driver.c to intel_display_driver, however that was a deeper
> > task and this series is already deep enough.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> I did individual reviews of all 31 patches in the series, and I gave my
> RB to all of them with some notes.  However, there were just a few
> things that I felt I needed to say before we called this series closed.

First of all, thank you so much for the detailed and careful review of
every patch.
Also, I'm sorry for taking so long to come back here.

> 
> Don't worry: none of this is blocking.

No worries. In my first step now I'm only getting the likely
non-controversial ones so we can already merge them and then
iterate on the ones where you and others might have questions
and concerns.

> 
> 
> 1. Are we sure that aligning i915 and Xe like this is the correct
> course of action?  I've been reprimanded in the past for creating
> patches whose overall conceit is "we do 'X' thing in 'Y' way on
> i915, so we should also be doing 'X' thing in 'Y' way on Xe".  So, I
> figured I should at least point this out.

Yes, we are sure on this. Our overall goal is to have a independent
display driver. So every display functionality, including the PM flows
should be identical and living in i915/display side.

> 
> 
> 2. Several patches in the series operate on both i915 and Xe at the
> same time.  I don't think there's any strict rule stating that we can't
> do something like that, but IMO we should consider splitting those
> patches into i915 and Xe components where possible/logical.

yeap, indeed. But in every case here, if we need separated patches we
would need to create some temporary stub. So, better to do them together.
And the goal is to get them through drm-intel-next since they are all
display related in the end of the day.

> 
> 
> 3. When all is said and done, this series eventually puts all of the
> driver management code into intel_display_driver.c.  So any
> changes made to xe_display.c, for example, seem superfluous
> given we're intending on centralizing the display code outside of
> xe_display.c in the end.  It'd be a lot of work to reorder the patch
> series, especially at this point, but if we needed to start again from
> scratch for any reason, it might be worth ordering the changes as such:
> 
> 	- Create all necessary helper functions in intel_display_driver
> 	   for i915 and Xe, using them in the i915 execution path.
> 
> 	- Use the new helper functions in the xe execution path.
> 
> 	- Create and use all necessary helper functions for the xe
> 	   execution path.

In xe we decided to have an extra way to disable display. So  we need
this intermediate stub. Otherwise this is the goal, to have everything
under i915/display/

> 
> 
> Again, none of this is blocking, so there's no need to upend everything
> and start over.  I just felt I needed to state some of these things as a
> part of due diligence.
> 
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> -Jonathan Cavitt

Thank you so much,
Rodrigo.

> 
> > 
> > 
> > Rodrigo Vivi (31):
> >   drm/i915: Remove vga and gmbus seq out of i915_restore_display
> >   drm/i915/display: Convert i915_suspend into i9xx_display_sr
> >   drm/i915/display: Move regfile registers intel_display.restore
> >   drm/i915/display: Move shutdown sequences under display driver
> >   drm/xe: At shutdown disable commit helpers instead of flushing
> >   drm/xe: Use i915-display shutdown sequence directly
> >   drm/{i915,xe}/display: Move DP MST calls to display_driver
> >   drm/i915/display: Move suspend sequences to intel_display_driver
> >   drm/xe/display: Delay hpd_init resume
> >   drm/xe/display: Spin-off xe_display runtime/d3cold sequences
> >   drm/{i915,xe}: Consolidate display resume functions
> >   drm/i915: Remove lingering pci_save_state
> >   drm/{i915,xe}: Consolidate display suspend functions
> >   drm/i915/display: Move resume sequences to intel_display_driver
> >   drm/xe/display: Delay dsm handler registration
> >   drm/{i915,xe}: Move power_domains suspend/resume to display_power
> >   drm/{i915,xe}: Move remaining intel_power_domains to intel_display
> >   drm/i915/display: Split resume_noirq calls for now
> >   drm/xe/display: Align display resume sequence with i915
> >   drm/xe/display: Align suspend sequence with i915
> >   drm/{i915,xe}/display: Move dsm registration under intel_driver
> >   drm/i915/display: Move runtime pm related calls under
> >     intel_display_driver
> >   drm/xe/display: Prepare runtime pm functions
> >   drm/xe/display: Call intel_hpd_init on every runtime resume
> >   drm/xe/display: Move hpd_poll calls to later runtime stages
> >   drm/xe/display: Add missing watermark ipc update at runtime resume
> >   drm/xe/display: Notify opregion upon runtime suspend/resume non-d3cold
> >   drm/xe/display: Move display runtime suspend to a later point
> >   drm/xe/display: Kill crtc commit flush
> >   drm/xe/display: Add missing power display handling on non-d3cold rpm
> >   drm/{i915,xe}/display: Consolidade entire runtime pm sequence
> > 
> >  drivers/gpu/drm/i915/Makefile                 |   2 +-
> >  .../gpu/drm/i915/display/i9xx_display_sr.c    |  99 +++++++
> >  .../gpu/drm/i915/display/i9xx_display_sr.h    |  14 +
> >  .../gpu/drm/i915/display/intel_display_core.h |   5 +
> >  .../drm/i915/display/intel_display_driver.c   | 247 ++++++++++++++++++
> >  .../drm/i915/display/intel_display_driver.h   |  18 ++
> >  .../drm/i915/display/intel_display_power.c    |   6 +-
> >  .../drm/i915/display/intel_display_power.h    |   2 +-
> >  drivers/gpu/drm/i915/i915_driver.c            | 161 ++----------
> >  drivers/gpu/drm/i915/i915_drv.h               |   9 -
> >  drivers/gpu/drm/i915/i915_suspend.c           | 141 ----------
> >  drivers/gpu/drm/i915/i915_suspend.h           |  14 -
> >  drivers/gpu/drm/xe/display/xe_display.c       | 185 ++++---------
> >  drivers/gpu/drm/xe/display/xe_display.h       |  11 +-
> >  drivers/gpu/drm/xe/display/xe_fb_pin.c        |   8 +
> >  drivers/gpu/drm/xe/xe_device.c                |   4 +-
> >  drivers/gpu/drm/xe/xe_pm.c                    |  16 +-
> >  17 files changed, 498 insertions(+), 444 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.c
> >  create mode 100644 drivers/gpu/drm/i915/display/i9xx_display_sr.h
> >  delete mode 100644 drivers/gpu/drm/i915/i915_suspend.c
> >  delete mode 100644 drivers/gpu/drm/i915/i915_suspend.h
> > 
> > -- 
> > 2.46.0
> > 
> >