[0/2] split out intel_display_power

Message

Daniele Ceraolo Spurio May 31, 2019, 10:24 p.m. UTC

Separate the display PM from the PCI-level runtime PM.
I'll follow this up with v2 of the rpm encapsulation series [1], but
I'd like to get this in before that to avoid having to carry this
big dumb diff in that series.

[1] https://patchwork.freedesktop.org/series/60751/

Daniele Ceraolo Spurio (2):
  drm/i915: extract intel_display_power.h/c from intel_runtime_pm.h/c
  drm/i915: move more defs in intel_display_power.h

 drivers/gpu/drm/i915/Makefile              |    1 +
 drivers/gpu/drm/i915/Makefile.header-test  |    1 +
 drivers/gpu/drm/i915/i915_drv.h            |  111 +-
 drivers/gpu/drm/i915/intel_display.h       |   82 -
 drivers/gpu/drm/i915/intel_display_power.c | 4612 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_display_power.h |  288 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c    | 4791 +-------------------
 drivers/gpu/drm/i915/intel_runtime_pm.h    |   83 +-
 8 files changed, 5004 insertions(+), 4965 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_display_power.c
 create mode 100644 drivers/gpu/drm/i915/intel_display_power.h

Comments

Chris Wilson June 1, 2019, 8:45 a.m. UTC | #1

Quoting Daniele Ceraolo Spurio (2019-05-31 23:24:07)
> Separate the display PM from the PCI-level runtime PM.
> I'll follow this up with v2 of the rpm encapsulation series [1], but
> I'd like to get this in before that to avoid having to carry this
> big dumb diff in that series.

With RUNTIME_PM_DEBUG disabled,

add/remove: 3/1 grow/shrink: 6/8 up/down: 396/-393 (3)
Function                                     old     new   delta
intel_runtime_pm_release                       -     274    +274
intel_runtime_pm_put_raw                       -      45     +45
intel_runtime_pm_put_unchecked                10      48     +38
intel_display_power_put_async_work           179     192     +13
intel_display_power_flush_work               117     126      +9
__intel_display_power_put_async              193     199      +6
intel_runtime_pm_get_raw                       -       4      +4
intel_display_power_grab_async_put_ref       117     121      +4
__warned                                     469     472      +3
intel_runtime_pm_get                          10       7      -3
intel_power_domains_enable                    38      33      -5
intel_display_power_put_unchecked             23      18      -5
intel_display_power_get_if_enabled           143     138      -5
intel_display_power_get                       84      79      -5
intel_power_domains_suspend                  490     480     -10
intel_power_domains_fini_hw                  116     106     -10
release_async_put_domains                    220     203     -17
__intel_runtime_pm_put.constprop             333       -    -333
Total: Before=23394388, After=23394391, chg +0.00%

which is my biggest worry when meddling with these, that we accidentally
explode production code with unused debugging (all those wakerefs).

Lgtm, I would like Jani to indicate that he's happy with this split as
well since he has been looking at very similar ideas.
-Chris

Jani Nikula June 3, 2019, 6:43 p.m. UTC | #2

On Sat, 01 Jun 2019, Chris Wilson <[e-mail]> wrote:
> Quoting Daniele Ceraolo Spurio (2019-05-31 23:24:07)
>> Separate the display PM from the PCI-level runtime PM.
>> I'll follow this up with v2 of the rpm encapsulation series [1], but
>> I'd like to get this in before that to avoid having to carry this
>> big dumb diff in that series.
>
> With RUNTIME_PM_DEBUG disabled,
>
> add/remove: 3/1 grow/shrink: 6/8 up/down: 396/-393 (3)
> Function                                     old     new   delta
> intel_runtime_pm_release                       -     274    +274
> intel_runtime_pm_put_raw                       -      45     +45
> intel_runtime_pm_put_unchecked                10      48     +38
> intel_display_power_put_async_work           179     192     +13
> intel_display_power_flush_work               117     126      +9
> __intel_display_power_put_async              193     199      +6
> intel_runtime_pm_get_raw                       -       4      +4
> intel_display_power_grab_async_put_ref       117     121      +4
> __warned                                     469     472      +3
> intel_runtime_pm_get                          10       7      -3
> intel_power_domains_enable                    38      33      -5
> intel_display_power_put_unchecked             23      18      -5
> intel_display_power_get_if_enabled           143     138      -5
> intel_display_power_get                       84      79      -5
> intel_power_domains_suspend                  490     480     -10
> intel_power_domains_fini_hw                  116     106     -10
> release_async_put_domains                    220     203     -17
> __intel_runtime_pm_put.constprop             333       -    -333
> Total: Before=23394388, After=23394391, chg +0.00%
>
> which is my biggest worry when meddling with these, that we accidentally
> explode production code with unused debugging (all those wakerefs).
>
> Lgtm, I would like Jani to indicate that he's happy with this split as
> well since he has been looking at very similar ideas.

I might bikeshed the naming, from the POV that functions would be nice
to be (eventually) named based on the name of the file they reside
in. But I guess intel_display_power.[ch] is as good as any I could come
up with, and not everything is going to follow the naming pattern
anyway.

I'd still like to get an ack from Imre before merging, but from my side
this is,

Acked-by: Jani Nikula <[e-mail]>

Thanks for doing this.



> -Chris
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak June 4, 2019, 7:12 a.m. UTC | #3

