[PATCH 0/7] Improvde S0ix stability

[PATCH 0/7] Improvde S0ix stability

[Alex Deucher]

This series improves S0ix stability by avoiding touching
registers that should be handled as part of gfxoff.

Alex Deucher (7):
  drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix
  drm/amdgpu/gmc10: don't touch gfxhub registers during S0ix
  drm/amdgpu/gmc11: don't touch gfxhub registers during S0ix
  drm/amdgpu: don't mess with SDMA clock or powergating in S0ix
  drm/amdgpu: for S0ix, skip SMDA 5.x+ suspend/resume
  Revert "drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle
    resume"
  Revert "drm/amdgpu: force exit gfxoff on sdma resume for rmb s0ix"

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 32 ++++++++--------------
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c     | 26 ++++++++++++------
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c     |  6 ++--
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 26 ++++++++++++++----
 drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c     |  8 ------
 5 files changed, 53 insertions(+), 45 deletions(-)

-- 
2.38.1

[PATCH 1/7] drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix

[Alex Deucher]

gfxhub registers are part of gfx IP and should not need to be
changed.  Doing so without disabling gfxoff can hang the gfx IP.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 08d6cf79fb15..5f07c85bf729 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -484,6 +484,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 			for (i = 0; i < 16; i++) {
 				reg = hub->vm_context0_cntl + i;
 
+				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
+					continue;
+
 				if (j == AMDGPU_GFXHUB_0)
 					tmp = RREG32_SOC15_IP(GC, reg);
 				else
@@ -504,6 +507,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 			for (i = 0; i < 16; i++) {
 				reg = hub->vm_context0_cntl + i;
 
+				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
+					continue;
+
 				if (j == AMDGPU_GFXHUB_0)
 					tmp = RREG32_SOC15_IP(GC, reg);
 				else
@@ -1862,9 +1868,12 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
 	}
 
 	amdgpu_gtt_mgr_recover(&adev->mman.gtt_mgr);
-	r = adev->gfxhub.funcs->gart_enable(adev);
-	if (r)
-		return r;
+
+	if (!adev->in_s0ix) {
+		r = adev->gfxhub.funcs->gart_enable(adev);
+		if (r)
+			return r;
+	}
 
 	r = adev->mmhub.funcs->gart_enable(adev);
 	if (r)
@@ -1911,11 +1920,15 @@ static int gmc_v9_0_hw_init(void *handle)
 		value = true;
 
 	if (!amdgpu_sriov_vf(adev)) {
-		adev->gfxhub.funcs->set_fault_enable_default(adev, value);
+		if (!adev->in_s0ix)
+			adev->gfxhub.funcs->set_fault_enable_default(adev, value);
 		adev->mmhub.funcs->set_fault_enable_default(adev, value);
 	}
-	for (i = 0; i < adev->num_vmhubs; ++i)
+	for (i = 0; i < adev->num_vmhubs; ++i) {
+		if (adev->in_s0ix && (i == AMDGPU_GFXHUB_0))
+			continue;
 		gmc_v9_0_flush_gpu_tlb(adev, 0, i, 0);
+	}
 
 	if (adev->umc.funcs && adev->umc.funcs->init_registers)
 		adev->umc.funcs->init_registers(adev);
@@ -1939,7 +1952,8 @@ static int gmc_v9_0_hw_init(void *handle)
  */
 static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
 {
-	adev->gfxhub.funcs->gart_disable(adev);
+	if (!adev->in_s0ix)
+		adev->gfxhub.funcs->gart_disable(adev);
 	adev->mmhub.funcs->gart_disable(adev);
 }
 
-- 
2.38.1

Re: [PATCH 1/7] drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix

[Christian König]

Am 14.12.22 um 23:16 schrieb Alex Deucher:
> gfxhub registers are part of gfx IP and should not need to be
> changed.  Doing so without disabling gfxoff can hang the gfx IP.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 08d6cf79fb15..5f07c85bf729 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -484,6 +484,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
>   			for (i = 0; i < 16; i++) {
>   				reg = hub->vm_context0_cntl + i;
>   
> +				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
> +					continue;
> +

I'm not sure we can do this because the higher layer will then have an 
incorrect impression of the interrupt state here.

Christian.

>   				if (j == AMDGPU_GFXHUB_0)
>   					tmp = RREG32_SOC15_IP(GC, reg);
>   				else
> @@ -504,6 +507,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
>   			for (i = 0; i < 16; i++) {
>   				reg = hub->vm_context0_cntl + i;
>   
> +				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
> +					continue;
> +
>   				if (j == AMDGPU_GFXHUB_0)
>   					tmp = RREG32_SOC15_IP(GC, reg);
>   				else
> @@ -1862,9 +1868,12 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
>   	}
>   
>   	amdgpu_gtt_mgr_recover(&adev->mman.gtt_mgr);
> -	r = adev->gfxhub.funcs->gart_enable(adev);
> -	if (r)
> -		return r;
> +
> +	if (!adev->in_s0ix) {
> +		r = adev->gfxhub.funcs->gart_enable(adev);
> +		if (r)
> +			return r;
> +	}
>   
>   	r = adev->mmhub.funcs->gart_enable(adev);
>   	if (r)
> @@ -1911,11 +1920,15 @@ static int gmc_v9_0_hw_init(void *handle)
>   		value = true;
>   
>   	if (!amdgpu_sriov_vf(adev)) {
> -		adev->gfxhub.funcs->set_fault_enable_default(adev, value);
> +		if (!adev->in_s0ix)
> +			adev->gfxhub.funcs->set_fault_enable_default(adev, value);
>   		adev->mmhub.funcs->set_fault_enable_default(adev, value);
>   	}
> -	for (i = 0; i < adev->num_vmhubs; ++i)
> +	for (i = 0; i < adev->num_vmhubs; ++i) {
> +		if (adev->in_s0ix && (i == AMDGPU_GFXHUB_0))
> +			continue;
>   		gmc_v9_0_flush_gpu_tlb(adev, 0, i, 0);
> +	}
>   
>   	if (adev->umc.funcs && adev->umc.funcs->init_registers)
>   		adev->umc.funcs->init_registers(adev);
> @@ -1939,7 +1952,8 @@ static int gmc_v9_0_hw_init(void *handle)
>    */
>   static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
>   {
> -	adev->gfxhub.funcs->gart_disable(adev);
> +	if (!adev->in_s0ix)
> +		adev->gfxhub.funcs->gart_disable(adev);
>   	adev->mmhub.funcs->gart_disable(adev);
>   }
>   

Re: [PATCH 1/7] drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix

[Alex Deucher]

On Thu, Dec 15, 2022 at 2:45 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.12.22 um 23:16 schrieb Alex Deucher:
> > gfxhub registers are part of gfx IP and should not need to be
> > changed.  Doing so without disabling gfxoff can hang the gfx IP.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 26 ++++++++++++++++++++------
> >   1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index 08d6cf79fb15..5f07c85bf729 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -484,6 +484,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
> >                       for (i = 0; i < 16; i++) {
> >                               reg = hub->vm_context0_cntl + i;
> >
> > +                             if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
> > +                                     continue;
> > +
>
> I'm not sure we can do this because the higher layer will then have an
> incorrect impression of the interrupt state here.

In this particular case it works because we only touch this interrupt
on init/resume and fini/suspend.  So over the course of a s0i3 cycle
the state would end up being the same, we just don't touch the
registers.

Alex

>
> Christian.
>
> >                               if (j == AMDGPU_GFXHUB_0)
> >                                       tmp = RREG32_SOC15_IP(GC, reg);
> >                               else
> > @@ -504,6 +507,9 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
> >                       for (i = 0; i < 16; i++) {
> >                               reg = hub->vm_context0_cntl + i;
> >
> > +                             if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
> > +                                     continue;
> > +
> >                               if (j == AMDGPU_GFXHUB_0)
> >                                       tmp = RREG32_SOC15_IP(GC, reg);
> >                               else
> > @@ -1862,9 +1868,12 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
> >       }
> >
> >       amdgpu_gtt_mgr_recover(&adev->mman.gtt_mgr);
> > -     r = adev->gfxhub.funcs->gart_enable(adev);
> > -     if (r)
> > -             return r;
> > +
> > +     if (!adev->in_s0ix) {
> > +             r = adev->gfxhub.funcs->gart_enable(adev);
> > +             if (r)
> > +                     return r;
> > +     }
> >
> >       r = adev->mmhub.funcs->gart_enable(adev);
> >       if (r)
> > @@ -1911,11 +1920,15 @@ static int gmc_v9_0_hw_init(void *handle)
> >               value = true;
> >
> >       if (!amdgpu_sriov_vf(adev)) {
> > -             adev->gfxhub.funcs->set_fault_enable_default(adev, value);
> > +             if (!adev->in_s0ix)
> > +                     adev->gfxhub.funcs->set_fault_enable_default(adev, value);
> >               adev->mmhub.funcs->set_fault_enable_default(adev, value);
> >       }
> > -     for (i = 0; i < adev->num_vmhubs; ++i)
> > +     for (i = 0; i < adev->num_vmhubs; ++i) {
> > +             if (adev->in_s0ix && (i == AMDGPU_GFXHUB_0))
> > +                     continue;
> >               gmc_v9_0_flush_gpu_tlb(adev, 0, i, 0);
> > +     }
> >
> >       if (adev->umc.funcs && adev->umc.funcs->init_registers)
> >               adev->umc.funcs->init_registers(adev);
> > @@ -1939,7 +1952,8 @@ static int gmc_v9_0_hw_init(void *handle)
> >    */
> >   static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
> >   {
> > -     adev->gfxhub.funcs->gart_disable(adev);
> > +     if (!adev->in_s0ix)
> > +             adev->gfxhub.funcs->gart_disable(adev);
> >       adev->mmhub.funcs->gart_disable(adev);
> >   }
> >
>

[PATCH 2/7] drm/amdgpu/gmc10: don't touch gfxhub registers during S0ix

[Alex Deucher]

gfxhub registers are part of gfx IP and should not need to be
changed.  Doing so without disabling gfxoff can hang the gfx IP.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 21e46817d82d..6b5aa9b55455 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -78,13 +78,15 @@ gmc_v10_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 		/* MM HUB */
 		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB_0, false);
 		/* GFX HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, false);
+		if (!adev->in_s0ix)
+			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, false);
 		break;
 	case AMDGPU_IRQ_STATE_ENABLE:
 		/* MM HUB */
 		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB_0, true);
 		/* GFX HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, true);
+		if (!adev->in_s0ix)
+			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, true);
 		break;
 	default:
 		break;
@@ -1061,9 +1063,12 @@ static int gmc_v10_0_gart_enable(struct amdgpu_device *adev)
 	}
 
 	amdgpu_gtt_mgr_recover(&adev->mman.gtt_mgr);
-	r = adev->gfxhub.funcs->gart_enable(adev);
-	if (r)
-		return r;
+
+	if (!adev->in_s0ix) {
+		r = adev->gfxhub.funcs->gart_enable(adev);
+		if (r)
+			return r;
+	}
 
 	r = adev->mmhub.funcs->gart_enable(adev);
 	if (r)
@@ -1077,10 +1082,12 @@ static int gmc_v10_0_gart_enable(struct amdgpu_device *adev)
 	value = (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_ALWAYS) ?
 		false : true;
 
-	adev->gfxhub.funcs->set_fault_enable_default(adev, value);
+	if (!adev->in_s0ix)
+		adev->gfxhub.funcs->set_fault_enable_default(adev, value);
 	adev->mmhub.funcs->set_fault_enable_default(adev, value);
 	gmc_v10_0_flush_gpu_tlb(adev, 0, AMDGPU_MMHUB_0, 0);
-	gmc_v10_0_flush_gpu_tlb(adev, 0, AMDGPU_GFXHUB_0, 0);
+	if (!adev->in_s0ix)
+		gmc_v10_0_flush_gpu_tlb(adev, 0, AMDGPU_GFXHUB_0, 0);
 
 	DRM_INFO("PCIE GART of %uM enabled (table at 0x%016llX).\n",
 		 (unsigned)(adev->gmc.gart_size >> 20),
@@ -1101,7 +1108,7 @@ static int gmc_v10_0_hw_init(void *handle)
 	 * harvestable groups in gc_utcl2 need to be programmed before any GFX block
 	 * register setup within GMC, or else system hang when harvesting SA.
 	 */
-	if (adev->gfxhub.funcs && adev->gfxhub.funcs->utcl2_harvest)
+	if (!adev->in_s0ix && adev->gfxhub.funcs && adev->gfxhub.funcs->utcl2_harvest)
 		adev->gfxhub.funcs->utcl2_harvest(adev);
 
 	r = gmc_v10_0_gart_enable(adev);
@@ -1129,7 +1136,8 @@ static int gmc_v10_0_hw_init(void *handle)
  */
 static void gmc_v10_0_gart_disable(struct amdgpu_device *adev)
 {
-	adev->gfxhub.funcs->gart_disable(adev);
+	if (!adev->in_s0ix)
+		adev->gfxhub.funcs->gart_disable(adev);
 	adev->mmhub.funcs->gart_disable(adev);
 }
 
-- 
2.38.1

[PATCH 3/7] drm/amdgpu/gmc11: don't touch gfxhub registers during S0ix

[Alex Deucher]

gfxhub registers are part of gfx IP and should not need to be
changed.  Doing so without disabling gfxoff can hang the gfx IP.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
index 4326078689cd..7e7188a06f42 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
@@ -64,13 +64,15 @@ gmc_v11_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 		/* MM HUB */
 		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB_0, false);
 		/* GFX HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, false);
+		if (!adev->in_s0ix)
+			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, false);
 		break;
 	case AMDGPU_IRQ_STATE_ENABLE:
 		/* MM HUB */
 		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB_0, true);
 		/* GFX HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, true);
+		if (!adev->in_s0ix)
+			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB_0, true);
 		break;
 	default:
 		break;
-- 
2.38.1

[PATCH 4/7] drm/amdgpu: don't mess with SDMA clock or powergating in S0ix

[Alex Deucher]

It's handled by GFXOFF for SDMA 5.x and SMU saves the state on
SDMA 4.x.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 64660a41d53c..a99b327d5f09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2581,9 +2581,10 @@ int amdgpu_device_set_cg_state(struct amdgpu_device *adev,
 		i = state == AMD_CG_STATE_GATE ? j : adev->num_ip_blocks - j - 1;
 		if (!adev->ip_blocks[i].status.late_initialized)
 			continue;
-		/* skip CG for GFX on S0ix */
+		/* skip CG for GFX, SDMA on S0ix */
 		if (adev->in_s0ix &&
-		    adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX)
+		    (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX ||
+		     adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA))
 			continue;
 		/* skip CG for VCE/UVD, it's handled specially */
 		if (adev->ip_blocks[i].version->type != AMD_IP_BLOCK_TYPE_UVD &&
@@ -2617,9 +2618,10 @@ int amdgpu_device_set_pg_state(struct amdgpu_device *adev,
 		i = state == AMD_PG_STATE_GATE ? j : adev->num_ip_blocks - j - 1;
 		if (!adev->ip_blocks[i].status.late_initialized)
 			continue;
-		/* skip PG for GFX on S0ix */
+		/* skip PG for GFX, SDMA on S0ix */
 		if (adev->in_s0ix &&
-		    adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX)
+		    (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX ||
+		     adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA))
 			continue;
 		/* skip CG for VCE/UVD, it's handled specially */
 		if (adev->ip_blocks[i].version->type != AMD_IP_BLOCK_TYPE_UVD &&
-- 
2.38.1

[PATCH 5/7] drm/amdgpu: for S0ix, skip SMDA 5.x+ suspend/resume

[Alex Deucher]

SDMA 5.x is part of the GFX block so it's controlled via
GFXOFF.  Skip suspend as it should be handled the same
as GFX.

v2: drop SDMA 4.x.  That requires special handling.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a99b327d5f09..5c0719c03c37 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3028,6 +3028,12 @@ static int amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
 		     adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX))
 			continue;
 
