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

[Boyuan Zhang <Boyuan.Zhang@amd.com>]

On 2024-10-02 07:19, Christian König wrote:
> 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.


Fixed the format. Also re-worked all PM patches to remove all duplicated 
functions. Please check the new v2 patch set.

Regards,
Boyuan


>
>>         /*
>>        * 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;
>