public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: deduplicate JPEG v5.0 interrupt routine
@ 2026-04-21 20:02 Andre Luiz Batista Bueno
  2026-04-22 21:54 ` Claude review: " Claude Code Review Bot
  2026-04-22 21:54 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Andre Luiz Batista Bueno @ 2026-04-21 20:02 UTC (permalink / raw)
  To: amd-gfx, alexander.deucher, christian.koenig, airlied, simona
  Cc: enzo.spinella, Andre Luiz Batista Bueno, dri-devel

Both jpeg_v5_0_1.c and jpeg_v5_0_2.c implement identical
interrupt processing routines. To avoid code duplication,
extract this implementation into a shared common function
in new jpeg_v5_0_interrupt.c and .h files.

Signed-off-by: Andre Luiz Batista Bueno <andrebueno.mac@gmail.com>
Co-developed-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
Signed-off-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
---
 drivers/gpu/drm/amd/amdgpu/Makefile           |  1 +
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_1.c      | 57 +----------------
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_2.c      | 57 +----------------
 .../gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.c  | 64 +++++++++++++++++++
 .../gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.h  | 12 ++++
 5 files changed, 83 insertions(+), 108 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.c
 create mode 100644 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index db66c6372199..69e5e55e1a2b 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -229,6 +229,7 @@ amdgpu-y += \
 	jpeg_v5_0_0.o \
 	jpeg_v5_0_1.o \
 	jpeg_v5_0_2.o \
+	jpeg_v5_0_interrupt.o \
 	jpeg_v5_3_0.o
 
 # add VPE block
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_1.c
index edecbfe66c79..6e5611921eda 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_1.c
@@ -34,6 +34,8 @@
 #include "vcn/vcn_5_0_0_sh_mask.h"
 #include "ivsrcid/vcn/irqsrcs_vcn_5_0.h"
 
+#include "jpeg_v5_0_interrupt.h"
+
 static int jpeg_v5_0_1_start_sriov(struct amdgpu_device *adev);
 static void jpeg_v5_0_1_set_dec_ring_funcs(struct amdgpu_device *adev);
 static void jpeg_v5_0_1_set_irq_funcs(struct amdgpu_device *adev);
@@ -759,60 +761,7 @@ static int jpeg_v5_0_1_process_interrupt(struct amdgpu_device *adev,
 					 struct amdgpu_irq_src *source,
 					 struct amdgpu_iv_entry *entry)
 {
-	u32 i, inst;
-
-	i = node_id_to_phys_map[entry->node_id];
-	DRM_DEV_DEBUG(adev->dev, "IH: JPEG TRAP\n");
-
-	for (inst = 0; inst < adev->jpeg.num_jpeg_inst; ++inst)
-		if (adev->jpeg.inst[inst].aid_id == i)
-			break;
-
-	if (inst >= adev->jpeg.num_jpeg_inst) {
-		dev_WARN_ONCE(adev->dev, 1,
-			      "Interrupt received for unknown JPEG instance %d",
-			      entry->node_id);
-		return 0;
-	}
-
-	switch (entry->src_id) {
-	case VCN_5_0__SRCID__JPEG_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[0]);
-		break;
-	case VCN_5_0__SRCID__JPEG1_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[1]);
-		break;
-	case VCN_5_0__SRCID__JPEG2_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[2]);
-		break;
-	case VCN_5_0__SRCID__JPEG3_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[3]);
-		break;
-	case VCN_5_0__SRCID__JPEG4_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[4]);
-		break;
-	case VCN_5_0__SRCID__JPEG5_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[5]);
-		break;
-	case VCN_5_0__SRCID__JPEG6_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[6]);
-		break;
-	case VCN_5_0__SRCID__JPEG7_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[7]);
-		break;
-	case VCN_5_0__SRCID__JPEG8_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[8]);
-		break;
-	case VCN_5_0__SRCID__JPEG9_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[9]);
-		break;
-	default:
-		DRM_DEV_ERROR(adev->dev, "Unhandled interrupt: %d %d\n",
-			      entry->src_id, entry->src_data[0]);
-		break;
-	}
-
-	return 0;
+	return jpeg_v5_0_process_interrupt_common(adev, entry);
 }
 
 static void jpeg_v5_0_1_core_stall_reset(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_2.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_2.c
index 285c459379c4..daea95907639 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_2.c
@@ -34,6 +34,8 @@
 #include "vcn/vcn_5_0_0_sh_mask.h"
 #include "ivsrcid/vcn/irqsrcs_vcn_5_0.h"
 
+#include "jpeg_v5_0_interrupt.h"
+
 static void jpeg_v5_0_2_set_dec_ring_funcs(struct amdgpu_device *adev);
 static void jpeg_v5_0_2_set_irq_funcs(struct amdgpu_device *adev);
 static int jpeg_v5_0_2_set_powergating_state(struct amdgpu_ip_block *ip_block,
@@ -587,60 +589,7 @@ static int jpeg_v5_0_2_process_interrupt(struct amdgpu_device *adev,
 					 struct amdgpu_irq_src *source,
 					 struct amdgpu_iv_entry *entry)
 {
-	u32 i, inst;
-
-	i = node_id_to_phys_map[entry->node_id];
-	DRM_DEV_DEBUG(adev->dev, "IH: JPEG TRAP\n");
-
-	for (inst = 0; inst < adev->jpeg.num_jpeg_inst; ++inst)
-		if (adev->jpeg.inst[inst].aid_id == i)
-			break;
-
-	if (inst >= adev->jpeg.num_jpeg_inst) {
-		dev_WARN_ONCE(adev->dev, 1,
-			      "Interrupt received for unknown JPEG instance %d",
-			      entry->node_id);
-		return 0;
-	}
-
-	switch (entry->src_id) {
-	case VCN_5_0__SRCID__JPEG_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[0]);
-		break;
-	case VCN_5_0__SRCID__JPEG1_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[1]);
-		break;
-	case VCN_5_0__SRCID__JPEG2_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[2]);
-		break;
-	case VCN_5_0__SRCID__JPEG3_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[3]);
-		break;
-	case VCN_5_0__SRCID__JPEG4_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[4]);
-		break;
-	case VCN_5_0__SRCID__JPEG5_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[5]);
-		break;
-	case VCN_5_0__SRCID__JPEG6_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[6]);
-		break;
-	case VCN_5_0__SRCID__JPEG7_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[7]);
-		break;
-	case VCN_5_0__SRCID__JPEG8_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[8]);
-		break;
-	case VCN_5_0__SRCID__JPEG9_DECODE:
-		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[9]);
-		break;
-	default:
-		DRM_DEV_ERROR(adev->dev, "Unhandled interrupt: %d %d\n",
-			      entry->src_id, entry->src_data[0]);
-		break;
-	}
-
-	return 0;
+	return jpeg_v5_0_process_interrupt_common(adev, entry);
 }
 
 static void jpeg_v5_0_2_core_stall_reset(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.c
new file mode 100644
index 000000000000..a76ee6586b63
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+#include "amdgpu.h"
+#include "ivsrcid/vcn/irqsrcs_vcn_5_0.h"
+#include "jpeg_v5_0_interrupt.h"
+
+int jpeg_v5_0_process_interrupt_common(struct amdgpu_device *adev,
+				       struct amdgpu_iv_entry *entry)
+{
+	u32 i, inst;
+
+	i = node_id_to_phys_map[entry->node_id];
+	DRM_DEV_DEBUG(adev->dev, "IH: JPEG TRAP\n");
+
+	for (inst = 0; inst < adev->jpeg.num_jpeg_inst; ++inst)
+		if (adev->jpeg.inst[inst].aid_id == i)
+			break;
+
+	if (inst >= adev->jpeg.num_jpeg_inst) {
+		dev_WARN_ONCE(adev->dev, 1,
+			      "Interrupt received for unknown JPEG instance %d",
+			      entry->node_id);
+		return 0;
+	}
+
+	switch (entry->src_id) {
+	case VCN_5_0__SRCID__JPEG_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[0]);
+		break;
+	case VCN_5_0__SRCID__JPEG1_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[1]);
+		break;
+	case VCN_5_0__SRCID__JPEG2_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[2]);
+		break;
+	case VCN_5_0__SRCID__JPEG3_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[3]);
+		break;
+	case VCN_5_0__SRCID__JPEG4_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[4]);
+		break;
+	case VCN_5_0__SRCID__JPEG5_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[5]);
+		break;
+	case VCN_5_0__SRCID__JPEG6_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[6]);
+		break;
+	case VCN_5_0__SRCID__JPEG7_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[7]);
+		break;
+	case VCN_5_0__SRCID__JPEG8_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[8]);
+		break;
+	case VCN_5_0__SRCID__JPEG9_DECODE:
+		amdgpu_fence_process(&adev->jpeg.inst[inst].ring_dec[9]);
+		break;
+	default:
+		DRM_DEV_ERROR(adev->dev, "Unhandled interrupt: %d %d\n",
+			      entry->src_id, entry->src_data[0]);
+		break;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.h b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.h
new file mode 100644
index 000000000000..046bcb4a3906
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_interrupt.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+
+#ifndef __JPEG_V5_0_INTERRUPT_H__
+#define __JPEG_V5_0_INTERRUPT_H__
+
+struct amdgpu_device;
+struct amdgpu_iv_entry;
+
+int jpeg_v5_0_process_interrupt_common(struct amdgpu_device *adev,
+				       struct amdgpu_iv_entry *entry);
+
+#endif /* __JPEG_V5_0_INTERRUPT_H__ */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: deduplicate JPEG v5.0 interrupt routine
  2026-04-21 20:02 [PATCH] drm/amdgpu: deduplicate JPEG v5.0 interrupt routine Andre Luiz Batista Bueno
@ 2026-04-22 21:54 ` Claude Code Review Bot
  2026-04-22 21:54 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:54 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amdgpu: deduplicate JPEG v5.0 interrupt routine
