[PATCH] drm/amd/powerplay: handle features disablement for baco reset in SMU FW

[Quan, Evan]

>Do we need the in_gpu_reset check here?  Is there ever a case where would
>ever want to execute the rest of this?  What if we enable BACO for power
>savings on arcturus?
That is used to rule out the system suspend case.
But yes, it may cause some problem for the runtime suspend case(which uses baco for power saving).
How can we know whether it's a runtime suspend or just system suspend?

>-----Original Message-----
>From: Alex Deucher <alexdeucher at gmail.com>
>Sent: Thursday, February 6, 2020 10:01 PM
>To: Quan, Evan <Evan.Quan at amd.com>
>Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Zhang, Hawking
><Hawking.Zhang at amd.com>
>Subject: Re: [PATCH] drm/amd/powerplay: handle features disablement for
>baco reset in SMU FW
>
>On Thu, Feb 6, 2020 at 3:19 AM Evan Quan <evan.quan at amd.com> wrote:
>>
>> SMU FW will handle the features disablement for baco reset on
>> Arcturus.
>>
>> Change-Id: Ifd87a09de2fca0c67c041afbd5e711973769119a
>> Signed-off-by: Evan Quan <evan.quan at amd.com>
>> ---
>>  drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 53
>> +++++++++++++++++-----
>>  1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> index 9d1075823681..efd10e6fa9ef 100644
>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> @@ -1467,24 +1467,39 @@ void smu_late_fini(void *handle)
>>         smu_send_smc_msg(smu, SMU_MSG_PrepareMp1ForShutdown);  }
>>
>> -static int smu_suspend(void *handle)
>> +static int smu_disabled_dpms(struct smu_context *smu)
>>  {
>> -       int ret;
>> -       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> -       struct smu_context *smu = &adev->smu;
>> -       bool baco_feature_is_enabled = false;
>> +       struct amdgpu_device *adev = smu->adev;
>> +       uint32_t smu_version;
>> +       int ret = 0;
>>
>> -       if (!smu->pm_enabled)
>> -               return 0;
>> +       ret = smu_get_smc_version(smu, NULL, &smu_version);
>> +       if (ret) {
>> +               pr_err("Failed to get smu version.\n");
>> +               return ret;
>> +       }
>>
>> -       if(!smu->is_apu)
>> -               baco_feature_is_enabled = smu_feature_is_enabled(smu,
>SMU_FEATURE_BACO_BIT);
>> +       /*
>> +        * For baco reset on Arcturus, this operation
>> +        * (disable all smu feature) will be handled by SMU FW.
>> +        */
>> +       if (adev->in_gpu_reset &&
>> +           (amdgpu_asic_reset_method(adev) ==
>AMD_RESET_METHOD_BACO) &&
>> +           (adev->asic_type == CHIP_ARCTURUS && smu_version > 0x360e00))
>> +               return 0;
>
>Do we need the in_gpu_reset check here?  Is there ever a case where would
>ever want to execute the rest of this?  What if we enable BACO for power
>savings on arcturus?
>
>>
>> +       /* Disable all enabled SMU features */
>>         ret = smu_system_features_control(smu, false);
>> -       if (ret)
>> +       if (ret) {
>> +               pr_err("Failed to disable smu features.\n");
>>                 return ret;
>> +       }
>>
>> -       if (baco_feature_is_enabled) {
>> +       /* For baco reset, need to leave BACO feature enabled */
>> +       if (adev->in_gpu_reset &&
>> +           (amdgpu_asic_reset_method(adev) ==
>AMD_RESET_METHOD_BACO) &&
>> +           !smu->is_apu &&
>> +           smu_feature_is_enabled(smu, SMU_FEATURE_BACO_BIT)) {
>
>This change will break BACO for power savings on navi1x.  I think we can drop
>this hunk.
>
>>                 ret = smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT,
>true);
>>                 if (ret) {
>>                         pr_warn("set BACO feature enabled failed,
>> return %d\n", ret); @@ -1492,6 +1507,22 @@ static int smu_suspend(void
>*handle)
>>                 }
>>         }
>>
>> +       return ret;
>> +}
>> +
>> +static int smu_suspend(void *handle)
>> +{
>> +       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +       struct smu_context *smu = &adev->smu;
>> +       int ret;
>> +
>> +       if (!smu->pm_enabled)
>> +               return 0;
>> +
>> +       ret = smu_disabled_dpms(smu);
>> +       if (ret)
>> +               return ret;
>> +
>>         smu->watermarks_bitmap &= ~(WATERMARKS_LOADED);
>>
>>         if (adev->asic_type >= CHIP_NAVI10 &&
>> --
>> 2.25.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-
>gfx&data=02%7C01%7Cev
>>
>an.quan%40amd.com%7Ce4212e8faa2849ebda5008d7ab0cfbfd%7C3dd8961fe
>4884e6
>>
>08e11a82d994e183d%7C0%7C0%7C637165944568797358&sdata=M9jaswf
>JV%2Bq
>> KLZTxff%2Bju81y47M9WKT2iULEZjHBHcw%3D&reserved=0