[PATCH] drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0

[PATCH] drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0

[Ryan Oliveira]

From: Ryan Oliveira <ryan.oliveiraf@usp.br>

This patch consolidates vm_fault_interrupt_state handling logic found
in gmc_v10_0.c and gmc_v12_0.c into a shared helper function inside
amdgpu_gmc.c

Since the v10 and v12_0 implementations are identical copies, moving
this to amdgpu_gmc simplifies maintenance and reduces code duplication.

Signed-off-by: Ryan Oliveira <ryan.oliveiraf@usp.br>
Co-developed-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Kaiky Cintra <kaikycintra@usp.br>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 37 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  3 ++
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  | 39 +------------------------
 drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c  | 38 +-----------------------
 4 files changed, 42 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 13bec8461cde..eebf5c17efb4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -590,6 +590,43 @@ int amdgpu_gmc_handle_retry_fault(struct amdgpu_device *adev,
 	return 0;
 }
 
+int amdgpu_gmc_vm_fault_interrupt_state(struct amdgpu_device *adev,
+				   struct amdgpu_irq_src *src, unsigned int type,
+				   enum amdgpu_interrupt_state state)
+{
+	switch (state) {
+	case AMDGPU_IRQ_STATE_DISABLE:
+		/* MM HUB */
+		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB0(0), false);
+		/* GFX HUB */
+		/* 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)
+			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_MMHUB0(0), true);
+		/* GFX HUB */
+		/* 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)
+			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), true);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+
 int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev)
 {
 	int r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index 676e3aaa1f27..386e5f292c4e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -437,6 +437,9 @@ int amdgpu_gmc_handle_retry_fault(struct amdgpu_device *adev,
 				  u32 cam_index,
 				  u32 node_id,
 				  bool write_fault);
+int amdgpu_gmc_vm_fault_interrupt_state(struct amdgpu_device *adev,
+				   struct amdgpu_irq_src *src, unsigned int type,
+				   enum amdgpu_interrupt_state state);
 int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev);
 int amdgpu_gmc_ras_late_init(struct amdgpu_device *adev);
 void amdgpu_gmc_ras_fini(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 8523833a74fb..4aaa3c65e186 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -59,43 +59,6 @@ static int gmc_v10_0_ecc_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int
-gmc_v10_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
-				   struct amdgpu_irq_src *src, unsigned int type,
-				   enum amdgpu_interrupt_state state)
-{
-	switch (state) {
-	case AMDGPU_IRQ_STATE_DISABLE:
-		/* MM HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB0(0), false);
-		/* GFX HUB */
-		/* 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)
-			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_MMHUB0(0), true);
-		/* GFX HUB */
-		/* 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)
-			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), true);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
 static int gmc_v10_0_process_interrupt(struct amdgpu_device *adev,
 				       struct amdgpu_irq_src *source,
 				       struct amdgpu_iv_entry *entry)
@@ -168,7 +131,7 @@ static int gmc_v10_0_process_interrupt(struct amdgpu_device *adev,
 }
 
 static const struct amdgpu_irq_src_funcs gmc_v10_0_irq_funcs = {
-	.set = gmc_v10_0_vm_fault_interrupt_state,
+	.set = amdgpu_gmc_vm_fault_interrupt_state,
 	.process = gmc_v10_0_process_interrupt,
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
index 586703ec0dfa..d3eabe3b6dff 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
@@ -53,42 +53,6 @@ static int gmc_v12_0_ecc_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int gmc_v12_0_vm_fault_interrupt_state(struct amdgpu_device *adev,
-					      struct amdgpu_irq_src *src, unsigned type,
-					      enum amdgpu_interrupt_state state)
-{
-	switch (state) {
-	case AMDGPU_IRQ_STATE_DISABLE:
-		/* MM HUB */
-		amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_MMHUB0(0), false);
-		/* GFX HUB */
-		/* 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)
-			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_MMHUB0(0), true);
-		/* GFX HUB */
-		/* 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)
-			amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), true);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
 static int gmc_v12_0_process_interrupt(struct amdgpu_device *adev,
 				       struct amdgpu_irq_src *source,
 				       struct amdgpu_iv_entry *entry)
@@ -160,7 +124,7 @@ static int gmc_v12_0_process_interrupt(struct amdgpu_device *adev,
 }
 
 static const struct amdgpu_irq_src_funcs gmc_v12_0_irq_funcs = {
-	.set = gmc_v12_0_vm_fault_interrupt_state,
+	.set = amdgpu_gmc_vm_fault_interrupt_state,
 	.process = gmc_v12_0_process_interrupt,
 };
 
-- 
2.54.0

Claude review: drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0
Author: Ryan Oliveira <ryan.oliveiraf@usp.br>
Patches: 1
Reviewed: 2026-05-25T20:48:58.745626

---

This is a single-patch series that consolidates the identical `vm_fault_interrupt_state` function from `gmc_v10_0.c` and `gmc_v12_0.c` into a shared helper in `amdgpu_gmc.c`. The refactoring is straightforward and correct — the two removed implementations are byte-for-byte identical, so the consolidation is safe.

The patch is simple, low-risk, and does what it says. However, there are a couple of issues worth addressing.

**Verdict:** Needs minor fixes before merging.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The extracted function is a faithful copy of the code removed from both files. The `.set` callback pointer swap is correct.

**Issue 1 — Co-developed-by / Signed-off-by ordering (must fix)**

The current tag ordering is:
```
Signed-off-by: Ryan Oliveira <ryan.oliveiraf@usp.br>
Co-developed-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Kaiky Cintra <kaikycintra@usp.br>
```

Per `Documentation/process/submitting-patches.rst`, the submitter's (From: author's) `Signed-off-by` must be last, and each `Co-developed-by` must be immediately followed by the corresponding `Signed-off-by`. The correct ordering is:

```
Co-developed-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Ryan Oliveira <ryan.oliveiraf@usp.br>
```

**Issue 2 — Extra blank line (nit)**

There's a double blank line between the new function and `amdgpu_gmc_ras_sw_init`:

```c
	return 0;
}


int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev)
```

Kernel style uses a single blank line between functions.

**Issue 3 — gmc_v11_0 has a near-identical copy that could also be consolidated (suggestion)**

`gmc_v11_0.c` has an almost identical implementation, differing only in the `DISABLE` case where it adds an extra condition:

```c
// gmc_v11_0 DISABLE case:
if (!adev->in_s0ix && (adev->in_runpm || adev->in_suspend ||
                       amdgpu_in_reset(adev)))
    amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false);

// gmc_v10_0 / gmc_v12_0 (and the new shared helper) DISABLE case:
if (!adev->in_s0ix)
    amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false);
```

The ENABLE paths are identical across all three. This isn't a blocker for this patch, but the commit message could briefly note why gmc_v11_0 was intentionally excluded (different DISABLE logic) so future readers don't wonder. It's also worth considering whether gmc_v11_0 could be consolidated with a flag or callback parameter, but that's a separate patch.

Note: `gmc_v12_1`, `gmc_v9_0`, and the older versions (v6/v7/v8) all have structurally different implementations (register-level iteration, different hub handling), so they are correctly left untouched.

**Summary:** Correct and useful deduplication. Fix the SoB ordering and the extra blank line, and optionally note why gmc_v11_0 was left out.

---
Generated by Claude Code Patch Reviewer