Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/9/2021 2:15 PM, Quan, Evan wrote:

[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 ?


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);
    }
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux