[PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting
Lazar, Lijo
lijo.lazar at amd.com
Tue Nov 9 04:15:21 UTC 2021
On 11/9/2021 9:10 AM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Monday, November 8, 2021 7:16 PM
>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Borislav Petkov
>> <bp at suse.de>
>> 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 at amd.com>
>>> Tested-by: Borislav Petkov <bp at suse.de>
>>> ---
>>> 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
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);
>>> }
>>>
More information about the amd-gfx
mailing list