[PATCH V5 00/16] Unified entry point for other blocks to interact with power

Lazar, Lijo lijo.lazar at amd.com
Tue Dec 21 06:24:47 UTC 2021



On 12/13/2021 9:22 AM, Evan Quan wrote:
> 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.
> 
> Evan Quan (16):
>    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: 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
> 

Have some nitpick comments on two patches. With/Without those addressed 
and with the understanding that there will be some follow up patches to 
  address some other comments made, the series is -

	Reviewed-by: Lijo Lazar <lijo.lazar at amd.com>

Thanks,
Lijo

>   drivers/gpu/drm/amd/amdgpu/aldebaran.c        |    2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h           |    7 -
>   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/amdgpu/dce_v10_0.c        |    2 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v11_0.c        |    2 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v6_0.c         |    2 +-
>   drivers/gpu/drm/amd/amdgpu/dce_v8_0.c         |    2 +-
>   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  |  248 +-
>   drivers/gpu/drm/amd/include/amd_shared.h      |    2 -
>   .../gpu/drm/amd/include/kgd_pp_interface.h    |    8 +
>   drivers/gpu/drm/amd/pm/Makefile               |   14 +-
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c           | 2467 ++++++++---------
>   drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c  |   94 +
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c            |  552 ++--
>   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h       |  341 +--
>   .../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    | 1081 ++++++++
>   .../gpu/drm/amd/pm/legacy-dpm/legacy_dpm.h    |   38 +
>   .../amd/pm/{powerplay => legacy-dpm}/ppsmc.h  |    0
>   .../pm/{powerplay => legacy-dpm}/r600_dpm.h   |    0
>   .../amd/pm/{powerplay => legacy-dpm}/si_dpm.c |  163 +-
>   .../amd/pm/{powerplay => legacy-dpm}/si_dpm.h |   15 +-
>   .../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     |   94 +-
>   .../drm/amd/pm/{ => swsmu}/inc/amdgpu_smu.h   |   16 +-
>   .../inc/interface}/smu11_driver_if_arcturus.h |    0
>   .../smu11_driver_if_cyan_skillfish.h          |    0
>   .../inc/interface}/smu11_driver_if_navi10.h   |    0
>   .../smu11_driver_if_sienna_cichlid.h          |    0
>   .../inc/interface}/smu11_driver_if_vangogh.h  |    0
>   .../inc/interface}/smu12_driver_if.h          |    0
>   .../interface}/smu13_driver_if_aldebaran.h    |    0
>   .../interface}/smu13_driver_if_yellow_carp.h  |    0
>   .../inc/interface}/smu_v11_5_pmfw.h           |    0
>   .../inc/interface}/smu_v11_8_pmfw.h           |    0
>   .../inc/interface}/smu_v13_0_1_pmfw.h         |    0
>   .../inc/message}/aldebaran_ppsmc.h            |    0
>   .../inc/message}/arcturus_ppsmc.h             |    0
>   .../inc/message}/smu_v11_0_7_ppsmc.h          |    0
>   .../inc/message}/smu_v11_0_ppsmc.h            |    0
>   .../inc/message}/smu_v11_5_ppsmc.h            |    0
>   .../inc/message}/smu_v11_8_ppsmc.h            |    0
>   .../inc/message}/smu_v12_0_ppsmc.h            |    0
>   .../inc/message}/smu_v13_0_1_ppsmc.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_pptable.h  |    0
>   .../pm/{ => swsmu}/inc/smu_v11_0_pptable.h    |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_v12_0.h    |    0
>   .../drm/amd/pm/{ => swsmu}/inc/smu_v13_0.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 +
>   117 files changed, 3232 insertions(+), 2264 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 (98%)
>   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/amdgpu_smu.h (98%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu11_driver_if_arcturus.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu11_driver_if_cyan_skillfish.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu11_driver_if_navi10.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu11_driver_if_sienna_cichlid.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu11_driver_if_vangogh.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu12_driver_if.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu13_driver_if_aldebaran.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu13_driver_if_yellow_carp.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu_v11_5_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu_v11_8_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/interface}/smu_v13_0_1_pmfw.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/aldebaran_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/arcturus_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v11_0_7_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v11_0_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v11_5_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v11_8_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v12_0_ppsmc.h (100%)
>   rename drivers/gpu/drm/amd/pm/{inc => swsmu/inc/message}/smu_v13_0_1_ppsmc.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_pptable.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_v12_0.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_pptable.h (100%)
> 


More information about the amd-gfx mailing list