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

[Alex Deucher <alexdeucher@gmail.com>]

On Fri, Feb 7, 2020 at 3:28 AM Quan, Evan <Evan.Quan@amd.com> wrote:
>
> >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?

At the moment, we haven't had a need to differentiate them, but we
could add a flag.  That said, I don't quite understand why BACO at
runtime is any different from BACO for reset.  They should both
require the same steps to enter and exit.  What is different about
system suspend?

Alex

>
> >-----Original Message-----
> >From: Alex Deucher <alexdeucher@gmail.com>
> >Sent: Thursday, February 6, 2020 10:01 PM
> >To: Quan, Evan <Evan.Quan@amd.com>
> >Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Zhang, Hawking
> ><Hawking.Zhang@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@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@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@lists.freedesktop.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> >> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-
> >gfx&amp;data=02%7C01%7Cev
> >>
> >an.quan%40amd.com%7Ce4212e8faa2849ebda5008d7ab0cfbfd%7C3dd8961fe
> >4884e6
> >>
> >08e11a82d994e183d%7C0%7C0%7C637165944568797358&amp;sdata=M9jaswf
> >JV%2Bq
> >> KLZTxff%2Bju81y47M9WKT2iULEZjHBHcw%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx