[PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting
Lazar, Lijo
lijo.lazar at amd.com
Wed Nov 10 09:39:26 UTC 2021
On 11/9/2021 2:15 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Tuesday, November 9, 2021 12:15 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/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
> [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 ?
>
Yes, that looks fine.
Thanks,
Lijo
> 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);
>>>>> }
>>>>>
More information about the amd-gfx
mailing list