public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions
@ 2026-04-22  0:38 Tiago Dourado
  2026-04-22  0:38 ` [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt Tiago Dourado
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Tiago Dourado @ 2026-04-22  0:38 UTC (permalink / raw)
  To: airlied, alexander.deucher, christian.koenig, simona
  Cc: luiz.f.f.fernandes, Tiago Dourado, amd-gfx, dri-devel

The JPEG versions v3_0 and v5_3_0 have process_interrupt implementations identical to their predecessor versions (v2_0, v5_0_0 respectively). This patchset removes these duplicates by exporting the original functions and reusing them via macro aliases.

Tiago Dourado (2):
  drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt
  drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt

 drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h   |  4 ++++
 drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c   | 21 ++-------------------
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h |  4 ++++
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c | 22 +++-------------------
 6 files changed, 15 insertions(+), 40 deletions(-)

-- 
2.43.0


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

* [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt
  2026-04-22  0:38 [PATCH 0/2] drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Tiago Dourado
@ 2026-04-22  0:38 ` Tiago Dourado
  2026-04-22 21:56   ` Claude review: " Claude Code Review Bot
  2026-04-22  0:38 ` [PATCH 2/2] drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt Tiago Dourado
  2026-04-22 21:56 ` Claude review: drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Tiago Dourado @ 2026-04-22  0:38 UTC (permalink / raw)
  To: airlied, alexander.deucher, christian.koenig, simona
  Cc: luiz.f.f.fernandes, Tiago Dourado, amd-gfx, dri-devel

The jpeg_v3_0_process_interrupt function is identical to
jpeg_v2_0_process_interrupt. Remove the duplicate implementation
in jpeg_v3_0 and reuse the jpeg_v2_0 version via a macro alias.
Export jpeg_v2_0_process_interrupt through jpeg_v2_0.h to allow
cross-version reuse.

Signed-off-by: Tiago Dourado <tiagodourado@usp.br>
Co-developed-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
Signed-off-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
---
 drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h |  4 ++++
 drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c | 21 ++-------------------
 3 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c
index 9fe8d10ab..9006fc57e 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.c
@@ -747,7 +747,7 @@ static int jpeg_v2_0_set_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int jpeg_v2_0_process_interrupt(struct amdgpu_device *adev,
+int jpeg_v2_0_process_interrupt(struct amdgpu_device *adev,
 				      struct amdgpu_irq_src *source,
 				      struct amdgpu_iv_entry *entry)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h
index 654e43e83..4f400fb47 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v2_0.h
@@ -58,6 +58,10 @@ void jpeg_v2_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
 void jpeg_v2_0_dec_ring_emit_wreg(struct amdgpu_ring *ring, uint32_t reg, uint32_t val);
 void jpeg_v2_0_dec_ring_nop(struct amdgpu_ring *ring, uint32_t count);
 
+int jpeg_v2_0_process_interrupt(struct amdgpu_device *adev,
+				struct amdgpu_irq_src *source,
+				struct amdgpu_iv_entry *entry);
+
 extern const struct amdgpu_ip_block_version jpeg_v2_0_ip_block;
 
 #endif /* __JPEG_V2_0_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c
index 98f5e0622..5010dafc2 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v3_0.c
@@ -34,6 +34,8 @@
 
 #define mmUVD_JPEG_PITCH_INTERNAL_OFFSET	0x401f
 
+#define jpeg_v3_0_process_interrupt jpeg_v2_0_process_interrupt
+
 static const struct amdgpu_hwip_reg_entry jpeg_reg_list_3_0[] = {
 	SOC15_REG_ENTRY_STR(JPEG, 0, mmUVD_JPEG_POWER_STATUS),
 	SOC15_REG_ENTRY_STR(JPEG, 0, mmUVD_JPEG_INT_STAT),
@@ -539,25 +541,6 @@ static int jpeg_v3_0_set_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int jpeg_v3_0_process_interrupt(struct amdgpu_device *adev,
-				      struct amdgpu_irq_src *source,
-				      struct amdgpu_iv_entry *entry)
-{
-	DRM_DEBUG("IH: JPEG TRAP\n");
-
-	switch (entry->src_id) {
-	case VCN_2_0__SRCID__JPEG_DECODE:
-		amdgpu_fence_process(adev->jpeg.inst->ring_dec);
-		break;
-	default:
-		DRM_ERROR("Unhandled interrupt: %d %d\n",
-			  entry->src_id, entry->src_data[0]);
-		break;
-	}
-
-	return 0;
-}
-
 static int jpeg_v3_0_ring_reset(struct amdgpu_ring *ring,
 				unsigned int vmid,
 				struct amdgpu_fence *timedout_fence)
-- 
2.43.0


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

* [PATCH 2/2] drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt
  2026-04-22  0:38 [PATCH 0/2] drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Tiago Dourado
  2026-04-22  0:38 ` [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt Tiago Dourado
@ 2026-04-22  0:38 ` Tiago Dourado
  2026-04-22 21:56   ` Claude review: " Claude Code Review Bot
  2026-04-22 21:56 ` Claude review: drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Tiago Dourado @ 2026-04-22  0:38 UTC (permalink / raw)
  To: airlied, alexander.deucher, christian.koenig, simona
  Cc: luiz.f.f.fernandes, Tiago Dourado, amd-gfx, dri-devel

The jpeg_v5_3_0_process_interrupt function is identical to
jpeg_v5_0_0_process_interrupt. Remove the duplicate implementation
in jpeg_v5_3_0 and reuse the jpeg_v5_0_0 version via a macro alias.
Export jpeg_v5_0_0_process_interrupt through jpeg_v5_0_0.h to allow
cross-version reuse.

Signed-off-by: Tiago Dourado <tiagodourado@usp.br>
Co-developed-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
Signed-off-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
---
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h |  4 ++++
 drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c | 22 +++-------------------
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c
index 46bf15dce..4575d1f9d 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.c
@@ -625,7 +625,7 @@ static int jpeg_v5_0_0_set_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int jpeg_v5_0_0_process_interrupt(struct amdgpu_device *adev,
+int jpeg_v5_0_0_process_interrupt(struct amdgpu_device *adev,
 				      struct amdgpu_irq_src *source,
 				      struct amdgpu_iv_entry *entry)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h
index 5abb96159..4eeb0c147 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_0_0.h
@@ -32,4 +32,8 @@
 
 extern const struct amdgpu_ip_block_version jpeg_v5_0_0_ip_block;
 
+int jpeg_v5_0_0_process_interrupt(struct amdgpu_device *adev,
+				  struct amdgpu_irq_src *source,
+				  struct amdgpu_iv_entry *entry);
+
 #endif /* __JPEG_V5_0_0_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c
index 1821dced9..92aaf5dea 100644
--- a/drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v5_3_0.c
@@ -32,8 +32,11 @@
 #include "vcn/vcn_5_3_0_offset.h"
 #include "vcn/vcn_5_3_0_sh_mask.h"
 #include "ivsrcid/vcn/irqsrcs_vcn_5_0.h"
+#include "jpeg_v5_0_0.h"
 #include "jpeg_v5_3_0.h"
 
+#define jpeg_v5_3_0_process_interrupt jpeg_v5_0_0_process_interrupt
+
 static void jpeg_v5_3_0_set_dec_ring_funcs(struct amdgpu_device *adev);
 static void jpeg_v5_3_0_set_irq_funcs(struct amdgpu_device *adev);
 static int jpeg_v5_3_0_set_powergating_state(struct amdgpu_ip_block *ip_block,
@@ -608,25 +611,6 @@ static int jpeg_v5_3_0_set_interrupt_state(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int jpeg_v5_3_0_process_interrupt(struct amdgpu_device *adev,
-				      struct amdgpu_irq_src *source,
-				      struct amdgpu_iv_entry *entry)
-{
-	DRM_DEBUG("IH: JPEG TRAP\n");
-
-	switch (entry->src_id) {
-	case VCN_5_0__SRCID__JPEG_DECODE:
-		amdgpu_fence_process(adev->jpeg.inst->ring_dec);
-		break;
-	default:
-		DRM_DEV_ERROR(adev->dev, "Unhandled interrupt: %d %d\n",
-			  entry->src_id, entry->src_data[0]);
-		break;
-	}
-
-	return 0;
-}
-
 static int jpeg_v5_3_0_ring_reset(struct amdgpu_ring *ring,
 				  unsigned int vmid,
 				  struct amdgpu_fence *timedout_fence)
-- 
2.43.0


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

* Claude review: drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions
  2026-04-22  0:38 [PATCH 0/2] drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Tiago Dourado
  2026-04-22  0:38 ` [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt Tiago Dourado
  2026-04-22  0:38 ` [PATCH 2/2] drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt Tiago Dourado
@ 2026-04-22 21:56 ` Claude Code Review Bot
  2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:56 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions
Author: Tiago Dourado <tiagodourado@usp.br>
Patches: 3
Reviewed: 2026-04-23T07:56:58.862961

---

This is a straightforward deduplication series removing two copy-paste `process_interrupt` implementations in JPEG v3.0 and v5.3.0 that are identical to their predecessor versions (v2.0 and v5.0.0 respectively). The functional change is correct — the deleted code is genuinely identical to the reused functions. However, the implementation uses an unnecessary macro alias pattern instead of simply assigning the function pointer directly, and the Signed-off-by/Co-developed-by tag ordering is wrong per kernel documentation.

**Recommendation: Needs minor rework.**

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt
  2026-04-22  0:38 ` [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt Tiago Dourado
@ 2026-04-22 21:56   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:56 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness:** The deleted `jpeg_v3_0_process_interrupt` is verified identical to `jpeg_v2_0_process_interrupt` — both use `VCN_2_0__SRCID__JPEG_DECODE`, `DRM_ERROR`, and `adev->jpeg.inst->ring_dec`. The deduplication is valid.

**Style concern — macro alias is unnecessary indirection:**

```c
#define jpeg_v3_0_process_interrupt jpeg_v2_0_process_interrupt
```

This macro exists solely so the struct initializer can still say `.process = jpeg_v3_0_process_interrupt`. This is over-engineering — it hides the actual function being called and makes code search harder (grepping for the definition of `jpeg_v3_0_process_interrupt` now lands on a macro, not a function). It would be simpler and more readable to just use the function directly in the struct initializer:

```c
static const struct amdgpu_irq_src_funcs jpeg_v3_0_irq_funcs = {
    .set = jpeg_v3_0_set_interrupt_state,
    .process = jpeg_v2_0_process_interrupt,
};
```

This is the standard kernel pattern for reusing a function from another version — no macro needed.

**Tag ordering issue:** The Signed-off-by / Co-developed-by order is wrong per `Documentation/process/submitting-patches.rst`. Currently:

```
Signed-off-by: Tiago Dourado <tiagodourado@usp.br>
Co-developed-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
Signed-off-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
```

The submitter's `Signed-off-by` must be **last**, and every `Co-developed-by` must be immediately followed by its matching `Signed-off-by`. The correct order is:

```
Co-developed-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
Signed-off-by: Luiz Fernandes <luiz.f.f.fernandes@usp.br>
Signed-off-by: Tiago Dourado <tiagodourado@usp.br>
```

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt
  2026-04-22  0:38 ` [PATCH 2/2] drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt Tiago Dourado
@ 2026-04-22 21:56   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:56 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness:** The deleted `jpeg_v5_3_0_process_interrupt` is verified identical to `jpeg_v5_0_0_process_interrupt` — both use `VCN_5_0__SRCID__JPEG_DECODE`, `DRM_DEV_ERROR`, and `adev->jpeg.inst->ring_dec`. The deduplication is valid.

**Same style concern — unnecessary macro alias:**

```c
#define jpeg_v5_3_0_process_interrupt jpeg_v5_0_0_process_interrupt
```

Same feedback as patch 1: drop the macro and assign `.process = jpeg_v5_0_0_process_interrupt` directly in the `jpeg_v5_3_0_irq_funcs` struct.

**Same tag ordering issue** as patch 1 — submitter's `Signed-off-by` should be last.

---
Generated by Claude Code Patch Reviewer

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

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

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-22  0:38 [PATCH 0/2] drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions Tiago Dourado
2026-04-22  0:38 ` [PATCH 1/2] drm/amdgpu/jpeg: deduplicate jpeg_v3_0 process_interrupt Tiago Dourado
2026-04-22 21:56   ` Claude review: " Claude Code Review Bot
2026-04-22  0:38 ` [PATCH 2/2] drm/amdgpu/jpeg: deduplicate jpeg_v5_3_0 process_interrupt Tiago Dourado
2026-04-22 21:56   ` Claude review: " Claude Code Review Bot
2026-04-22 21:56 ` Claude review: drm/amdgpu/jpeg: deduplicate process_interrupt across JPEG versions 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