Re: [PATCH 05/18] drm/amd/pm: set vcn enable by instance

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

Am 02.10.24 um 06:36 schrieb boyuan.zhang@amd.com:
> From: Boyuan Zhang <boyuan.zhang@amd.com>
>
> The new function smu_dpm_set_vcn_enable_instance() will be used to enable
> or disable vcn engine dynamic power for the given vcn instance only.
>
> The original function smu_dpm_set_vcn_enable() will still be used to enable
> or disable vcn engine dynamic power for all VCN instances as before.
>
> Signed-off-by: Boyuan Zhang <boyuan.zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 132 +++++++++++++++---
>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |   2 +-
>   2 files changed, 115 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 939e15b24f86..ddfed7189708 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -232,12 +232,15 @@ static bool is_vcn_enabled(struct amdgpu_device *adev)
>   	return true;
>   }
>   
> -static int smu_dpm_set_vcn_enable(struct smu_context *smu,
> -				  bool enable)
> +static int smu_dpm_set_vcn_enable_instance(struct smu_context *smu,
> +				  bool enable,
> +				  int inst)
>   {
>   	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;
> +	bool single_inst = (adev->vcn.num_vcn_inst == 1);

Please keep the reverse xmas tree order, e.g. defines like ret last.

Apart from that patches #1-'6 are Acked-by: Christian König 
<christian.koenig@amd.com>, but Alex should probably take a look as well 
since I'm not that deep into PM.

Regards,
Christian.

>   
>   	/*
>   	 * don't poweron vcn/jpeg when they are skipped.
> @@ -245,15 +248,33 @@ static int smu_dpm_set_vcn_enable(struct smu_context *smu,
>   	if (!is_vcn_enabled(smu->adev))
>   		return 0;
>   
> -	if (!smu->ppt_funcs->dpm_set_vcn_enable)
> +	if (!smu->ppt_funcs->dpm_set_vcn_enable_instance && !single_inst)
>   		return 0;
>   
> -	if (atomic_read(&power_gate->vcn_gated) ^ enable)
> +	if (!smu->ppt_funcs->dpm_set_vcn_enable && single_inst)
>   		return 0;
>   
> -	ret = smu->ppt_funcs->dpm_set_vcn_enable(smu, enable);
> +	if (atomic_read(&power_gate->vcn_gated[inst]) ^ enable)
> +		return 0;
> +
> +	if (single_inst)
> +		ret = smu->ppt_funcs->dpm_set_vcn_enable(smu, enable);
> +	else
> +		ret = smu->ppt_funcs->dpm_set_vcn_enable_instance(smu, enable, inst);
>   	if (!ret)
> -		atomic_set(&power_gate->vcn_gated, !enable);
> +		atomic_set(&power_gate->vcn_gated[inst], !enable);
> +
> +	return ret;
> +}
> +
> +static int smu_dpm_set_vcn_enable(struct smu_context *smu,
> +				  bool enable)
> +{
> +	struct amdgpu_device *adev = smu->adev;
> +	int ret = 0;
> +
> +	for (int i = 0; i < adev->vcn.num_vcn_inst; i++)
> +		ret = smu_dpm_set_vcn_enable_instance(smu, enable, i);
>   
>   	return ret;
>   }
> @@ -408,6 +429,81 @@ static int smu_dpm_set_power_gate(void *handle,
>   	return ret;
>   }
>   
> +/**
> + * smu_dpm_set_power_gate_instance - power gate/ungate the specific IP block
> + *                                   for the specific instance
> + *
> + * @handle:     smu_context pointer
> + * @block_type: the IP block to power gate/ungate
> + * @gate:       to power gate if true, ungate otherwise
> + * @inst:       the instance to power gate/ungate
> + *
> + * This API uses no smu->mutex lock protection due to:
> + * 1. It is either called by other IP block(gfx/sdma/vcn/uvd/vce).
> + *    This is guarded to be race condition free by the caller.
> + * 2. Or get called on user setting request of power_dpm_force_performance_level.
> + *    Under this case, the smu->mutex lock protection is already enforced on
> + *    the parent API smu_force_performance_level of the call path.
> + */
> +static int smu_dpm_set_power_gate_instance(void *handle,
> +				  uint32_t block_type,
> +				  bool gate,
> +				  int inst)
> +{
> +	struct smu_context *smu = handle;
> +	int ret = 0;
> +
> +	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled) {
> +		dev_WARN(smu->adev->dev,
> +			 "SMU uninitialized but power %s requested for %u!\n",
> +			 gate ? "gate" : "ungate", block_type);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (block_type) {
> +	/*
> +	 * Some legacy code of amdgpu_vcn.c and vcn_v2*.c still uses
> +	 * AMD_IP_BLOCK_TYPE_UVD for VCN. So, here both of them are kept.
> +	 */
> +	case AMD_IP_BLOCK_TYPE_UVD:
> +	case AMD_IP_BLOCK_TYPE_VCN:
> +		ret = smu_dpm_set_vcn_enable_instance(smu, !gate, inst);
> +		if (ret)
> +			dev_err(smu->adev->dev, "Failed to power %s VCN!\n",
> +				gate ? "gate" : "ungate");
> +		break;
> +	case AMD_IP_BLOCK_TYPE_GFX:
> +		ret = smu_gfx_off_control(smu, gate);
> +		if (ret)
> +			dev_err(smu->adev->dev, "Failed to %s gfxoff!\n",
> +				gate ? "enable" : "disable");
> +		break;
> +	case AMD_IP_BLOCK_TYPE_SDMA:
> +		ret = smu_powergate_sdma(smu, gate);
> +		if (ret)
> +			dev_err(smu->adev->dev, "Failed to power %s SDMA!\n",
> +				gate ? "gate" : "ungate");
> +		break;
> +	case AMD_IP_BLOCK_TYPE_JPEG:
> +		ret = smu_dpm_set_jpeg_enable(smu, !gate);
> +		if (ret)
> +			dev_err(smu->adev->dev, "Failed to power %s JPEG!\n",
> +				gate ? "gate" : "ungate");
> +		break;
> +	case AMD_IP_BLOCK_TYPE_VPE:
> +		ret = smu_dpm_set_vpe_enable(smu, !gate);
> +		if (ret)
> +			dev_err(smu->adev->dev, "Failed to power %s VPE!\n",
> +				gate ? "gate" : "ungate");
> +		break;
> +	default:
> +		dev_err(smu->adev->dev, "Unsupported block type!\n");
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>   /**
>    * smu_set_user_clk_dependencies - set user profile clock dependencies
>    *
> @@ -774,19 +870,11 @@ static int smu_early_init(struct amdgpu_ip_block *ip_block)
>   static int smu_set_default_dpm_table(struct smu_context *smu)
>   {
>   	struct amdgpu_device *adev = smu->adev;
> -	struct smu_power_context *smu_power = &smu->smu_power;
> -	struct smu_power_gate *power_gate = &smu_power->power_gate;
> -	int vcn_gate, jpeg_gate;
>   	int ret = 0;
>   
>   	if (!smu->ppt_funcs->set_default_dpm_table)
>   		return 0;
>   
> -	if (adev->pg_flags & AMD_PG_SUPPORT_VCN)
> -		vcn_gate = atomic_read(&power_gate->vcn_gated);
> -	if (adev->pg_flags & AMD_PG_SUPPORT_JPEG)
> -		jpeg_gate = atomic_read(&power_gate->jpeg_gated);
> -
>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN) {
>   		ret = smu_dpm_set_vcn_enable(smu, true);
>   		if (ret)
> @@ -805,10 +893,10 @@ static int smu_set_default_dpm_table(struct smu_context *smu)
>   			"Failed to setup default dpm clock tables!\n");
>   
>   	if (adev->pg_flags & AMD_PG_SUPPORT_JPEG)
> -		smu_dpm_set_jpeg_enable(smu, !jpeg_gate);
> +		smu_dpm_set_jpeg_enable(smu, false);
>   err_out:
>   	if (adev->pg_flags & AMD_PG_SUPPORT_VCN)
> -		smu_dpm_set_vcn_enable(smu, !vcn_gate);
> +		smu_dpm_set_vcn_enable(smu, false);
>   
>   	return ret;
>   }
> @@ -1253,7 +1341,8 @@ static int smu_sw_init(struct amdgpu_ip_block *ip_block)
>   	smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
>   	smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
>   
> -	atomic_set(&smu->smu_power.power_gate.vcn_gated, 1);
> +	for (int i = 0; i < adev->vcn.num_vcn_inst; i++)
> +		atomic_set(&smu->smu_power.power_gate.vcn_gated[i], 1);
>   	atomic_set(&smu->smu_power.power_gate.jpeg_gated, 1);
>   	atomic_set(&smu->smu_power.power_gate.vpe_gated, 1);
>   	atomic_set(&smu->smu_power.power_gate.umsch_mm_gated, 1);
> @@ -2932,6 +3021,7 @@ static int smu_read_sensor(void *handle,
>   			   int *size_arg)
>   {
>   	struct smu_context *smu = handle;
> +	struct amdgpu_device *adev = smu->adev;
>   	struct smu_umd_pstate_table *pstate_table =
>   				&smu->pstate_table;
>   	int ret = 0;
> @@ -2980,7 +3070,13 @@ static int smu_read_sensor(void *handle,
>   		*size = 4;
>   		break;
>   	case AMDGPU_PP_SENSOR_VCN_POWER_STATE:
> -		*(uint32_t *)data = atomic_read(&smu->smu_power.power_gate.vcn_gated) ? 0 : 1;
> +		*(uint32_t *)data = 0;
> +		for (int i = 0; i < adev->vcn.num_vcn_inst; i++) {
> +			if (!atomic_read(&smu->smu_power.power_gate.vcn_gated[i])) {
> +				*(uint32_t *)data = 1;
> +				break;
> +			}
> +		}
>   		*size = 4;
>   		break;
>   	case AMDGPU_PP_SENSOR_MIN_FAN_RPM:
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index f88241cdf9b9..b8b6050877c1 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -399,7 +399,7 @@ struct smu_dpm_context {
>   struct smu_power_gate {
>   	bool uvd_gated;
>   	bool vce_gated;
> -	atomic_t vcn_gated;
> +	atomic_t vcn_gated[AMDGPU_MAX_VCN_INSTANCES];
>   	atomic_t jpeg_gated;
>   	atomic_t vpe_gated;
>   	atomic_t umsch_mm_gated;