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

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 11/15/2021 8:21 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 sometimes may cause unexpected issues.
>
> Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@xxxxxxx/
>
> Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> Tested-by: Borislav Petkov <bp@xxxxxxx>
> --
> 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)
> v2->v3:
>    - stick to original logics on handling unmentioned IP blocks(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        | 10 ++++++++++
>   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
>   4 files changed, 23 insertions(+), 1 deletion(-)
>
> 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..08362d506534 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:
> @@ -979,6 +986,9 @@ int amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev, uint32_t block
>   		break;
>   	}
>   
> +	if (!ret)
> +		atomic_set(&adev->pm.pwr_state[block_type], pwr_state);
> +

Once the default case is skipped, this will set the state for unhandled 
blocks. Should be fine if such blocks are not expected. Otherwise, just 
return 0 such that they hold unknown power states.

Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>

Thanks,
Lijo
>   	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