Author: Andre Luiz Batista Bueno <andrebueno.mac@gmail.com>
Patches: 1
Reviewed: 2026-04-23T07:54:53.885578

---

This is a single-patch series that extracts an identical interrupt processing routine from `jpeg_v5_0_1.c` and `jpeg_v5_0_2.c` into a shared `jpeg_v5_0_interrupt.c` file. The deduplication itself is correct and mechanical — the extracted function is a faithful copy of the original code from both files, which were byte-for-byte identical.

The patch has one process issue (incorrect `Co-developed-by` / `Signed-off-by` tag ordering) and a minor naming/scope observation. The code change itself is sound.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: deduplicate JPEG v5.0 interrupt routine
  2026-04-21 20:02 [PATCH] drm/amdgpu: deduplicate JPEG v5.0 interrupt routine Andre Luiz Batista Bueno
  2026-04-22 21:54 ` Claude review: " Claude Code Review Bot
@ 2026-04-22 21:54 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:54 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Co-developed-by / Signed-off-by ordering is wrong.** The patch has:

```
Signed-off-by: Andre Luiz Batista Bueno <andrebueno.mac@gmail.com>
Co-developed-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
Signed-off-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
```

Per `Documentation/process/submitting-patches.rst`, "the last Signed-off-by: must always be that of the developer submitting the patch." Since Andre is the `From:` author and submitter, it should be:

```
Co-developed-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
Signed-off-by: Enzo Furegatti Spinella <enzo.spinella@usp.br>
Signed-off-by: Andre Luiz Batista Bueno <andrebueno.mac@gmail.com>
```

**Code review — the extraction is correct.** The new `jpeg_v5_0_process_interrupt_common()` in `jpeg_v5_0_interrupt.c` is an exact copy of the duplicated logic. Both callers in `jpeg_v5_0_1.c` and `jpeg_v5_0_2.c` are converted to thin wrappers that correctly drop the unused `source` parameter:

```c
static int jpeg_v5_0_1_process_interrupt(struct amdgpu_device *adev,
					 struct amdgpu_irq_src *source,
					 struct amdgpu_iv_entry *entry)
{
	return jpeg_v5_0_process_interrupt_common(adev, entry);
}
```

This preserves the `.process` callback signature while the common function only takes what it actually uses. Fine.

**Header is well-structured.** The include guard, forward declarations, and prototype are all correct:

```c
struct amdgpu_device;
struct amdgpu_iv_entry;

