[PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Rajib Mahapatra]

[Why]
SDMA ring buffer test failed if suspend is aborted during
S0i3 resume.

[How]
If suspend is aborted for some reason during S0i3 resume
cycle, it follows SDMA ring test failing and errors in amdgpu
resume. For RN/CZN/Picasso, SMU saves and restores SDMA
registers during S0ix cycle. So, skipping SDMA suspend and
resume from driver solves the issue. This time, the system
is able to resume gracefully even the suspend is aborted.

v2: add changes on sdma_v4, skipping SDMA hw_init and hw_fini.
Signed-off-by: Rajib Mahapatra <rajib.mahapatra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 06a7ceda4c87..02115d63b071 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -2058,6 +2058,10 @@ static int sdma_v4_0_suspend(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	/* SMU saves SDMA state for us */
+	if (adev->in_s0ix)
+		return 0;
+
 	return sdma_v4_0_hw_fini(adev);
 }
 
@@ -2065,6 +2069,10 @@ static int sdma_v4_0_resume(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	/* SMU restores SDMA state for us */
+	if (adev->in_s0ix)
+		return 0;
+
 	return sdma_v4_0_hw_init(adev);
 }
 
-- 
2.25.1

RE: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> Sent: Thursday, February 10, 2022 07:35
> To: Liang, Prike <Prike.Liang@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>;
> Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> Subject: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.
> 
> [Why]
> SDMA ring buffer test failed if suspend is aborted during
> S0i3 resume.
> 
> [How]
> If suspend is aborted for some reason during S0i3 resume
> cycle, it follows SDMA ring test failing and errors in amdgpu
> resume. For RN/CZN/Picasso, SMU saves and restores SDMA
> registers during S0ix cycle. So, skipping SDMA suspend and
> resume from driver solves the issue. This time, the system
> is able to resume gracefully even the suspend is aborted.
> 
> v2: add changes on sdma_v4, skipping SDMA hw_init and hw_fini.

This line in the commit message should be "below" the ---

Besides that the code is better.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> Signed-off-by: Rajib Mahapatra <rajib.mahapatra@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 06a7ceda4c87..02115d63b071 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -2058,6 +2058,10 @@ static int sdma_v4_0_suspend(void *handle)
>  {
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> 
> +	/* SMU saves SDMA state for us */
> +	if (adev->in_s0ix)
> +		return 0;
> +
>  	return sdma_v4_0_hw_fini(adev);
>  }
> 
> @@ -2065,6 +2069,10 @@ static int sdma_v4_0_resume(void *handle)
>  {
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> 
> +	/* SMU restores SDMA state for us */
> +	if (adev->in_s0ix)
> +		return 0;
> +
>  	return sdma_v4_0_hw_init(adev);
>  }
> 
> --
> 2.25.1

Re: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Alex Deucher]

On Thu, Feb 10, 2022 at 9:04 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > Sent: Thursday, February 10, 2022 07:35
> > To: Liang, Prike <Prike.Liang@amd.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>;
> > Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > Subject: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.
> >
> > [Why]
> > SDMA ring buffer test failed if suspend is aborted during
> > S0i3 resume.
> >
> > [How]
> > If suspend is aborted for some reason during S0i3 resume
> > cycle, it follows SDMA ring test failing and errors in amdgpu
> > resume. For RN/CZN/Picasso, SMU saves and restores SDMA
> > registers during S0ix cycle. So, skipping SDMA suspend and
> > resume from driver solves the issue. This time, the system
> > is able to resume gracefully even the suspend is aborted.
> >
> > v2: add changes on sdma_v4, skipping SDMA hw_init and hw_fini.
>
> This line in the commit message should be "below" the ---
>
> Besides that the code is better.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

I presume sdma_v5.2.c needs a similar fix?

Alex


>
> > Signed-off-by: Rajib Mahapatra <rajib.mahapatra@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > index 06a7ceda4c87..02115d63b071 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > @@ -2058,6 +2058,10 @@ static int sdma_v4_0_suspend(void *handle)
> >  {
> >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >
> > +     /* SMU saves SDMA state for us */
> > +     if (adev->in_s0ix)
> > +             return 0;
> > +
> >       return sdma_v4_0_hw_fini(adev);
> >  }
> >
> > @@ -2065,6 +2069,10 @@ static int sdma_v4_0_resume(void *handle)
> >  {
> >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >
> > +     /* SMU restores SDMA state for us */
> > +     if (adev->in_s0ix)
> > +             return 0;
> > +
> >       return sdma_v4_0_hw_init(adev);
> >  }
> >
> > --
> > 2.25.1

