Re: [PATCH V2 00/17] Unified entry point for other blocks to interact with power

["Christian König" <christian.koenig@amd.com>]

Am 30.11.21 um 08:42 schrieb Evan Quan:
> There are several problems with current power implementations:
> 1. Too many internal details are exposed to other blocks. Thus to interact with
>     power, they need to know which power framework is used(powerplay vs swsmu)
>     or even whether some API is implemented.
> 2. A lot of cross callings exist which make it hard to get a whole picture of
>     the code hierarchy. And that makes any code change/increment error-prone.
> 3. Many different types of lock are used. It is calculated there is totally
>     13 different locks are used within power. Some of them are even designed for
>     the same purpose.
>
> To ease the problems above, this patch series try to
> 1. provide unified entry point for other blocks to interact with power.
> 2. relocate some source code piece/headers to avoid cross callings.
> 3. enforce a unified lock protection on those entry point APIs above.
>     That makes the future optimization for unnecessary power locks possible.

I only skimmed over it, but that looks really good on first glance.

But you need to have Alex take a look as well since I only have a very 
high level understanding of power management.

Regards,
Christian.

>
> Evan Quan (17):
>    drm/amd/pm: do not expose implementation details to other blocks out
>      of power
>    drm/amd/pm: do not expose power implementation details to amdgpu_pm.c
>    drm/amd/pm: do not expose power implementation details to display
>    drm/amd/pm: do not expose those APIs used internally only in
>      amdgpu_dpm.c
>    drm/amd/pm: do not expose those APIs used internally only in si_dpm.c
>    drm/amd/pm: do not expose the API used internally only in kv_dpm.c
>    drm/amd/pm: create a new holder for those APIs used only by legacy
>      ASICs(si/kv)
>    drm/amd/pm: move pp_force_state_enabled member to amdgpu_pm structure
>    drm/amd/pm: optimize the amdgpu_pm_compute_clocks() implementations
>    drm/amd/pm: move those code piece used by Stoney only to smu8_hwmgr.c
>    drm/amd/pm: correct the usage for amdgpu_dpm_dispatch_task()
>    drm/amd/pm: drop redundant or unused APIs and data structures
>    drm/amd/pm: do not expose the smu_context structure used internally in
>      power
>    drm/amd/pm: relocate the power related headers
>    drm/amd/pm: drop unnecessary gfxoff controls
>    drm/amd/pm: revise the performance level setting APIs
>    drm/amd/pm: unified lock protections in amdgpu_dpm.c
>
>   drivers/gpu/drm/amd/amdgpu/aldebaran.c        |    2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h           |    7 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c  |  421 ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.h  |   30 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c   |   25 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |    6 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c       |   18 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h       |    7 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       |    5 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c       |    5 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c   |    2 +-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |    6 +-
>   .../amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c  |  246 +-
>   .../gpu/drm/amd/include/kgd_pp_interface.h    |   14 +
>   drivers/gpu/drm/amd/pm/Makefile               |   12 +-
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c           | 2435 ++++++++---------
>   drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c  |   94 +
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c            |  568 ++--
>   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h       |  339 +--
>   .../gpu/drm/amd/pm/inc/amdgpu_dpm_internal.h  |   32 +
>   drivers/gpu/drm/amd/pm/legacy-dpm/Makefile    |   32 +
>   .../pm/{powerplay => legacy-dpm}/cik_dpm.h    |    0
>   .../amd/pm/{powerplay => legacy-dpm}/kv_dpm.c |   47 +-
>   .../amd/pm/{powerplay => legacy-dpm}/kv_dpm.h |    0
>   .../amd/pm/{powerplay => legacy-dpm}/kv_smc.c |    0
>   .../gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c    | 1510 ++++++++++
>   .../gpu/drm/amd/pm/legacy-dpm/legacy_dpm.h    |   71 +
>   .../amd/pm/{powerplay => legacy-dpm}/ppsmc.h  |    0
>   .../pm/{powerplay => legacy-dpm}/r600_dpm.h   |    0
>   .../amd/pm/{powerplay => legacy-dpm}/si_dpm.c |  111 +-
>   .../amd/pm/{powerplay => legacy-dpm}/si_dpm.h |    7 +
>   .../amd/pm/{powerplay => legacy-dpm}/si_smc.c |    0
>   .../{powerplay => legacy-dpm}/sislands_smc.h  |    0
>   drivers/gpu/drm/amd/pm/powerplay/Makefile     |    4 -
>   .../gpu/drm/amd/pm/powerplay/amd_powerplay.c  |   51 +-
>   .../drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c   |   10 +-
>   .../pm/{ => powerplay}/inc/amd_powerplay.h    |    0
>   .../drm/amd/pm/{ => powerplay}/inc/cz_ppsmc.h |    0
>   .../amd/pm/{ => powerplay}/inc/fiji_ppsmc.h   |    0
>   .../pm/{ => powerplay}/inc/hardwaremanager.h  |    0
>   .../drm/amd/pm/{ => powerplay}/inc/hwmgr.h    |    3 -
>   .../{ => powerplay}/inc/polaris10_pwrvirus.h  |    0
>   .../amd/pm/{ => powerplay}/inc/power_state.h  |    0
>   .../drm/amd/pm/{ => powerplay}/inc/pp_debug.h |    0
>   .../amd/pm/{ => powerplay}/inc/pp_endian.h    |    0
>   .../amd/pm/{ => powerplay}/inc/pp_thermal.h   |    0
>   .../amd/pm/{ => powerplay}/inc/ppinterrupt.h  |    0
>   .../drm/amd/pm/{ => powerplay}/inc/rv_ppsmc.h |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu10.h    |    0
>   .../pm/{ => powerplay}/inc/smu10_driver_if.h  |    0
>   .../pm/{ => powerplay}/inc/smu11_driver_if.h  |    0
>   .../gpu/drm/amd/pm/{ => powerplay}/inc/smu7.h |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu71.h    |    0
>   .../pm/{ => powerplay}/inc/smu71_discrete.h   |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu72.h    |    0
>   .../pm/{ => powerplay}/inc/smu72_discrete.h   |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu73.h    |    0
>   .../pm/{ => powerplay}/inc/smu73_discrete.h   |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu74.h    |    0
>   .../pm/{ => powerplay}/inc/smu74_discrete.h   |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smu75.h    |    0
>   .../pm/{ => powerplay}/inc/smu75_discrete.h   |    0
>   .../amd/pm/{ => powerplay}/inc/smu7_common.h  |    0
>   .../pm/{ => powerplay}/inc/smu7_discrete.h    |    0
>   .../amd/pm/{ => powerplay}/inc/smu7_fusion.h  |    0
>   .../amd/pm/{ => powerplay}/inc/smu7_ppsmc.h   |    0
>   .../gpu/drm/amd/pm/{ => powerplay}/inc/smu8.h |    0
>   .../amd/pm/{ => powerplay}/inc/smu8_fusion.h  |    0
>   .../gpu/drm/amd/pm/{ => powerplay}/inc/smu9.h |    0
>   .../pm/{ => powerplay}/inc/smu9_driver_if.h   |    0
>   .../{ => powerplay}/inc/smu_ucode_xfer_cz.h   |    0
>   .../{ => powerplay}/inc/smu_ucode_xfer_vi.h   |    0
>   .../drm/amd/pm/{ => powerplay}/inc/smumgr.h   |    0
>   .../amd/pm/{ => powerplay}/inc/tonga_ppsmc.h  |    0
>   .../amd/pm/{ => powerplay}/inc/vega10_ppsmc.h |    0
>   .../inc/vega12/smu9_driver_if.h               |    0
>   .../amd/pm/{ => powerplay}/inc/vega12_ppsmc.h |    0
>   .../amd/pm/{ => powerplay}/inc/vega20_ppsmc.h |    0
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     |   95 +-
>   .../amd/pm/{ => swsmu}/inc/aldebaran_ppsmc.h  |    0
>   .../drm/amd/pm/{ => swsmu}/inc/amdgpu_smu.h   |   20 +-
>   .../amd/pm/{ => swsmu}/inc/arcturus_ppsmc.h   |    0
>   .../inc/smu11_driver_if_arcturus.h            |    0
>   .../inc/smu11_driver_if_cyan_skillfish.h      |    0
>   .../{ => swsmu}/inc/smu11_driver_if_navi10.h  |    0
>   .../inc/smu11_driver_if_sienna_cichlid.h      |    0
>   .../{ => swsmu}/inc/smu11_driver_if_vangogh.h |    0
>   .../amd/pm/{ => swsmu}/inc/smu12_driver_if.h  |    0
>   .../inc/smu13_driver_if_aldebaran.h           |    0
>   .../inc/smu13_driver_if_yellow_carp.h         |    0
>   .../pm/{ => swsmu}/inc/smu_11_0_cdr_table.h   |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_types.h    |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_v11_0.h    |    0
>   .../pm/{ => swsmu}/inc/smu_v11_0_7_ppsmc.h    |    0
>   .../pm/{ => swsmu}/inc/smu_v11_0_7_pptable.h  |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v11_0_ppsmc.h  |    0
>   .../pm/{ => swsmu}/inc/smu_v11_0_pptable.h    |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v11_5_pmfw.h   |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v11_5_ppsmc.h  |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v11_8_pmfw.h   |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v11_8_ppsmc.h  |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_v12_0.h    |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v12_0_ppsmc.h  |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_v13_0.h    |    0
>   .../amd/pm/{ => swsmu}/inc/smu_v13_0_1_pmfw.h |    0
>   .../pm/{ => swsmu}/inc/smu_v13_0_1_ppsmc.h    |    0
>   .../pm/{ => swsmu}/inc/smu_v13_0_pptable.h    |    0
>   .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c |   10 +-
>   .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   |    9 +-
>   .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c   |   34 +-
>   .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    |   11 +-
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    |   10 +-
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |   15 +-
>   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |    4 +
>   114 files changed, 3657 insertions(+), 2671 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c
>   create mode 100644 drivers/gpu/drm/amd/pm/inc/amdgpu_dpm_internal.h
>   create mode 100644 drivers/gpu/drm/amd/pm/legacy-dpm/Makefile
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/cik_dpm.h (100%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/kv_dpm.c (99%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/kv_dpm.h (100%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/kv_smc.c (100%)
>   create mode 100644 drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c
>   create mode 100644 drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.h
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/r600_dpm.h (100%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/si_dpm.c (99%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/si_dpm.h (99%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/si_smc.c (100%)
>   rename drivers/gpu/drm/amd/pm/{powerplay => legacy-dpm}/sislands_smc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/amd_powerplay.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/cz_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/fiji_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/hardwaremanager.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/hwmgr.h (99%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/polaris10_pwrvirus.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/power_state.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/pp_debug.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/pp_endian.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/pp_thermal.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/ppinterrupt.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/rv_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu10.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu10_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu11_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu7.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu71.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu71_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu72.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu72_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu73.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu73_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu74.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu74_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu75.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu75_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu7_common.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu7_discrete.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu7_fusion.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu7_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu8.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu8_fusion.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu9.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu9_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu_ucode_xfer_cz.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smu_ucode_xfer_vi.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/smumgr.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/tonga_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/vega10_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/vega12/smu9_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/vega12_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => powerplay}/inc/vega20_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/aldebaran_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/amdgpu_smu.h (98%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/arcturus_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu11_driver_if_arcturus.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu11_driver_if_cyan_skillfish.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu11_driver_if_navi10.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu11_driver_if_sienna_cichlid.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu11_driver_if_vangogh.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu12_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu13_driver_if_aldebaran.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu13_driver_if_yellow_carp.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_11_0_cdr_table.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_types.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_0.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_0_7_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_0_7_pptable.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_0_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_0_pptable.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_5_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_5_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_8_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v11_8_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v12_0.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v12_0_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v13_0.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v13_0_1_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v13_0_1_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{ => swsmu}/inc/smu_v13_0_pptable.h (100%)
>