linux-kernel - Re: [PATCH v1 3/3] drm/amdgpu: only send the SMU RLC notification on S3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADnq5_NPwwtrjYaELVvrC+D5vb4_iosCnghud8i9+NO0zC+_qA@mail.gmail.com>
Date: Fri, 24 Oct 2025 12:08:07 -0400
From: Alex Deucher <alexdeucher@...il.com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: Antheas Kapenekakis <lkml@...heas.dev>, Alex Deucher <alexander.deucher@....com>, 
	Shyam Sundar S K <Shyam-sundar.S-k@....com>, Perry Yuan <perry.yuan@....com>, 
	amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v1 3/3] drm/amdgpu: only send the SMU RLC notification on S3

On Fri, Oct 24, 2025 at 11:54 AM Mario Limonciello
<mario.limonciello@....com> wrote:
>
>
>
> On 10/24/2025 10:21 AM, Antheas Kapenekakis wrote:
> > From: Alex Deucher <alexander.deucher@....com>
> >
> > For S0ix, the RLC is not powered down. Rework the Van Gogh logic to
> > skip powering it down and skip part of post-init.
> >
> > Fixes: 8c4e9105b2a8 ("drm/amdgpu: optimize RLC powerdown notification on Vangogh")
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4659
> > Signed-off-by: Alex Deucher <alexander.deucher@....com>
> > Tested-by: Antheas Kapenekakis <lkml@...heas.dev>
> > Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c       | 8 +++++---
> >   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c        | 6 ++++++
> >   drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 3 +++
> >   3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 3d032c4e2dce..220b12d59795 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -5243,9 +5243,11 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
> >       if (amdgpu_sriov_vf(adev))
> >               amdgpu_virt_release_full_gpu(adev, false);
> >
> > -     r = amdgpu_dpm_notify_rlc_state(adev, false);
> > -     if (r)
> > -             return r;
> > +     if (!adev->in_s0ix) {
> > +             r = amdgpu_dpm_notify_rlc_state(adev, false);
> > +             if (r)
> > +                     return r;
> > +     }
>
> Just FYI this is going to clash with my unwind failed suspend series [1].
>
> This is fine, just whichever "lands" first the other will need to rework
> a little bit and I wanted to mention it.
>
> Link:
> https://lore.kernel.org/amd-gfx/20251023165243.317153-2-mario.limonciello@amd.com/
> [1]
>
> This does have me wondering though why amdgpu_dpm_notify_rlc_state() is
> even in amdgpu_device_suspend()?  This is only used on Van Gogh.
> Should we be pushing this deeper into amdgpu_device_ip_suspend_phase2()?
>
> Or should we maybe overhaul this to move the RLC notification into a
> .set_mp1_state callback instead so it's more similar to all the other ASICs?
>

TBH, I don't think this is even required at all here.  The rlc is
stopped in smu_disable_dpms() and we already notify the SMU at that
point.  Notifying it again here seems superfluous.  Would need to test
that removing this one doesn't cause an issue with S3.

> >
> >       return 0;
> >   }
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index fb8086859857..244b8c364d45 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -2040,6 +2040,12 @@ static int smu_disable_dpms(struct smu_context *smu)
> >           smu->is_apu && (amdgpu_in_reset(adev) || adev->in_s0ix))
> >               return 0;
> >
> > +     /* vangogh s0ix */
> > +     if ((amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(11, 5, 0) ||
> > +          amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(11, 5, 2)) &&
> > +         adev->in_s0ix)
> > +             return 0;
> > +
>
> How about for GPU reset, does PMFW handle this too?

I'm not 100% sure.  We need to check with the PMFW team.  I think
vangogh works the same as gfx11 and newer APUs since the s0i3
implementation was more aligned with those chips than RMB.  These
special code paths were added specifically for S3 enablement since the
behavior is different relative to S0ix.

Alex

>
> >       /*
> >        * For gpu reset, runpm and hibernation through BACO,
> >        * BACO feature has to be kept enabled.
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > index 2c9869feba61..0708d0f0938b 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > @@ -2217,6 +2217,9 @@ static int vangogh_post_smu_init(struct smu_context *smu)
> >       uint32_t total_cu = adev->gfx.config.max_cu_per_sh *
> >               adev->gfx.config.max_sh_per_se * adev->gfx.config.max_shader_engines;
> >
> > +     if (adev->in_s0ix)
> > +             return 0;
> > +
> >       /* allow message will be sent after enable message on Vangogh*/
> >       if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_GFXCLK_BIT) &&
> >                       (adev->pg_flags & AMD_PG_SUPPORT_GFX_PG)) {
>