RE: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Thursday, February 10, 2022 09:28
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>; Liang, Prike
> <Prike.Liang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>;
> amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>
> Subject: Re: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for
> S0ix.
> 
> On Thu, Feb 10, 2022 at 9:04 AM Limonciello, Mario
> <Mario.Limonciello@amd.com> wrote:
> >
> > [Public]
> >
> >
> >
> > > -----Original Message-----
> > > From: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > > Sent: Thursday, February 10, 2022 07:35
> > > To: Liang, Prike <Prike.Liang@amd.com>; Limonciello, Mario
> > > <Mario.Limonciello@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>
> > > Cc: amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>;
> > > Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > > Subject: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for
> S0ix.
> > >
> > > [Why]
> > > SDMA ring buffer test failed if suspend is aborted during
> > > S0i3 resume.
> > >
> > > [How]
> > > If suspend is aborted for some reason during S0i3 resume
> > > cycle, it follows SDMA ring test failing and errors in amdgpu
> > > resume. For RN/CZN/Picasso, SMU saves and restores SDMA
> > > registers during S0ix cycle. So, skipping SDMA suspend and
> > > resume from driver solves the issue. This time, the system
> > > is able to resume gracefully even the suspend is aborted.
> > >
> > > v2: add changes on sdma_v4, skipping SDMA hw_init and hw_fini.
> >
> > This line in the commit message should be "below" the ---
> >
> > Besides that the code is better.
> >
> > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> I presume sdma_v5.2.c needs a similar fix?

VG doesn't do s0i3 right?
No, YC should not take a similar fix.    YC had an architectural change and to 
avoid a "similar" problem takes 26db706a6d77b9e184feb11725e97e53b7a89519.

> 
> Alex
> 
> 
> >
> > > Signed-off-by: Rajib Mahapatra <rajib.mahapatra@amd.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > index 06a7ceda4c87..02115d63b071 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > @@ -2058,6 +2058,10 @@ static int sdma_v4_0_suspend(void *handle)
> > >  {
> > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > >
> > > +     /* SMU saves SDMA state for us */
> > > +     if (adev->in_s0ix)
> > > +             return 0;
> > > +
> > >       return sdma_v4_0_hw_fini(adev);
> > >  }
> > >
> > > @@ -2065,6 +2069,10 @@ static int sdma_v4_0_resume(void *handle)
> > >  {
> > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > >
> > > +     /* SMU restores SDMA state for us */
> > > +     if (adev->in_s0ix)
> > > +             return 0;
> > > +
> > >       return sdma_v4_0_hw_init(adev);
> > >  }
> > >
> > > --
> > > 2.25.1

Re: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Alex Deucher]

On Thu, Feb 10, 2022 at 10:42 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com>
> > Sent: Thursday, February 10, 2022 09:28
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>; Liang, Prike
> > <Prike.Liang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>;
> > amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>
> > Subject: Re: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for
> > S0ix.
> >
> > On Thu, Feb 10, 2022 at 9:04 AM Limonciello, Mario
> > <Mario.Limonciello@amd.com> wrote:
> > >
> > > [Public]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > > > Sent: Thursday, February 10, 2022 07:35
> > > > To: Liang, Prike <Prike.Liang@amd.com>; Limonciello, Mario
> > > > <Mario.Limonciello@amd.com>; Deucher, Alexander
> > > > <Alexander.Deucher@amd.com>
> > > > Cc: amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>;
> > > > Mahapatra, Rajib <Rajib.Mahapatra@amd.com>
> > > > Subject: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for
> > S0ix.
> > > >
> > > > [Why]
> > > > SDMA ring buffer test failed if suspend is aborted during
> > > > S0i3 resume.
> > > >
> > > > [How]
> > > > If suspend is aborted for some reason during S0i3 resume
> > > > cycle, it follows SDMA ring test failing and errors in amdgpu
> > > > resume. For RN/CZN/Picasso, SMU saves and restores SDMA
> > > > registers during S0ix cycle. So, skipping SDMA suspend and
> > > > resume from driver solves the issue. This time, the system
> > > > is able to resume gracefully even the suspend is aborted.
> > > >
> > > > v2: add changes on sdma_v4, skipping SDMA hw_init and hw_fini.
> > >
> > > This line in the commit message should be "below" the ---
> > >
> > > Besides that the code is better.
> > >
> > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> >
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > I presume sdma_v5.2.c needs a similar fix?
>
> VG doesn't do s0i3 right?