+		/* SDMA 5.x+ is part of GFX power domain so it's covered by GFXOFF */
+		if (adev->in_s0ix &&
+		    (adev->ip_versions[SDMA0_HWIP][0] >= IP_VERSION(5, 0, 0)) &&
+		    (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA))
+			continue;
+
 		/* XXX handle errors */
 		r = adev->ip_blocks[i].version->funcs->suspend(adev);
 		/* XXX handle errors */
-- 
2.38.1

Re: [PATCH 5/7] drm/amdgpu: for S0ix, skip SMDA 5.x+ suspend/resume

[Bhardwaj, Rajneesh]

Don't we need a similar check on resume_phase2?

Other than that, looks good to me.

Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>

On 12/14/2022 5:16 PM, Alex Deucher wrote:
> SDMA 5.x is part of the GFX block so it's controlled via
> GFXOFF.  Skip suspend as it should be handled the same
> as GFX.
>
> v2: drop SDMA 4.x.  That requires special handling.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a99b327d5f09..5c0719c03c37 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3028,6 +3028,12 @@ static int amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
>   		     adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX))
>   			continue;
>   
> +		/* SDMA 5.x+ is part of GFX power domain so it's covered by GFXOFF */
> +		if (adev->in_s0ix &&
> +		    (adev->ip_versions[SDMA0_HWIP][0] >= IP_VERSION(5, 0, 0)) &&
> +		    (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA))
> +			continue;
> +
>   		/* XXX handle errors */
>   		r = adev->ip_blocks[i].version->funcs->suspend(adev);
>   		/* XXX handle errors */

Re: [PATCH 5/7] drm/amdgpu: for S0ix, skip SMDA 5.x+ suspend/resume

[Alex Deucher]

On Thu, Dec 15, 2022 at 10:05 AM Bhardwaj, Rajneesh
<rajneesh.bhardwaj@amd.com> wrote:
>
> Don't we need a similar check on resume_phase2?

The resume code looks to see if the IP was suspended in the first
place before trying to resume it so no need.

Alex

>
> Other than that, looks good to me.
>
> Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>
> On 12/14/2022 5:16 PM, Alex Deucher wrote:
> > SDMA 5.x is part of the GFX block so it's controlled via
> > GFXOFF.  Skip suspend as it should be handled the same
> > as GFX.
> >
> > v2: drop SDMA 4.x.  That requires special handling.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index a99b327d5f09..5c0719c03c37 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3028,6 +3028,12 @@ static int amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
> >                    adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GFX))
> >                       continue;
> >
> > +             /* SDMA 5.x+ is part of GFX power domain so it's covered by GFXOFF */
> > +             if (adev->in_s0ix &&
> > +                 (adev->ip_versions[SDMA0_HWIP][0] >= IP_VERSION(5, 0, 0)) &&
> > +                 (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SDMA))
> > +                     continue;
> > +
> >               /* XXX handle errors */
> >               r = adev->ip_blocks[i].version->funcs->suspend(adev);
> >               /* XXX handle errors */

[PATCH 6/7] Revert "drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume"

[Alex Deucher]

This reverts commit f543d28687480fad06b708bc6e0b0b6ec953b078.

This is no longer needed since we no longer touch SDMA 5.x for s0i3.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5c0719c03c37..582a80a9850e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3234,15 +3234,6 @@ static int amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
 			return r;
 		}
 		adev->ip_blocks[i].status.hw = true;
-
-		if (adev->in_s0ix && adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SMC) {
-			/* disable gfxoff for IP resume. The gfxoff will be re-enabled in
-			 * amdgpu_device_resume() after IP resume.
-			 */
-			amdgpu_gfx_off_ctrl(adev, false);
-			DRM_DEBUG("will disable gfxoff for re-initializing other blocks\n");
-		}
-
 	}
 
 	return 0;
@@ -4230,13 +4221,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
 	/* Make sure IB tests flushed */
 	flush_delayed_work(&adev->delayed_init_work);
 
-	if (adev->in_s0ix) {
-		/* re-enable gfxoff after IP resume. This re-enables gfxoff after
-		 * it was disabled for IP resume in amdgpu_device_ip_resume_phase2().
-		 */
-		amdgpu_gfx_off_ctrl(adev, true);
-		DRM_DEBUG("will enable gfxoff for the mission mode\n");
-	}
 	if (fbcon)
 		drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, false);
 
-- 
2.38.1

[PATCH 7/7] Revert "drm/amdgpu: force exit gfxoff on sdma resume for rmb s0ix"

[Alex Deucher]

This reverts commit e5d59cfa330523e47cba62a496864acc3948fc27.

This is no longer needed since we no longer suspend SDMA during
S0ix.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
index 809eca54fc61..65e7a710298d 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
@@ -809,12 +809,6 @@ static int sdma_v5_2_start(struct amdgpu_device *adev)
 			msleep(1000);
 	}
 
-	/* TODO: check whether can submit a doorbell request to raise
-	 * a doorbell fence to exit gfxoff.
-	 */
-	if (adev->in_s0ix)
-		amdgpu_gfx_off_ctrl(adev, false);
-
 	sdma_v5_2_soft_reset(adev);
 	/* unhalt the MEs */
 	sdma_v5_2_enable(adev, true);
@@ -823,8 +817,6 @@ static int sdma_v5_2_start(struct amdgpu_device *adev)
 
 	/* start the gfx rings and rlc compute queues */
 	r = sdma_v5_2_gfx_resume(adev);
-	if (adev->in_s0ix)
-		amdgpu_gfx_off_ctrl(adev, true);
 	if (r)
 		return r;
 	r = sdma_v5_2_rlc_resume(adev);