On Mon, Jun 03, 2019 at 09:43:59PM +0300, Jani Nikula wrote:
> On Sat, 01 Jun 2019, Chris Wilson <[e-mail]> wrote:
> > Quoting Daniele Ceraolo Spurio (2019-05-31 23:24:07)
> >> Separate the display PM from the PCI-level runtime PM.
> >> I'll follow this up with v2 of the rpm encapsulation series [1], but
> >> I'd like to get this in before that to avoid having to carry this
> >> big dumb diff in that series.
> >
> > With RUNTIME_PM_DEBUG disabled,
> >
> > add/remove: 3/1 grow/shrink: 6/8 up/down: 396/-393 (3)
> > Function                                     old     new   delta
> > intel_runtime_pm_release                       -     274    +274
> > intel_runtime_pm_put_raw                       -      45     +45
> > intel_runtime_pm_put_unchecked                10      48     +38
> > intel_display_power_put_async_work           179     192     +13
> > intel_display_power_flush_work               117     126      +9
> > __intel_display_power_put_async              193     199      +6
> > intel_runtime_pm_get_raw                       -       4      +4
> > intel_display_power_grab_async_put_ref       117     121      +4
> > __warned                                     469     472      +3
> > intel_runtime_pm_get                          10       7      -3
> > intel_power_domains_enable                    38      33      -5
> > intel_display_power_put_unchecked             23      18      -5
> > intel_display_power_get_if_enabled           143     138      -5
> > intel_display_power_get                       84      79      -5
> > intel_power_domains_suspend                  490     480     -10
> > intel_power_domains_fini_hw                  116     106     -10
> > release_async_put_domains                    220     203     -17
> > __intel_runtime_pm_put.constprop             333       -    -333
> > Total: Before=23394388, After=23394391, chg +0.00%
> >
> > which is my biggest worry when meddling with these, that we accidentally
> > explode production code with unused debugging (all those wakerefs).
> >
> > Lgtm, I would like Jani to indicate that he's happy with this split as
> > well since he has been looking at very similar ideas.
> 
> I might bikeshed the naming, from the POV that functions would be nice
> to be (eventually) named based on the name of the file they reside
> in. But I guess intel_display_power.[ch] is as good as any I could come
> up with, and not everything is going to follow the naming pattern
> anyway.
> 
> I'd still like to get an ack from Imre before merging, but from my side
> this is,
> 
> Acked-by: Jani Nikula <[e-mail]>
> 
> Thanks for doing this.

Keeping the display power related functions grouped in a separate file
makes sense to me:

Acked-by: Imre Deak <[e-mail]>

> 
> 
> 
> > -Chris
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Chris Wilson June 4, 2019, 7:34 a.m. UTC | #4

Quoting Imre Deak (2019-06-04 08:12:22)
> On Mon, Jun 03, 2019 at 09:43:59PM +0300, Jani Nikula wrote:
> > On Sat, 01 Jun 2019, Chris Wilson <[e-mail]> wrote:
> > > Quoting Daniele Ceraolo Spurio (2019-05-31 23:24:07)
> > >> Separate the display PM from the PCI-level runtime PM.
> > >> I'll follow this up with v2 of the rpm encapsulation series [1], but
> > >> I'd like to get this in before that to avoid having to carry this
> > >> big dumb diff in that series.
> > >
> > > With RUNTIME_PM_DEBUG disabled,
> > >
> > > add/remove: 3/1 grow/shrink: 6/8 up/down: 396/-393 (3)
> > > Function                                     old     new   delta
> > > intel_runtime_pm_release                       -     274    +274
> > > intel_runtime_pm_put_raw                       -      45     +45
> > > intel_runtime_pm_put_unchecked                10      48     +38
> > > intel_display_power_put_async_work           179     192     +13
> > > intel_display_power_flush_work               117     126      +9
> > > __intel_display_power_put_async              193     199      +6
> > > intel_runtime_pm_get_raw                       -       4      +4
> > > intel_display_power_grab_async_put_ref       117     121      +4
> > > __warned                                     469     472      +3
> > > intel_runtime_pm_get                          10       7      -3
> > > intel_power_domains_enable                    38      33      -5
> > > intel_display_power_put_unchecked             23      18      -5
> > > intel_display_power_get_if_enabled           143     138      -5
> > > intel_display_power_get                       84      79      -5
> > > intel_power_domains_suspend                  490     480     -10
> > > intel_power_domains_fini_hw                  116     106     -10
> > > release_async_put_domains                    220     203     -17
> > > __intel_runtime_pm_put.constprop             333       -    -333
> > > Total: Before=23394388, After=23394391, chg +0.00%
> > >
> > > which is my biggest worry when meddling with these, that we accidentally
> > > explode production code with unused debugging (all those wakerefs).
> > >
> > > Lgtm, I would like Jani to indicate that he's happy with this split as
> > > well since he has been looking at very similar ideas.
> > 
> > I might bikeshed the naming, from the POV that functions would be nice
> > to be (eventually) named based on the name of the file they reside
> > in. But I guess intel_display_power.[ch] is as good as any I could come
> > up with, and not everything is going to follow the naming pattern
> > anyway.
> > 
> > I'd still like to get an ack from Imre before merging, but from my side
> > this is,
> > 
> > Acked-by: Jani Nikula <[e-mail]>
> > 
> > Thanks for doing this.
> 
> Keeping the display power related functions grouped in a separate file
> makes sense to me:
> 
> Acked-by: Imre Deak <[e-mail]>

Sold.
-Chris