int jpeg_v5_0_process_interrupt_common(struct amdgpu_device *adev,
				       struct amdgpu_iv_entry *entry);
```

**Naming observation.** The file is named `jpeg_v5_0_interrupt` which could imply it covers all v5.0.x variants, but `jpeg_v5_0_0.c` and `jpeg_v5_3_0.c` have substantially different interrupt handlers — they are single-instance, don't use `node_id_to_phys_map`, and only handle `VCN_5_0__SRCID__JPEG_DECODE` (one ring, not ten). So this common function is only applicable to the multi-instance variants (v5_0_1 and v5_0_2). The name isn't wrong, but something like `jpeg_v5_0_multi_interrupt` or placing the function in an existing common file might be clearer. This is a minor nit.

**Missed opportunity.** The 10-case switch mapping sequential `JPEG[N]_DECODE` source IDs to `ring_dec[N]` could potentially be replaced with arithmetic if the SRCID values are sequential, but that's a separate improvement and would change behavior during a refactoring patch, so keeping the switch as-is is the correct choice here.

**Makefile change is correct.** The new object file is added in alphabetical order in the right section.

**Verdict:** The code change is correct. The `Co-developed-by` / `Signed-off-by` tag ordering needs to be fixed before merging.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-04-22 21:54 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-21 20:02 [PATCH] drm/amdgpu: deduplicate JPEG v5.0 interrupt routine Andre Luiz Batista Bueno
2026-04-22 21:54 ` Claude review: " Claude Code Review Bot
2026-04-22 21:54 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox