[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Tuesday, November 9, 2021 12:15 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Borislav Petkov
> <bp@xxxxxxx>
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
>
>
>
> On 11/9/2021 9:10 AM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Monday, November 8, 2021 7:16 PM
> >> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Borislav Petkov
> >> <bp@xxxxxxx>
> >> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> >> setting
> >>
> >>
> >>
> >> On 11/8/2021 10:17 AM, Evan Quan wrote:
> >>> Just bail out if the target IP block is already in the desired
> >>> powergate/ungate state. This can avoid some duplicate settings which
> >>> sometime may cause unexpected issues.
> >>>
> >>> Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> >>> Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> >>> Tested-by: Borislav Petkov <bp@xxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 7 +++++++
> >>> drivers/gpu/drm/amd/include/amd_shared.h | 3 ++-
> >>> drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 13 ++++++++++++-
> >>> drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c | 3 +++
> >>> drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c | 3 +++
> >>> drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 3 +++
> >>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 +++
> >>> 7 files changed, 33 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index b85b67a88a3d..19fa21ad8a44 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -767,9 +767,16 @@ enum amd_hw_ip_block_type {
> >>> #define HW_ID_MAX 300
> >>> #define IP_VERSION(mj, mn, rv) (((mj) << 16) | ((mn) << 8) |
> >>> (rv))
> >>>
> >>> +enum amd_ip_powergate_state {
> >>> + POWERGATE_STATE_INITIAL,
> >>> + POWERGATE_STATE_GATE,
> >>> + POWERGATE_STATE_UNGATE,
> >>> +};
> >>> +
> >>
> >> To reflect the current state, they could be named like
> >> POWERGATE_STATE_UNKNOWN/ON/OFF.
> > [Quan, Evan] This may be more confusing. POWERGATE_STATE_ON means
> "gfx on" or "gate on which means gfx off"?
>
> What I meant is -
> PG_STATE_ON - Power gated
> PG_STATE_OFF - Not power gated
[Quan, Evan] Yeah, but I mean other user may be confusing about these. Since when we take about the PG state, we usually use "Gate" or "Ungate". How about POWER_STATE_ON - Power on/ungate
POWER_STATE_OFF - Power off/gate ?
BR
Evan
> Thanks,
> Lijo
>
> >>
> >>
> >>> struct amd_powerplay {
> >>> void *pp_handle;
> >>> const struct amd_pm_funcs *pp_funcs;
> >>> + atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
> >>
> >> Maybe add another field in amdgpu_ip_block_status? Downside is it
> >> won't reflect the PG state achieved through paths other than PMFW
> >> control and ipblock needs to be queried through
> >> amdgpu_device_ip_get_ip_block()
> > [Quan, Evan] Yes, we will need to track pg state other than PMFW control
> then.
> > That will need extra effort and seems unnecessary considering there is no
> such use case(need to know the PG state out of PMFW control).
> >
> > BR
> > Evan
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> };
> >>>
> >>> /* polaris10 kickers */
> >>> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> >> b/drivers/gpu/drm/amd/include/amd_shared.h
> >>> index f1a46d16f7ea..4b9e68a79f06 100644
> >>> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> >>> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> >>> @@ -98,7 +98,8 @@ enum amd_ip_block_type {
> >>> AMD_IP_BLOCK_TYPE_ACP,
> >>> AMD_IP_BLOCK_TYPE_VCN,
> >>> AMD_IP_BLOCK_TYPE_MES,
> >>> - AMD_IP_BLOCK_TYPE_JPEG
> >>> + AMD_IP_BLOCK_TYPE_JPEG,
> >>> + AMD_IP_BLOCK_TYPE_NUM,
> >>> };
> >>>
> >>> enum amd_clockgating_state {
> >>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >> b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> index 03581d5b1836..a6b337b6f4a9 100644
> >>> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> @@ -927,6 +927,14 @@ int
> >> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> >> uint32_t block
> >>> {
> >>> int ret = 0;
> >>> const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> >>> + enum amd_ip_powergate_state pg_state =
> >>> + gate ? POWERGATE_STATE_GATE :
> >> POWERGATE_STATE_UNGATE;
> >>> +
> >>> + if (atomic_read(&adev->powerplay.pg_state[block_type]) ==
> >> pg_state) {
> >>> + dev_dbg(adev->dev, "IP block%d already in the target %s
> >> state!",
> >>> + block_type, gate ? "gate" : "ungate");
> >>> + return 0;
> >>> + }
> >>>
> >>> switch (block_type) {
> >>> case AMD_IP_BLOCK_TYPE_UVD:
> >>> @@ -976,9 +984,12 @@ int
> >> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> >> uint32_t block
> >>> }
> >>> break;
> >>> default:
> >>> - break;
> >>> + return -EINVAL;
> >>> }
> >>>
> >>> + if (!ret)
> >>> + atomic_set(&adev->powerplay.pg_state[block_type],
> >> pg_state);
> >>> +
> >>> return ret;
> >>> }
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> >> b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> >>> index bba7479f6265..8d8a7cf615eb 100644
> >>> --- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> >>> +++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> >>> @@ -38,6 +38,7 @@ static const struct amd_pm_funcs pp_dpm_funcs;
> >>> static int amd_powerplay_create(struct amdgpu_device *adev)
> >>> {
> >>> struct pp_hwmgr *hwmgr;
> >>> + int i;
> >>>
> >>> if (adev == NULL)
> >>> return -EINVAL;
> >>> @@ -57,6 +58,8 @@ static int amd_powerplay_create(struct
> >> amdgpu_device *adev)
> >>> hwmgr->display_config = &adev->pm.pm_display_cfg;
> >>> adev->powerplay.pp_handle = hwmgr;
> >>> adev->powerplay.pp_funcs = &pp_dpm_funcs;
> >>> + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> >>> + atomic_set(&adev->powerplay.pg_state[i],
> >> POWERGATE_STATE_INITIAL);
> >>> return 0;
> >>> }
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> >> b/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> >>> index bcae42cef374..f84f56552fd0 100644
> >>> --- a/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> >>> @@ -2965,9 +2965,12 @@ static int kv_dpm_get_temp(void *handle)
> >>> static int kv_dpm_early_init(void *handle)
> >>> {
> >>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>> + int i;
> >>>
> >>> adev->powerplay.pp_funcs = &kv_dpm_funcs;
> >>> adev->powerplay.pp_handle = adev;
> >>> + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> >>> + atomic_set(&adev->powerplay.pg_state[i],
> >> POWERGATE_STATE_INITIAL);
> >>> kv_dpm_set_irq_funcs(adev);
> >>>
> >>> return 0;
> >>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> >> b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> >>> index 81f82aa05ec2..f1eb22c9bb59 100644
> >>> --- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> >>> @@ -7916,9 +7916,12 @@ static int si_dpm_early_init(void *handle)
> >>> {
> >>>
> >>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>> + int i;
> >>>
> >>> adev->powerplay.pp_funcs = &si_dpm_funcs;
> >>> adev->powerplay.pp_handle = adev;
> >>> + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> >>> + atomic_set(&adev->powerplay.pg_state[i],
> >> POWERGATE_STATE_INITIAL);
> >>> si_dpm_set_irq_funcs(adev);
> >>> return 0;
> >>> }
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> index 01168b8955bf..fdc25bae8237 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> @@ -627,6 +627,7 @@ static int smu_early_init(void *handle)
> >>> {
> >>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>> struct smu_context *smu = &adev->smu;
> >>> + int i;
> >>>
> >>> smu->adev = adev;
> >>> smu->pm_enabled = !!amdgpu_dpm;
> >>> @@ -639,6 +640,8 @@ static int smu_early_init(void *handle)
> >>>
> >>> adev->powerplay.pp_handle = smu;
> >>> adev->powerplay.pp_funcs = &swsmu_pm_funcs;
> >>> + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> >>> + atomic_set(&adev->powerplay.pg_state[i],
> >> POWERGATE_STATE_INITIAL);
> >>>
> >>> return smu_set_funcs(adev);
> >>> }
> >>>
