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

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

[Evan Quan]

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@amd.com>
Tested-by: Borislav Petkov <bp@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,
+};
+
 struct amd_powerplay {
 	void *pp_handle;
 	const struct amd_pm_funcs *pp_funcs;
+	atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
 };
 
 /* 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);
 }
-- 
2.29.0

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

[Paul Menzel]

Dear Evan,


Am 08.11.21 um 05:47 schrieb Evan Quan:
> 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.

sometime*s*

Please elaborate the kind of issues.

On what systems did you test this? Also, there is a new debug log message.

> Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Tested-by: Borislav Petkov <bp@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,
> +};
> +
>   struct amd_powerplay {
>   	void *pp_handle;
>   	const struct amd_pm_funcs *pp_funcs;
> +	atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
>   };
>   
>   /* 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;

I’d use `unsigned int` or `size_t` to make it clear, that they cannot 
get negative.

>   
>   	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);
>   }
> 

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

[Borislav Petkov]

On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
> Please elaborate the kind of issues.

It fails to reboot on Carrizo-based laptops.

Whoever commits this, pls add

Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic

so that it is clear what the whole story way.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg

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

[Lazar, Lijo]

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@amd.com>
> Tested-by: Borislav Petkov <bp@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.


>   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()

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);
>   }
> 

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

[Christian König]

Am 08.11.21 um 12:15 schrieb Borislav Petkov:
> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
>> Please elaborate the kind of issues.
> It fails to reboot on Carrizo-based laptops.

That doesn't necessary sounds like a good idea to me then.

What exactly is going wrong here? And what is the rational that we must 
fix this by avoiding updating the current state?

See we usually assume that updating to the already set state is 
unproblematic and that here sounds like just trying to mitigated some 
issues instead of fixing the root cause.

Regards,
Christian.

>
> Whoever commits this, pls add
>
> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
>
> so that it is clear what the whole story way.
>
> Thx.
>

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

[Lazar, Lijo]

On 11/8/2021 7:44 PM, Christian König wrote:
> Am 08.11.21 um 12:15 schrieb Borislav Petkov:
>> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
>>> Please elaborate the kind of issues.
>> It fails to reboot on Carrizo-based laptops.
> 
> That doesn't necessary sounds like a good idea to me then.
> 
> What exactly is going wrong here? And what is the rational that we must 
> fix this by avoiding updating the current state?
> 

Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now 
there is an added logic to power gate the IP as part of suspend 
sequence. In case of UVD/VCE, power gating would have already happened 
as part of idle work execution.

In any case, power gating is done by SMU FW. The assumption here is - 
the logic to power gate IP could involve register programming. AFAIK, 
accessing some UVD/VCE regs during powergate state could cause a hang 
unless the anti-hang mechanism is not programmed. That means either FW 
or driver needs to track the state of IP before accessing those regs and 
in this case probably FW is assuming driver to be responsible. i.e., not 
to call power off again if it's already powered down.

Though that seems to be a bad assumption on part of FW, it is still a 
possibility. Haven't seen the actual FW code, it's a very old program.

Thanks,
Lijo

> See we usually assume that updating to the already set state is 
> unproblematic and that here sounds like just trying to mitigated some 
> issues instead of fixing the root cause.
> 
> Regards,
> Christian.
> 
>>
>> Whoever commits this, pls add
>>
>> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
>>
>> so that it is clear what the whole story way.
>>
>> Thx.
>>
> 

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, November 8, 2021 4:51 PM
> To: Quan, Evan <Evan.Quan@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Lazar, Lijo
> <Lijo.Lazar@amd.com>; Borislav Petkov <bp@suse.de>; amd-
> gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> Dear Evan,
> 
> 
> Am 08.11.21 um 05:47 schrieb Evan Quan:
> > 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.
> 
> sometime*s*
> 
> Please elaborate the kind of issues.
> 
> On what systems did you test this? Also, there is a new debug log message.
[Quan, Evan] Locally I verified this on my Polaris12 card and Boris helped to verify on his Carrizo platform. Both show positive results.
The log message was left intentionally. But I downgraded the log level from INFO to DEBUG to avoid confusing.
> 
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.quan@amd.com>
> > Tested-by: Borislav Petkov <bp@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,
> > +};
> > +
> >   struct amd_powerplay {
> >   	void *pp_handle;
> >   	const struct amd_pm_funcs *pp_funcs;
> > +	atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
> >   };
> >
> >   /* 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;
> 
> I’d use `unsigned int` or `size_t` to make it clear, that they cannot get
> negative.
[Quan, Evan] Sure, I can do that.

BR
Evan
> 
> >
> >   	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);
> >   }
> >

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Borislav Petkov <bp@suse.de>
> Sent: Monday, November 8, 2021 7:15 PM
> To: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Quan, Evan <Evan.Quan@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; amd-
> gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
> > Please elaborate the kind of issues.
> 
> It fails to reboot on Carrizo-based laptops.
> 
> Whoever commits this, pls add
> 
> Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fr%2FYV81vidWQLWvATMM%40zn.tnic&amp;data=04%7C01%7
> Cevan.quan%40amd.com%7C274191d3b11c4ba6dc5208d9a2a906d4%7C3dd8
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C637719669118839695%7CUnk
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=JAfoXZHWV6KoCxsSJR4plt5
> P%2FeXzTodZJeAUMPU5kYg%3D&amp;reserved=0
> 
> so that it is clear what the whole story way.
[Quan, Evan] Thanks. Will add this.

BR
Evan
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG
> Nürnberg

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Monday, November 8, 2021 7:16 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
> <bp@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@amd.com>
> > Tested-by: Borislav Petkov <bp@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"?
> 
> 
> >   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);
> >   }
> >

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Monday, November 8, 2021 10:41 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Borislav Petkov
> <bp@suse.de>; Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/8/2021 7:44 PM, Christian König wrote:
> > Am 08.11.21 um 12:15 schrieb Borislav Petkov:
> >> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
> >>> Please elaborate the kind of issues.
> >> It fails to reboot on Carrizo-based laptops.
> >
> > That doesn't necessary sounds like a good idea to me then.
> >
> > What exactly is going wrong here? And what is the rational that we
> > must fix this by avoiding updating the current state?
> >
> 
> Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now there
> is an added logic to power gate the IP as part of suspend sequence. In case of
> UVD/VCE, power gating would have already happened as part of idle work
> execution.
[Quan, Evan] Thanks Lijo. Some supplement for the added-on power gate logic on suspend:
1. if the UVD/VCE is still using on reboot triggered, idle work will be not triggered and the add-on power gate logic
can help to put the Ips into correct gate state.
2. If the UVD/VCE is already idle on reboot triggered, then as Lijo mentioned, the idle work will be already triggered
and the iP is in gate state. Then the add-on power gate logic on suspend will be extra and even harmful(on Carrizo platform).

So, to address the issue of 2 and not break 1, we will track the pg state internally and just bail out on 2nd consecutive gate request.

P.S. gfxoff feature suffers the same issue. The patch can address it also.
 
BR
Evan
> 
> In any case, power gating is done by SMU FW. The assumption here is - the
> logic to power gate IP could involve register programming. AFAIK, accessing
> some UVD/VCE regs during powergate state could cause a hang unless the
> anti-hang mechanism is not programmed. That means either FW or driver
> needs to track the state of IP before accessing those regs and in this case
> probably FW is assuming driver to be responsible. i.e., not to call power off
> again if it's already powered down.
> 
> Though that seems to be a bad assumption on part of FW, it is still a
> possibility. Haven't seen the actual FW code, it's a very old program.
> 
> Thanks,
> Lijo
> 
> > See we usually assume that updating to the already set state is
> > unproblematic and that here sounds like just trying to mitigated some
> > issues instead of fixing the root cause.
> >
> > Regards,
> > Christian.
> >
> >>
> >> Whoever commits this, pls add
> >>
> >> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
> >>
> >> so that it is clear what the whole story way.
> >>
> >> Thx.
> >>
> >

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

[Lazar, Lijo]

On 11/9/2021 9:10 AM, Quan, Evan wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Monday, November 8, 2021 7:16 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
>> <bp@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@amd.com>
>>> Tested-by: Borislav Petkov <bp@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);
>>>    }
>>>

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

[Christian König]

Am 08.11.21 um 15:41 schrieb Lazar, Lijo:
>
>
> On 11/8/2021 7:44 PM, Christian König wrote:
>> Am 08.11.21 um 12:15 schrieb Borislav Petkov:
>>> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
>>>> Please elaborate the kind of issues.
>>> It fails to reboot on Carrizo-based laptops.
>>
>> That doesn't necessary sounds like a good idea to me then.
>>
>> What exactly is going wrong here? And what is the rational that we 
>> must fix this by avoiding updating the current state?
>>
>
> Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now 
> there is an added logic to power gate the IP as part of suspend 
> sequence. In case of UVD/VCE, power gating would have already happened 
> as part of idle work execution.
>
> In any case, power gating is done by SMU FW. The assumption here is - 
> the logic to power gate IP could involve register programming. AFAIK, 
> accessing some UVD/VCE regs during powergate state could cause a hang 
> unless the anti-hang mechanism is not programmed. That means either FW 
> or driver needs to track the state of IP before accessing those regs 
> and in this case probably FW is assuming driver to be responsible. 
> i.e., not to call power off again if it's already powered down.
>
> Though that seems to be a bad assumption on part of FW, it is still a 
> possibility. Haven't seen the actual FW code, it's a very old program.


Ok guys I've double checked the git history and found that this here is 
not as it is intended to be.

See the code in question was just added in August by the following commit:

commit 859e4659273f1df3a23e3990826bcb41e85f68a5
Author: Evan Quan <evan.quan@amd.com>
Date:   Thu Aug 19 12:07:59 2021 +0800

     drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE suspend

     This is a supplement for commit below:
     "drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend".

     Signed-off-by: Evan Quan <evan.quan@amd.com>
     Reviewed-by: Guchun Chen <guchun.chen@amd.com>
     Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Before that we where just not touching the UVD power state at all during 
suspend and so we won't had a problem in the first place.

So what we should do instead is to just revert commit 
859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and 
explanation why that is a bad idea.

Regards,
Christian.


>
> Thanks,
> Lijo
>
>> See we usually assume that updating to the already set state is 
>> unproblematic and that here sounds like just trying to mitigated some 
>> issues instead of fixing the root cause.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Whoever commits this, pls add
>>>
>>> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
>>>
>>> so that it is clear what the whole story way.
>>>
>>> Thx.
>>>
>>

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

[Lazar, Lijo]

On 11/9/2021 12:46 PM, Christian König wrote:
> Am 08.11.21 um 15:41 schrieb Lazar, Lijo:
>>
>>
>> On 11/8/2021 7:44 PM, Christian König wrote:
>>> Am 08.11.21 um 12:15 schrieb Borislav Petkov:
>>>> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
>>>>> Please elaborate the kind of issues.
>>>> It fails to reboot on Carrizo-based laptops.
>>>
>>> That doesn't necessary sounds like a good idea to me then.
>>>
>>> What exactly is going wrong here? And what is the rational that we 
>>> must fix this by avoiding updating the current state?
>>>
>>
>> Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now 
>> there is an added logic to power gate the IP as part of suspend 
>> sequence. In case of UVD/VCE, power gating would have already happened 
>> as part of idle work execution.
>>
>> In any case, power gating is done by SMU FW. The assumption here is - 
>> the logic to power gate IP could involve register programming. AFAIK, 
>> accessing some UVD/VCE regs during powergate state could cause a hang 
>> unless the anti-hang mechanism is not programmed. That means either FW 
>> or driver needs to track the state of IP before accessing those regs 
>> and in this case probably FW is assuming driver to be responsible. 
>> i.e., not to call power off again if it's already powered down.
>>
>> Though that seems to be a bad assumption on part of FW, it is still a 
>> possibility. Haven't seen the actual FW code, it's a very old program.
> 
> 
> Ok guys I've double checked the git history and found that this here is 
> not as it is intended to be.
> 
> See the code in question was just added in August by the following commit:
> 
> commit 859e4659273f1df3a23e3990826bcb41e85f68a5
> Author: Evan Quan <evan.quan@amd.com>
> Date:   Thu Aug 19 12:07:59 2021 +0800
> 
>      drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE suspend
> 
>      This is a supplement for commit below:
>      "drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend".
> 
>      Signed-off-by: Evan Quan <evan.quan@amd.com>
>      Reviewed-by: Guchun Chen <guchun.chen@amd.com>
>      Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> 
> Before that we where just not touching the UVD power state at all during 
> suspend and so we won't had a problem in the first place.
> 
> So what we should do instead is to just revert commit 
> 859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and 
> explanation why that is a bad idea.
> 

Yeah, right. If I remember correctly, this commit was originally added 
to fix hangs with S3 suspend/resume cycles triggered during video 
playback. Reverting could bring back that one. Evan will know more 
background details.

Thanks,
Lijo

> Regards,
> Christian.
> 
> 
>>
>> Thanks,
>> Lijo
>>
>>> See we usually assume that updating to the already set state is 
>>> unproblematic and that here sounds like just trying to mitigated some 
>>> issues instead of fixing the root cause.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Whoever commits this, pls add
>>>>
>>>> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
>>>>
>>>> so that it is clear what the whole story way.
>>>>
>>>> Thx.
>>>>
>>>
> 

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

[Christian König]

Am 09.11.21 um 08:28 schrieb Lazar, Lijo:
> [SNIP]
>>
>> Ok guys I've double checked the git history and found that this here 
>> is not as it is intended to be.
>>
>> See the code in question was just added in August by the following 
>> commit:
>>
>> commit 859e4659273f1df3a23e3990826bcb41e85f68a5
>> Author: Evan Quan <evan.quan@amd.com>
>> Date:   Thu Aug 19 12:07:59 2021 +0800
>>
>>      drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE suspend
>>
>>      This is a supplement for commit below:
>>      "drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on 
>> suspend".
>>
>>      Signed-off-by: Evan Quan <evan.quan@amd.com>
>>      Reviewed-by: Guchun Chen <guchun.chen@amd.com>
>>      Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> Before that we where just not touching the UVD power state at all 
>> during suspend and so we won't had a problem in the first place.
>>
>> So what we should do instead is to just revert commit 
>> 859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and 
>> explanation why that is a bad idea.
>>
>
> Yeah, right. If I remember correctly, this commit was originally added 
> to fix hangs with S3 suspend/resume cycles triggered during video 
> playback. Reverting could bring back that one. Evan will know more 
> background details.

Exactly that's my memory as well. So what happens here is that we go in 
circles with one patch fixing a bug and the next patch effectively 
reverting it again to fix another bug.

What we need to do is stop this madness and take a look at the 
underlying problems instead of trying to work around them.

Thanks,
Christian.

>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> See we usually assume that updating to the already set state is 
>>>> unproblematic and that here sounds like just trying to mitigated 
>>>> some issues instead of fixing the root cause.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Whoever commits this, pls add
>>>>>
>>>>> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
>>>>>
>>>>> so that it is clear what the whole story way.
>>>>>
>>>>> Thx.
>>>>>
>>>>
>>

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Tuesday, November 9, 2021 12:15 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
> <bp@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@amd.com>
> >> Sent: Monday, November 8, 2021 7:16 PM
> >> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
> >> <bp@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@amd.com>
> >>> Tested-by: Borislav Petkov <bp@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 ?

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);
> >>>    }
> >>>

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Tuesday, November 9, 2021 3:29 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Borislav Petkov
> <bp@suse.de>; Paul Menzel <pmenzel@molgen.mpg.de>; Liu, Leo
> <Leo.Liu@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/9/2021 12:46 PM, Christian König wrote:
> > Am 08.11.21 um 15:41 schrieb Lazar, Lijo:
> >>
> >>
> >> On 11/8/2021 7:44 PM, Christian König wrote:
> >>> Am 08.11.21 um 12:15 schrieb Borislav Petkov:
> >>>> On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
> >>>>> Please elaborate the kind of issues.
> >>>> It fails to reboot on Carrizo-based laptops.
> >>>
> >>> That doesn't necessary sounds like a good idea to me then.
> >>>
> >>> What exactly is going wrong here? And what is the rational that we
> >>> must fix this by avoiding updating the current state?
> >>>
> >>
> >> Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now
> >> there is an added logic to power gate the IP as part of suspend
> >> sequence. In case of UVD/VCE, power gating would have already
> >> happened as part of idle work execution.
> >>
> >> In any case, power gating is done by SMU FW. The assumption here is -
> >> the logic to power gate IP could involve register programming. AFAIK,
> >> accessing some UVD/VCE regs during powergate state could cause a hang
> >> unless the anti-hang mechanism is not programmed. That means either
> >> FW or driver needs to track the state of IP before accessing those
> >> regs and in this case probably FW is assuming driver to be responsible.
> >> i.e., not to call power off again if it's already powered down.
> >>
> >> Though that seems to be a bad assumption on part of FW, it is still a
> >> possibility. Haven't seen the actual FW code, it's a very old program.
> >
> >
> > Ok guys I've double checked the git history and found that this here
> > is not as it is intended to be.
> >
> > See the code in question was just added in August by the following commit:
> >
> > commit 859e4659273f1df3a23e3990826bcb41e85f68a5
> > Author: Evan Quan <evan.quan@amd.com>
> > Date:   Thu Aug 19 12:07:59 2021 +0800
> >
> >      drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE
> > suspend
> >
> >      This is a supplement for commit below:
> >      "drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend".
> >
> >      Signed-off-by: Evan Quan <evan.quan@amd.com>
> >      Reviewed-by: Guchun Chen <guchun.chen@amd.com>
> >      Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > Before that we where just not touching the UVD power state at all
> > during suspend and so we won't had a problem in the first place.
> >
> > So what we should do instead is to just revert commit
> > 859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and
> > explanation why that is a bad idea.
> >
> 
> Yeah, right. If I remember correctly, this commit was originally added
> to fix hangs with S3 suspend/resume cycles triggered during video
> playback. Reverting could bring back that one. Evan will know more
> background details.
[Quan, Evan] Yes, 859e4659273f1df3a23e3990826bcb41e85f68a5 aimed the issue below. It cannot be reverted.
"If the UVD/VCE is still using on reboot/suspend triggered, idle work will be not triggered and the add-on power gate logic can help to put the Ips into correct gate state."

BR
Evan
> 
> Thanks,
> Lijo
> 
> > Regards,
> > Christian.
> >
> >
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> See we usually assume that updating to the already set state is
> >>> unproblematic and that here sounds like just trying to mitigated some
> >>> issues instead of fixing the root cause.
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>>
> >>>> Whoever commits this, pls add
> >>>>
> >>>> Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@zn.tnic
> >>>>
> >>>> so that it is clear what the whole story way.
> >>>>
> >>>> Thx.
> >>>>
> >>>
> >

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

[Lazar, Lijo]

On 11/9/2021 2:15 PM, Quan, Evan wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Tuesday, November 9, 2021 12:15 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
>> <bp@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@amd.com>
>>>> Sent: Monday, November 8, 2021 7:16 PM
>>>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
>>>> <bp@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@amd.com>
>>>>> Tested-by: Borislav Petkov <bp@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);
>>>>>     }
>>>>>

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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Friday, November 12, 2021 4:46 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Borislav Petkov
> <bp@suse.de>
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/11/2021 1:27 PM, 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
> > sometimes may cause unexpected issues.
> >
> > Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@zn.tnic/
> >
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.quan@amd.com>
> > Tested-by: Borislav Petkov <bp@suse.de>
> > --
> > v1->v2:
> >    - typo fix and add link for the issue referred in commit
> >      message(Paul/Boris)
> >    - update the data type to uint32_t(Paul)
> >    - better Macro naming(Lijo)
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +++
> >   drivers/gpu/drm/amd/include/amd_shared.h   |  3 ++-
> >   drivers/gpu/drm/amd/pm/amdgpu_dpm.c        | 12 +++++++++++-
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
> >   4 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 0bd90ec9e43e..fca592394fa1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3508,6 +3508,9 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
> >   		adev->rmmio_size = pci_resource_len(adev->pdev, 2);
> >   	}
> >
> > +	for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > +		atomic_set(&adev->pm.pwr_state[i],
> POWER_STATE_UNKNOWN);
> > +
> >   	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
> >   	if (adev->rmmio == NULL) {
> >   		return -ENOMEM;
> > 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..40fda199a86f 100644
> > --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > @@ -927,6 +927,13 @@ 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 ip_power_state pwr_state = gate ? POWER_STATE_OFF :
> > +POWER_STATE_ON;
> > +
> > +	if (atomic_read(&adev->pm.pwr_state[block_type]) == pwr_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 +983,12 @@ int
> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> uint32_t block
> >   		}
> >   		break;
> >   	default:
> > -		break;
> > +		return -EINVAL;
> 
> IP blocks which were not handled by PMFW were default pass before. Will
> this be a problem or return 0 here to keep the legacy behavior?
[Quan, Evan] Although I do not think there will be a problem with such change(considering there is no such IP block). I will update the patch
to stick to its original logic.

Thanks,
Evan
> 
> Thanks,
> Lijo
> 
> >   	}
> >
> > +	if (!ret)
> > +		atomic_set(&adev->pm.pwr_state[block_type], pwr_state);
> > +
> >   	return ret;
> >   }
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > index 98f1b3d8c1d5..16e3f72d31b9 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > @@ -417,6 +417,12 @@ struct amdgpu_dpm {
> >   	enum amd_dpm_forced_level forced_level;
> >   };
> >
> > +enum ip_power_state {
> > +	POWER_STATE_UNKNOWN,
> > +	POWER_STATE_ON,
> > +	POWER_STATE_OFF,
> > +};
> > +
> >   struct amdgpu_pm {
> >   	struct mutex		mutex;
> >   	u32                     current_sclk;
> > @@ -452,6 +458,8 @@ struct amdgpu_pm {
> >   	struct i2c_adapter smu_i2c;
> >   	struct mutex		smu_i2c_mutex;
> >   	struct list_head	pm_attr_list;
> > +
> > +	atomic_t		pwr_state[AMD_IP_BLOCK_TYPE_NUM];
> >   };
> >
> >   #define R600_SSTU_DFLT                               0
> >

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

[Lazar, Lijo]

On 11/11/2021 1:27 PM, 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 sometimes may cause unexpected issues.
> 
> Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@zn.tnic/
> 
> Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Tested-by: Borislav Petkov <bp@suse.de>
> --
> v1->v2:
>    - typo fix and add link for the issue referred in commit
>      message(Paul/Boris)
>    - update the data type to uint32_t(Paul)
>    - better Macro naming(Lijo)
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +++
>   drivers/gpu/drm/amd/include/amd_shared.h   |  3 ++-
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c        | 12 +++++++++++-
>   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
>   4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 0bd90ec9e43e..fca592394fa1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3508,6 +3508,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   		adev->rmmio_size = pci_resource_len(adev->pdev, 2);
>   	}
>   
> +	for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> +		atomic_set(&adev->pm.pwr_state[i], POWER_STATE_UNKNOWN);
> +
>   	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
>   	if (adev->rmmio == NULL) {
>   		return -ENOMEM;
> 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..40fda199a86f 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -927,6 +927,13 @@ 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 ip_power_state pwr_state = gate ? POWER_STATE_OFF : POWER_STATE_ON;
> +
> +	if (atomic_read(&adev->pm.pwr_state[block_type]) == pwr_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 +983,12 @@ int amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev, uint32_t block
>   		}
>   		break;
>   	default:
> -		break;
> +		return -EINVAL;

IP blocks which were not handled by PMFW were default pass before. Will 
this be a problem or return 0 here to keep the legacy behavior?

Thanks,
Lijo

>   	}
>   
> +	if (!ret)
> +		atomic_set(&adev->pm.pwr_state[block_type], pwr_state);
> +
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> index 98f1b3d8c1d5..16e3f72d31b9 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> @@ -417,6 +417,12 @@ struct amdgpu_dpm {
>   	enum amd_dpm_forced_level forced_level;
>   };
>   
> +enum ip_power_state {
> +	POWER_STATE_UNKNOWN,
> +	POWER_STATE_ON,
> +	POWER_STATE_OFF,
> +};
> +
>   struct amdgpu_pm {
>   	struct mutex		mutex;
>   	u32                     current_sclk;
> @@ -452,6 +458,8 @@ struct amdgpu_pm {
>   	struct i2c_adapter smu_i2c;
>   	struct mutex		smu_i2c_mutex;
>   	struct list_head	pm_attr_list;
> +
> +	atomic_t		pwr_state[AMD_IP_BLOCK_TYPE_NUM];
>   };
>   
>   #define R600_SSTU_DFLT                               0
> 

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

[Evan Quan]

Just bail out if the target IP block is already in the desired
powergate/ungate state. This can avoid some duplicate settings
which sometimes may cause unexpected issues.

Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@zn.tnic/

Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
Signed-off-by: Evan Quan <evan.quan@amd.com>
Tested-by: Borislav Petkov <bp@suse.de>
--
v1->v2:
  - typo fix and add link for the issue referred in commit
    message(Paul/Boris)
  - update the data type to uint32_t(Paul)
  - better Macro naming(Lijo)
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +++
 drivers/gpu/drm/amd/include/amd_shared.h   |  3 ++-
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c        | 12 +++++++++++-
 drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0bd90ec9e43e..fca592394fa1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3508,6 +3508,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		adev->rmmio_size = pci_resource_len(adev->pdev, 2);
 	}
 
+	for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
+		atomic_set(&adev->pm.pwr_state[i], POWER_STATE_UNKNOWN);
+
 	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
 	if (adev->rmmio == NULL) {
 		return -ENOMEM;
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..40fda199a86f 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
@@ -927,6 +927,13 @@ 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 ip_power_state pwr_state = gate ? POWER_STATE_OFF : POWER_STATE_ON;
+
+	if (atomic_read(&adev->pm.pwr_state[block_type]) == pwr_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 +983,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->pm.pwr_state[block_type], pwr_state);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
index 98f1b3d8c1d5..16e3f72d31b9 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
@@ -417,6 +417,12 @@ struct amdgpu_dpm {
 	enum amd_dpm_forced_level forced_level;
 };
 
+enum ip_power_state {
+	POWER_STATE_UNKNOWN,
+	POWER_STATE_ON,
+	POWER_STATE_OFF,
+};
+
 struct amdgpu_pm {
 	struct mutex		mutex;
 	u32                     current_sclk;
@@ -452,6 +458,8 @@ struct amdgpu_pm {
 	struct i2c_adapter smu_i2c;
 	struct mutex		smu_i2c_mutex;
 	struct list_head	pm_attr_list;
+
+	atomic_t		pwr_state[AMD_IP_BLOCK_TYPE_NUM];
 };
 
 #define R600_SSTU_DFLT                               0
-- 
2.29.0