Right.

> No, YC should not take a similar fix.    YC had an architectural change and to
> avoid a "similar" problem takes 26db706a6d77b9e184feb11725e97e53b7a89519.

Isn't that likely just a workaround for the same issue?  This seems cleaner.

Alex

>
> >
> > Alex
> >
> >
> > >
> > > > Signed-off-by: Rajib Mahapatra <rajib.mahapatra@amd.com>
> > > > ---
> > > >  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > > b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > > index 06a7ceda4c87..02115d63b071 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > > > @@ -2058,6 +2058,10 @@ static int sdma_v4_0_suspend(void *handle)
> > > >  {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >
> > > > +     /* SMU saves SDMA state for us */
> > > > +     if (adev->in_s0ix)
> > > > +             return 0;
> > > > +
> > > >       return sdma_v4_0_hw_fini(adev);
> > > >  }
> > > >
> > > > @@ -2065,6 +2069,10 @@ static int sdma_v4_0_resume(void *handle)
> > > >  {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >
> > > > +     /* SMU restores SDMA state for us */
> > > > +     if (adev->in_s0ix)
> > > > +             return 0;
> > > > +
> > > >       return sdma_v4_0_hw_init(adev);
> > > >  }
> > > >
> > > > --
> > > > 2.25.1

RE: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Limonciello, Mario]

[Public]

> > VG doesn't do s0i3 right?
> 
> Right.
> 
> > No, YC should not take a similar fix.    YC had an architectural change and to
> > avoid a "similar" problem takes
> 26db706a6d77b9e184feb11725e97e53b7a89519.
> 
> Isn't that likely just a workaround for the same issue?  This seems cleaner.
> 

The SMU doesn't handle the restore of the SDMA registers for YC though, this
explicitly changed.  So I don't believe we can do an identical fix there.

@Liang, Prike comments?

Re: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Alex Deucher]

On Thu, Feb 10, 2022 at 11:08 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > > VG doesn't do s0i3 right?
> >
> > Right.
> >
> > > No, YC should not take a similar fix.    YC had an architectural change and to
> > > avoid a "similar" problem takes
> > 26db706a6d77b9e184feb11725e97e53b7a89519.
> >
> > Isn't that likely just a workaround for the same issue?  This seems cleaner.
> >
>
> The SMU doesn't handle the restore of the SDMA registers for YC though, this
> explicitly changed.  So I don't believe we can do an identical fix there.
>
> @Liang, Prike comments?

Ah, ok, in that case, it's probably correct as is.  I assumed the smu
would probably behave the same.

Alex

RE: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for S0ix.

[Liang, Prike]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, February 11, 2022 12:09 AM
> To: Alex Deucher <alexdeucher@gmail.com>; Liang, Prike
> <Prike.Liang@amd.com>
> Cc: Mahapatra, Rajib <Rajib.Mahapatra@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; S, Shirish
> <Shirish.S@amd.com>
> Subject: RE: [PATCH v2] drm/amdgpu: skipping SDMA hw_init and hw_fini for
> S0ix.
> 
> [Public]
> 
> > > VG doesn't do s0i3 right?
> >
> > Right.
> >
> > > No, YC should not take a similar fix.    YC had an architectural change and
> to
> > > avoid a "similar" problem takes
> > 26db706a6d77b9e184feb11725e97e53b7a89519.
> >
> > Isn't that likely just a workaround for the same issue?  This seems cleaner.
> >
> 
> The SMU doesn't handle the restore of the SDMA registers for YC though,
> this explicitly changed.  So I don't believe we can do an identical fix there.
> 
> @Liang, Prike comments?

Yeah, in the gfx10 series looks the SMU doesn't handle SDMA save and restore in the PMFW anymore.