[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