Re: [PATCH 02/32] drm/amd/pm: power up or down vcn by instance

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 08.10.24 um 23:15 schrieb boyuan.zhang@amd.com:
> From: Boyuan Zhang <boyuan.zhang@amd.com>
>
> For smu ip with multiple vcn instances (smu 11/13/14), remove all the
> for loop in dpm_set_vcn_enable() functions. And use the instance
> argument to power up/down vcn for the given instance only, instead
> of powering up/down for all vcn instances.
>
> v2: remove all duplicated functions in v1.
>
> remove for-loop from each ip, and temporarily move to dpm_set_vcn_enable,
> in order to keep the exact same logic as before, until further separation
> in the next patch.
>
> Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     |  7 +++-
>   .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c   | 20 +++++------
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    | 16 ++++-----
>   .../gpu/drm/amd/pm/swsmu/smu14/smu_v14_0.c    | 35 ++++++++-----------
>   4 files changed, 35 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 6305fd7d2573..410d5baedfbc 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -237,6 +237,7 @@ static int smu_dpm_set_vcn_enable(struct smu_context *smu,
>   {
>   	struct smu_power_context *smu_power = &smu->smu_power;
>   	struct smu_power_gate *power_gate = &smu_power->power_gate;
> +	struct amdgpu_device *adev = smu->adev;
>   	int ret = 0;
>   
>   	/*
> @@ -251,7 +252,11 @@ static int smu_dpm_set_vcn_enable(struct smu_context *smu,
>   	if (atomic_read(&power_gate->vcn_gated) ^ enable)
>   		return 0;
>   
> -	ret = smu->ppt_funcs->dpm_set_vcn_enable(smu, enable, 0xff);
> +	for (int i = 0; i < adev->vcn.num_vcn_inst; i++) {
> +		ret = smu->ppt_funcs->dpm_set_vcn_enable(smu, enable, i);
> +		if (ret)
> +			return ret;
> +	}
>   	if (!ret)

I think you should be able to drop this if (!ret) now.

Apart from that looks really good,
Christian.

>   		atomic_set(&power_gate->vcn_gated, !enable);
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> index 2438f813d6db..f41d8fd09bce 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> @@ -1157,19 +1157,15 @@ static int sienna_cichlid_dpm_set_vcn_enable(struct smu_context *smu,
>   					      int inst)
>   {
>   	struct amdgpu_device *adev = smu->adev;
> -	int i, ret = 0;
> +	int ret = 0;
>   
> -	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> -		if (adev->vcn.harvest_config & (1 << i))
> -			continue;
> -		/* vcn dpm on is a prerequisite for vcn power gate messages */
> -		if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_MM_DPM_PG_BIT)) {
> -			ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> -							      SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> -							      0x10000 * i, NULL);
> -			if (ret)
> -				return ret;
> -		}
> +	if (adev->vcn.harvest_config & (1 << inst))
> +		return ret;
> +	/* vcn dpm on is a prerequisite for vcn power gate messages */
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_MM_DPM_PG_BIT)) {
> +		ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> +                                            SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> +                                            0x10000 * inst, NULL);
>   	}
>   
>   	return ret;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index 1d37b9e251d4..e57d4926250e 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -2092,18 +2092,14 @@ int smu_v13_0_set_vcn_enable(struct smu_context *smu,
>   			      int inst)
>   {
>   	struct amdgpu_device *adev = smu->adev;
> -	int i, ret = 0;
> +	int ret = 0;
>   
> -	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> -		if (adev->vcn.harvest_config & (1 << i))
> -			continue;
> +	if (adev->vcn.harvest_config & (1 << inst))
> +		return ret;
>   
> -		ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> -						      SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> -						      i << 16U, NULL);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> +					      SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> +					      inst << 16U, NULL);
>   
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0.c
> index 5e1165ecdf7b..20822ac623c7 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0.c
> @@ -1496,29 +1496,24 @@ int smu_v14_0_set_vcn_enable(struct smu_context *smu,
>   			      int inst)
>   {
>   	struct amdgpu_device *adev = smu->adev;
> -	int i, ret = 0;
> +	int ret = 0;
>   
> -	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> -		if (adev->vcn.harvest_config & (1 << i))
> -			continue;
> +	if (adev->vcn.harvest_config & (1 << inst))
> +		return ret;
>   
> -		if (smu->is_apu) {
> -			if (i == 0)
> -				ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> -								      SMU_MSG_PowerUpVcn0 : SMU_MSG_PowerDownVcn0,
> -								      i << 16U, NULL);
> -			else if (i == 1)
> -				ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> -								      SMU_MSG_PowerUpVcn1 : SMU_MSG_PowerDownVcn1,
> -								      i << 16U, NULL);
> -		} else {
> +	if (smu->is_apu) {
> +		if (inst == 0)
>   			ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> -							      SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> -							      i << 16U, NULL);
> -		}
> -
> -		if (ret)
> -			return ret;
> +							      SMU_MSG_PowerUpVcn0 : SMU_MSG_PowerDownVcn0,
> +							      inst << 16U, NULL);
> +		else if (inst == 1)
> +			ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> +							      SMU_MSG_PowerUpVcn1 : SMU_MSG_PowerDownVcn1,
> +							      inst << 16U, NULL);
> +	} else {
> +		ret = smu_cmn_send_smc_msg_with_param(smu, enable ?
> +						      SMU_MSG_PowerUpVcn : SMU_MSG_PowerDownVcn,
> +						      inst << 16U, NULL);
>   	}
>   
>   	return ret;