-- 
2.38.1

[PATCH 0/7 v2] Improve S0ix stability

[Alex Deucher]

This series improves S0ix stability by avoiding touching
registers that should be handled as part of gfxoff.

v2: add comments in gmc code to explain why we can
skip the vm fault state setting for gfxhub.

Alex Deucher (7):
  drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix
  drm/amdgpu/gmc10: don't touch gfxhub registers during S0ix
  drm/amdgpu/gmc11: don't touch gfxhub registers during S0ix
  drm/amdgpu: don't mess with SDMA clock or powergating in S0ix
  drm/amdgpu: for S0ix, skip SMDA 5.x+ suspend/resume
  Revert "drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle
    resume"
  Revert "drm/amdgpu: force exit gfxoff on sdma resume for rmb s0ix"

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 32 ++++++++-----------
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c     | 36 ++++++++++++++++------
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c     | 16 ++++++++--
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 36 ++++++++++++++++++----
 drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c     |  8 -----
 5 files changed, 83 insertions(+), 45 deletions(-)

-- 
2.38.1

[PATCH 1/7] drm/amdgpu/gmc9: don't touch gfxhub registers during S0ix

[Alex Deucher]

gfxhub registers are part of gfx IP and should not need to be
changed.  Doing so without disabling gfxoff can hang the gfx IP.

v2: add comments explaining why we can skip the interrupt
    control for S0i3

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 36 ++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 08d6cf79fb15..8f7fa468decb 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -484,6 +484,14 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 			for (i = 0; i < 16; i++) {
 				reg = hub->vm_context0_cntl + i;
 
+				/* This works because this interrupt is only
+				 * enabled at init/resume and disabled in
+				 * fini/suspend, so the overall state doesn't
+				 * change over the course of suspend/resume.
+				 */
+				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
+					continue;
+
 				if (j == AMDGPU_GFXHUB_0)
 					tmp = RREG32_SOC15_IP(GC, reg);
 				else
@@ -504,6 +512,14 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
 			for (i = 0; i < 16; i++) {
 				reg = hub->vm_context0_cntl + i;
 
+				/* This works because this interrupt is only
+				 * enabled at init/resume and disabled in
+				 * fini/suspend, so the overall state doesn't
+				 * change over the course of suspend/resume.
+				 */
+				if (adev->in_s0ix && (j == AMDGPU_GFXHUB_0))
+					continue;
+
 				if (j == AMDGPU_GFXHUB_0)
 					tmp = RREG32_SOC15_IP(GC, reg);
 				else
@@ -1862,9 +1878,12 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
 	}
 
 	amdgpu_gtt_mgr_recover(&adev->mman.gtt_mgr);
-	r = adev->gfxhub.funcs->gart_enable(adev);
-	if (r)
-		return r;
+
+	if (!adev->in_s0ix) {
+		r = adev->gfxhub.funcs->gart_enable(adev);
+		if (r)
+			return r;
+	}
 
 	r = adev->mmhub.funcs->gart_enable(adev);
 	if (r)
@@ -1911,11 +1930,15 @@ static int gmc_v9_0_hw_init(void *handle)
 		value = true;
 
 	if (!amdgpu_sriov_vf(adev)) {
-		adev->gfxhub.funcs->set_fault_enable_default(adev, value);
+		if (!adev->in_s0ix)
+			adev->gfxhub.funcs->set_fault_enable_default(adev, value);
 		adev->mmhub.funcs->set_fault_enable_default(adev, value);
 	}
-	for (i = 0; i < adev->num_vmhubs; ++i)
+	for (i = 0; i < adev->num_vmhubs; ++i) {
+		if (adev->in_s0ix && (i == AMDGPU_GFXHUB_0))
+			continue;
 		gmc_v9_0_flush_gpu_tlb(adev, 0, i, 0);
+	}
 
 	if (adev->umc.funcs && adev->umc.funcs->init_registers)
 		adev->umc.funcs->init_registers(adev);
@@ -1939,7 +1962,8 @@ static int gmc_v9_0_hw_init(void *handle)
  */
 static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
 {
-	adev->gfxhub.funcs->gart_disable(adev);
+	if (!adev->in_s0ix)
+		adev->gfxhub.funcs->gart_disable(adev);
 	adev->mmhub.funcs->gart_disable(adev);
 }
 
-- 
2.38.1