* [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler @ 2026-04-20 3:28 Giovanna Uchoa 2026-04-20 18:18 ` Alex Deucher 0 siblings, 1 reply; 8+ messages in thread From: Giovanna Uchoa @ 2026-04-20 3:28 UTC (permalink / raw) To: alexander.deucher, christian.koenig, airlied, simona; +Cc: amd-gfx, dri-devel Move the amdgpu_sdma_process_trap_irq handler from version-specific implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA module (amdgpu_sdma). This eliminates code duplication and centralizes the trap interrupt handling logic, which is identical across all SDMA versions. Update the trap_irq_funcs in each version-specific module to reference the common handler implementation. Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> --- drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- 5 files changed, 47 insertions(+), 124 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c index 321310ba2..4f15334ce 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, return 0; } +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, + struct amdgpu_irq_src *source, + struct amdgpu_iv_entry *entry) +{ + u8 instance_id, queue_id; + + instance_id = (entry->ring_id & 0x3) >> 0; + queue_id = (entry->ring_id & 0xc) >> 2; + DRM_DEBUG("IH: SDMA trap\n"); + switch (instance_id) { + case 0: + switch (queue_id) { + case 0: + amdgpu_fence_process(&adev->sdma.instance[0].ring); + break; + case 1: + /* XXX compute */ + break; + case 2: + /* XXX compute */ + break; + } + break; + case 1: + switch (queue_id) { + case 0: + amdgpu_fence_process(&adev->sdma.instance[1].ring); + break; + case 1: + /* XXX compute */ + break; + case 2: + /* XXX compute */ + break; + } + break; + } + + return 0; +} + static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) { uint16_t version_major; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h index 2bf365609..ca4fd94ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry); +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, + struct amdgpu_irq_src *source, + struct amdgpu_iv_entry *entry); int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, bool duplicate); void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c index 120da838a..1bf1af633 100644 --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, return 0; } -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, - struct amdgpu_irq_src *source, - struct amdgpu_iv_entry *entry) -{ - u8 instance_id, queue_id; - - instance_id = (entry->ring_id & 0x3) >> 0; - queue_id = (entry->ring_id & 0xc) >> 2; - DRM_DEBUG("IH: SDMA trap\n"); - switch (instance_id) { - case 0: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[0].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - case 1: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[1].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - } - - return 0; -} - static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { .set = cik_sdma_set_trap_irq_state, - .process = cik_sdma_process_trap_irq, + .process = amdgpu_sdma_process_trap_irq, }; static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c index 93ec52c1f..545077897 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, return 0; } -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, - struct amdgpu_irq_src *source, - struct amdgpu_iv_entry *entry) -{ - u8 instance_id, queue_id; - - instance_id = (entry->ring_id & 0x3) >> 0; - queue_id = (entry->ring_id & 0xc) >> 2; - DRM_DEBUG("IH: SDMA trap\n"); - switch (instance_id) { - case 0: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[0].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - case 1: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[1].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - } - return 0; -} - static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { .set = sdma_v2_4_set_trap_irq_state, - .process = sdma_v2_4_process_trap_irq, + .process = amdgpu_sdma_process_trap_irq, }; static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c index 3fde9be74..b3eab4e11 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, return 0; } -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, - struct amdgpu_irq_src *source, - struct amdgpu_iv_entry *entry) -{ - u8 instance_id, queue_id; - - instance_id = (entry->ring_id & 0x3) >> 0; - queue_id = (entry->ring_id & 0xc) >> 2; - DRM_DEBUG("IH: SDMA trap\n"); - switch (instance_id) { - case 0: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[0].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - case 1: - switch (queue_id) { - case 0: - amdgpu_fence_process(&adev->sdma.instance[1].ring); - break; - case 1: - /* XXX compute */ - break; - case 2: - /* XXX compute */ - break; - } - break; - } - return 0; -} - static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { .set = sdma_v3_0_set_trap_irq_state, - .process = sdma_v3_0_process_trap_irq, + .process = amdgpu_sdma_process_trap_irq, }; static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { -- 2.53.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 3:28 [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler Giovanna Uchoa @ 2026-04-20 18:18 ` Alex Deucher 2026-04-20 18:23 ` Christian König ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Alex Deucher @ 2026-04-20 18:18 UTC (permalink / raw) To: Giovanna Uchoa Cc: alexander.deucher, christian.koenig, airlied, simona, amd-gfx, dri-devel Applied. Thanks! Alex On Mon, Apr 20, 2026 at 9:09 AM Giovanna Uchoa <giovannauchoa@usp.br> wrote: > > Move the amdgpu_sdma_process_trap_irq handler from version-specific > implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA > module (amdgpu_sdma). This eliminates code duplication and centralizes > the trap interrupt handling logic, which is identical across all SDMA > versions. > > Update the trap_irq_funcs in each version-specific module to reference > the common handler implementation. > > Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ > drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- > drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- > drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- > 5 files changed, 47 insertions(+), 124 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > index 321310ba2..4f15334ce 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > return 0; > } > > +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > + struct amdgpu_irq_src *source, > + struct amdgpu_iv_entry *entry) > +{ > + u8 instance_id, queue_id; > + > + instance_id = (entry->ring_id & 0x3) >> 0; > + queue_id = (entry->ring_id & 0xc) >> 2; > + DRM_DEBUG("IH: SDMA trap\n"); > + switch (instance_id) { > + case 0: > + switch (queue_id) { > + case 0: > + amdgpu_fence_process(&adev->sdma.instance[0].ring); > + break; > + case 1: > + /* XXX compute */ > + break; > + case 2: > + /* XXX compute */ > + break; > + } > + break; > + case 1: > + switch (queue_id) { > + case 0: > + amdgpu_fence_process(&adev->sdma.instance[1].ring); > + break; > + case 1: > + /* XXX compute */ > + break; > + case 2: > + /* XXX compute */ > + break; > + } > + break; > + } > + > + return 0; > +} > + > static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) > { > uint16_t version_major; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > index 2bf365609..ca4fd94ac 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, > int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > struct amdgpu_irq_src *source, > struct amdgpu_iv_entry *entry); > +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > + struct amdgpu_irq_src *source, > + struct amdgpu_iv_entry *entry); > int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, > bool duplicate); > void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, > diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > index 120da838a..1bf1af633 100644 > --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, > return 0; > } > > -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, > - struct amdgpu_irq_src *source, > - struct amdgpu_iv_entry *entry) > -{ > - u8 instance_id, queue_id; > - > - instance_id = (entry->ring_id & 0x3) >> 0; > - queue_id = (entry->ring_id & 0xc) >> 2; > - DRM_DEBUG("IH: SDMA trap\n"); > - switch (instance_id) { > - case 0: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[0].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - case 1: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[1].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - } > - > - return 0; > -} > - > static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, > struct amdgpu_irq_src *source, > struct amdgpu_iv_entry *entry) > @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) > > static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { > .set = cik_sdma_set_trap_irq_state, > - .process = cik_sdma_process_trap_irq, > + .process = amdgpu_sdma_process_trap_irq, > }; > > static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > index 93ec52c1f..545077897 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, > return 0; > } > > -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, > - struct amdgpu_irq_src *source, > - struct amdgpu_iv_entry *entry) > -{ > - u8 instance_id, queue_id; > - > - instance_id = (entry->ring_id & 0x3) >> 0; > - queue_id = (entry->ring_id & 0xc) >> 2; > - DRM_DEBUG("IH: SDMA trap\n"); > - switch (instance_id) { > - case 0: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[0].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - case 1: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[1].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - } > - return 0; > -} > - > static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, > struct amdgpu_irq_src *source, > struct amdgpu_iv_entry *entry) > @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) > > static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { > .set = sdma_v2_4_set_trap_irq_state, > - .process = sdma_v2_4_process_trap_irq, > + .process = amdgpu_sdma_process_trap_irq, > }; > > static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > index 3fde9be74..b3eab4e11 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, > return 0; > } > > -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, > - struct amdgpu_irq_src *source, > - struct amdgpu_iv_entry *entry) > -{ > - u8 instance_id, queue_id; > - > - instance_id = (entry->ring_id & 0x3) >> 0; > - queue_id = (entry->ring_id & 0xc) >> 2; > - DRM_DEBUG("IH: SDMA trap\n"); > - switch (instance_id) { > - case 0: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[0].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - case 1: > - switch (queue_id) { > - case 0: > - amdgpu_fence_process(&adev->sdma.instance[1].ring); > - break; > - case 1: > - /* XXX compute */ > - break; > - case 2: > - /* XXX compute */ > - break; > - } > - break; > - } > - return 0; > -} > - > static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, > struct amdgpu_irq_src *source, > struct amdgpu_iv_entry *entry) > @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) > > static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { > .set = sdma_v3_0_set_trap_irq_state, > - .process = sdma_v3_0_process_trap_irq, > + .process = amdgpu_sdma_process_trap_irq, > }; > > static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { > -- > 2.53.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:18 ` Alex Deucher @ 2026-04-20 18:23 ` Christian König 2026-04-20 18:25 ` Alex Deucher 2026-04-22 23:40 ` Claude review: " Claude Code Review Bot 2026-04-22 23:40 ` Claude Code Review Bot 2 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2026-04-20 18:23 UTC (permalink / raw) To: Alex Deucher, Giovanna Uchoa Cc: alexander.deucher, airlied, simona, amd-gfx, dri-devel Wait a second Alex, this patch actually doesn't make sense at all. The code is only common for a subset of SDMA engines and so shouldn't be moved into a common handler. Regards, Christian. On 4/20/26 20:18, Alex Deucher wrote: > Applied. Thanks! > > Alex > > On Mon, Apr 20, 2026 at 9:09 AM Giovanna Uchoa <giovannauchoa@usp.br> wrote: >> >> Move the amdgpu_sdma_process_trap_irq handler from version-specific >> implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA >> module (amdgpu_sdma). This eliminates code duplication and centralizes >> the trap interrupt handling logic, which is identical across all SDMA >> versions. >> >> Update the trap_irq_funcs in each version-specific module to reference >> the common handler implementation. >> >> Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ >> drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- >> drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- >> drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- >> 5 files changed, 47 insertions(+), 124 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >> index 321310ba2..4f15334ce 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >> @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, >> return 0; >> } >> >> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, >> + struct amdgpu_irq_src *source, >> + struct amdgpu_iv_entry *entry) >> +{ >> + u8 instance_id, queue_id; >> + >> + instance_id = (entry->ring_id & 0x3) >> 0; >> + queue_id = (entry->ring_id & 0xc) >> 2; >> + DRM_DEBUG("IH: SDMA trap\n"); >> + switch (instance_id) { >> + case 0: >> + switch (queue_id) { >> + case 0: >> + amdgpu_fence_process(&adev->sdma.instance[0].ring); >> + break; >> + case 1: >> + /* XXX compute */ >> + break; >> + case 2: >> + /* XXX compute */ >> + break; >> + } >> + break; >> + case 1: >> + switch (queue_id) { >> + case 0: >> + amdgpu_fence_process(&adev->sdma.instance[1].ring); >> + break; >> + case 1: >> + /* XXX compute */ >> + break; >> + case 2: >> + /* XXX compute */ >> + break; >> + } >> + break; >> + } >> + >> + return 0; >> +} >> + >> static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) >> { >> uint16_t version_major; >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >> index 2bf365609..ca4fd94ac 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >> @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, >> int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, >> struct amdgpu_irq_src *source, >> struct amdgpu_iv_entry *entry); >> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, >> + struct amdgpu_irq_src *source, >> + struct amdgpu_iv_entry *entry); >> int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, >> bool duplicate); >> void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, >> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >> index 120da838a..1bf1af633 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >> @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, >> return 0; >> } >> >> -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, >> - struct amdgpu_irq_src *source, >> - struct amdgpu_iv_entry *entry) >> -{ >> - u8 instance_id, queue_id; >> - >> - instance_id = (entry->ring_id & 0x3) >> 0; >> - queue_id = (entry->ring_id & 0xc) >> 2; >> - DRM_DEBUG("IH: SDMA trap\n"); >> - switch (instance_id) { >> - case 0: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - case 1: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - } >> - >> - return 0; >> -} >> - >> static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, >> struct amdgpu_irq_src *source, >> struct amdgpu_iv_entry *entry) >> @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) >> >> static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { >> .set = cik_sdma_set_trap_irq_state, >> - .process = cik_sdma_process_trap_irq, >> + .process = amdgpu_sdma_process_trap_irq, >> }; >> >> static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { >> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >> index 93ec52c1f..545077897 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >> @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, >> return 0; >> } >> >> -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, >> - struct amdgpu_irq_src *source, >> - struct amdgpu_iv_entry *entry) >> -{ >> - u8 instance_id, queue_id; >> - >> - instance_id = (entry->ring_id & 0x3) >> 0; >> - queue_id = (entry->ring_id & 0xc) >> 2; >> - DRM_DEBUG("IH: SDMA trap\n"); >> - switch (instance_id) { >> - case 0: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - case 1: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - } >> - return 0; >> -} >> - >> static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, >> struct amdgpu_irq_src *source, >> struct amdgpu_iv_entry *entry) >> @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) >> >> static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { >> .set = sdma_v2_4_set_trap_irq_state, >> - .process = sdma_v2_4_process_trap_irq, >> + .process = amdgpu_sdma_process_trap_irq, >> }; >> >> static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { >> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >> index 3fde9be74..b3eab4e11 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >> @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, >> return 0; >> } >> >> -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, >> - struct amdgpu_irq_src *source, >> - struct amdgpu_iv_entry *entry) >> -{ >> - u8 instance_id, queue_id; >> - >> - instance_id = (entry->ring_id & 0x3) >> 0; >> - queue_id = (entry->ring_id & 0xc) >> 2; >> - DRM_DEBUG("IH: SDMA trap\n"); >> - switch (instance_id) { >> - case 0: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - case 1: >> - switch (queue_id) { >> - case 0: >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >> - break; >> - case 1: >> - /* XXX compute */ >> - break; >> - case 2: >> - /* XXX compute */ >> - break; >> - } >> - break; >> - } >> - return 0; >> -} >> - >> static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, >> struct amdgpu_irq_src *source, >> struct amdgpu_iv_entry *entry) >> @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) >> >> static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { >> .set = sdma_v3_0_set_trap_irq_state, >> - .process = sdma_v3_0_process_trap_irq, >> + .process = amdgpu_sdma_process_trap_irq, >> }; >> >> static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { >> -- >> 2.53.0 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:23 ` Christian König @ 2026-04-20 18:25 ` Alex Deucher 2026-04-20 18:31 ` Christian König 0 siblings, 1 reply; 8+ messages in thread From: Alex Deucher @ 2026-04-20 18:25 UTC (permalink / raw) To: Christian König Cc: Giovanna Uchoa, alexander.deucher, airlied, simona, amd-gfx, dri-devel On Mon, Apr 20, 2026 at 2:23 PM Christian König <christian.koenig@amd.com> wrote: > > Wait a second Alex, this patch actually doesn't make sense at all. > > The code is only common for a subset of SDMA engines and so shouldn't be moved into a common handler. It still reduces duplication across the chips where it is shared. I can kind of go either way on this. Alex > > Regards, > Christian. > > On 4/20/26 20:18, Alex Deucher wrote: > > Applied. Thanks! > > > > Alex > > > > On Mon, Apr 20, 2026 at 9:09 AM Giovanna Uchoa <giovannauchoa@usp.br> wrote: > >> > >> Move the amdgpu_sdma_process_trap_irq handler from version-specific > >> implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA > >> module (amdgpu_sdma). This eliminates code duplication and centralizes > >> the trap interrupt handling logic, which is identical across all SDMA > >> versions. > >> > >> Update the trap_irq_funcs in each version-specific module to reference > >> the common handler implementation. > >> > >> Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> > >> --- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ > >> drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- > >> drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- > >> drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- > >> 5 files changed, 47 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >> index 321310ba2..4f15334ce 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >> @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > >> return 0; > >> } > >> > >> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > >> + struct amdgpu_irq_src *source, > >> + struct amdgpu_iv_entry *entry) > >> +{ > >> + u8 instance_id, queue_id; > >> + > >> + instance_id = (entry->ring_id & 0x3) >> 0; > >> + queue_id = (entry->ring_id & 0xc) >> 2; > >> + DRM_DEBUG("IH: SDMA trap\n"); > >> + switch (instance_id) { > >> + case 0: > >> + switch (queue_id) { > >> + case 0: > >> + amdgpu_fence_process(&adev->sdma.instance[0].ring); > >> + break; > >> + case 1: > >> + /* XXX compute */ > >> + break; > >> + case 2: > >> + /* XXX compute */ > >> + break; > >> + } > >> + break; > >> + case 1: > >> + switch (queue_id) { > >> + case 0: > >> + amdgpu_fence_process(&adev->sdma.instance[1].ring); > >> + break; > >> + case 1: > >> + /* XXX compute */ > >> + break; > >> + case 2: > >> + /* XXX compute */ > >> + break; > >> + } > >> + break; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) > >> { > >> uint16_t version_major; > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >> index 2bf365609..ca4fd94ac 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >> @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, > >> int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > >> struct amdgpu_irq_src *source, > >> struct amdgpu_iv_entry *entry); > >> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > >> + struct amdgpu_irq_src *source, > >> + struct amdgpu_iv_entry *entry); > >> int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, > >> bool duplicate); > >> void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, > >> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >> index 120da838a..1bf1af633 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >> @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, > >> return 0; > >> } > >> > >> -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, > >> - struct amdgpu_irq_src *source, > >> - struct amdgpu_iv_entry *entry) > >> -{ > >> - u8 instance_id, queue_id; > >> - > >> - instance_id = (entry->ring_id & 0x3) >> 0; > >> - queue_id = (entry->ring_id & 0xc) >> 2; > >> - DRM_DEBUG("IH: SDMA trap\n"); > >> - switch (instance_id) { > >> - case 0: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - case 1: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - } > >> - > >> - return 0; > >> -} > >> - > >> static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, > >> struct amdgpu_irq_src *source, > >> struct amdgpu_iv_entry *entry) > >> @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) > >> > >> static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { > >> .set = cik_sdma_set_trap_irq_state, > >> - .process = cik_sdma_process_trap_irq, > >> + .process = amdgpu_sdma_process_trap_irq, > >> }; > >> > >> static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { > >> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >> index 93ec52c1f..545077897 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >> @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, > >> return 0; > >> } > >> > >> -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, > >> - struct amdgpu_irq_src *source, > >> - struct amdgpu_iv_entry *entry) > >> -{ > >> - u8 instance_id, queue_id; > >> - > >> - instance_id = (entry->ring_id & 0x3) >> 0; > >> - queue_id = (entry->ring_id & 0xc) >> 2; > >> - DRM_DEBUG("IH: SDMA trap\n"); > >> - switch (instance_id) { > >> - case 0: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - case 1: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - } > >> - return 0; > >> -} > >> - > >> static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, > >> struct amdgpu_irq_src *source, > >> struct amdgpu_iv_entry *entry) > >> @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) > >> > >> static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { > >> .set = sdma_v2_4_set_trap_irq_state, > >> - .process = sdma_v2_4_process_trap_irq, > >> + .process = amdgpu_sdma_process_trap_irq, > >> }; > >> > >> static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { > >> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >> index 3fde9be74..b3eab4e11 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >> @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, > >> return 0; > >> } > >> > >> -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, > >> - struct amdgpu_irq_src *source, > >> - struct amdgpu_iv_entry *entry) > >> -{ > >> - u8 instance_id, queue_id; > >> - > >> - instance_id = (entry->ring_id & 0x3) >> 0; > >> - queue_id = (entry->ring_id & 0xc) >> 2; > >> - DRM_DEBUG("IH: SDMA trap\n"); > >> - switch (instance_id) { > >> - case 0: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - case 1: > >> - switch (queue_id) { > >> - case 0: > >> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >> - break; > >> - case 1: > >> - /* XXX compute */ > >> - break; > >> - case 2: > >> - /* XXX compute */ > >> - break; > >> - } > >> - break; > >> - } > >> - return 0; > >> -} > >> - > >> static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, > >> struct amdgpu_irq_src *source, > >> struct amdgpu_iv_entry *entry) > >> @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) > >> > >> static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { > >> .set = sdma_v3_0_set_trap_irq_state, > >> - .process = sdma_v3_0_process_trap_irq, > >> + .process = amdgpu_sdma_process_trap_irq, > >> }; > >> > >> static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { > >> -- > >> 2.53.0 > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:25 ` Alex Deucher @ 2026-04-20 18:31 ` Christian König 2026-04-20 18:35 ` Alex Deucher 0 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2026-04-20 18:31 UTC (permalink / raw) To: Alex Deucher Cc: Giovanna Uchoa, alexander.deucher, airlied, simona, amd-gfx, dri-devel On 4/20/26 20:25, Alex Deucher wrote: > On Mon, Apr 20, 2026 at 2:23 PM Christian König > <christian.koenig@amd.com> wrote: >> >> Wait a second Alex, this patch actually doesn't make sense at all. >> >> The code is only common for a subset of SDMA engines and so shouldn't be moved into a common handler. > > It still reduces duplication across the chips where it is shared. I > can kind of go either way on this. Well it was your decision when we started amdgpu that we don't want to do this and as far as I can see it is still the right thing to do. We have avoided tons of issues with that approach since the code is independent per generation and modifications doesn't automatically affect all of them. I mean we can clearly cleanup the code, the DRM_DEBUG() is just superflous since we have a general tracepoint for IRQs and I think we can reduce the switch case to just an if. E.g. something like: u8 instance_id, queue_id; instance_id = (entry->ring_id & 0x3) >> 0; queue_id = (entry->ring_id & 0xc) >> 2; if (instance_id <= 1 && queue_id == 0) amdgpu_fence_process(&adev->sdma.instance[instance_id].ring); Regards, Christian. > > Alex > >> >> Regards, >> Christian. >> >> On 4/20/26 20:18, Alex Deucher wrote: >>> Applied. Thanks! >>> >>> Alex >>> >>> On Mon, Apr 20, 2026 at 9:09 AM Giovanna Uchoa <giovannauchoa@usp.br> wrote: >>>> >>>> Move the amdgpu_sdma_process_trap_irq handler from version-specific >>>> implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA >>>> module (amdgpu_sdma). This eliminates code duplication and centralizes >>>> the trap interrupt handling logic, which is identical across all SDMA >>>> versions. >>>> >>>> Update the trap_irq_funcs in each version-specific module to reference >>>> the common handler implementation. >>>> >>>> Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ >>>> drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- >>>> drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- >>>> drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- >>>> 5 files changed, 47 insertions(+), 124 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >>>> index 321310ba2..4f15334ce 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c >>>> @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, >>>> return 0; >>>> } >>>> >>>> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, >>>> + struct amdgpu_irq_src *source, >>>> + struct amdgpu_iv_entry *entry) >>>> +{ >>>> + u8 instance_id, queue_id; >>>> + >>>> + instance_id = (entry->ring_id & 0x3) >> 0; >>>> + queue_id = (entry->ring_id & 0xc) >> 2; >>>> + DRM_DEBUG("IH: SDMA trap\n"); >>>> + switch (instance_id) { >>>> + case 0: >>>> + switch (queue_id) { >>>> + case 0: >>>> + amdgpu_fence_process(&adev->sdma.instance[0].ring); >>>> + break; >>>> + case 1: >>>> + /* XXX compute */ >>>> + break; >>>> + case 2: >>>> + /* XXX compute */ >>>> + break; >>>> + } >>>> + break; >>>> + case 1: >>>> + switch (queue_id) { >>>> + case 0: >>>> + amdgpu_fence_process(&adev->sdma.instance[1].ring); >>>> + break; >>>> + case 1: >>>> + /* XXX compute */ >>>> + break; >>>> + case 2: >>>> + /* XXX compute */ >>>> + break; >>>> + } >>>> + break; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) >>>> { >>>> uint16_t version_major; >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >>>> index 2bf365609..ca4fd94ac 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h >>>> @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, >>>> int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, >>>> struct amdgpu_irq_src *source, >>>> struct amdgpu_iv_entry *entry); >>>> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, >>>> + struct amdgpu_irq_src *source, >>>> + struct amdgpu_iv_entry *entry); >>>> int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, >>>> bool duplicate); >>>> void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >>>> index 120da838a..1bf1af633 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c >>>> @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, >>>> return 0; >>>> } >>>> >>>> -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, >>>> - struct amdgpu_irq_src *source, >>>> - struct amdgpu_iv_entry *entry) >>>> -{ >>>> - u8 instance_id, queue_id; >>>> - >>>> - instance_id = (entry->ring_id & 0x3) >> 0; >>>> - queue_id = (entry->ring_id & 0xc) >> 2; >>>> - DRM_DEBUG("IH: SDMA trap\n"); >>>> - switch (instance_id) { >>>> - case 0: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - case 1: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - } >>>> - >>>> - return 0; >>>> -} >>>> - >>>> static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, >>>> struct amdgpu_irq_src *source, >>>> struct amdgpu_iv_entry *entry) >>>> @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) >>>> >>>> static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { >>>> .set = cik_sdma_set_trap_irq_state, >>>> - .process = cik_sdma_process_trap_irq, >>>> + .process = amdgpu_sdma_process_trap_irq, >>>> }; >>>> >>>> static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >>>> index 93ec52c1f..545077897 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c >>>> @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, >>>> return 0; >>>> } >>>> >>>> -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, >>>> - struct amdgpu_irq_src *source, >>>> - struct amdgpu_iv_entry *entry) >>>> -{ >>>> - u8 instance_id, queue_id; >>>> - >>>> - instance_id = (entry->ring_id & 0x3) >> 0; >>>> - queue_id = (entry->ring_id & 0xc) >> 2; >>>> - DRM_DEBUG("IH: SDMA trap\n"); >>>> - switch (instance_id) { >>>> - case 0: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - case 1: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - } >>>> - return 0; >>>> -} >>>> - >>>> static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, >>>> struct amdgpu_irq_src *source, >>>> struct amdgpu_iv_entry *entry) >>>> @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) >>>> >>>> static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { >>>> .set = sdma_v2_4_set_trap_irq_state, >>>> - .process = sdma_v2_4_process_trap_irq, >>>> + .process = amdgpu_sdma_process_trap_irq, >>>> }; >>>> >>>> static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >>>> index 3fde9be74..b3eab4e11 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c >>>> @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, >>>> return 0; >>>> } >>>> >>>> -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, >>>> - struct amdgpu_irq_src *source, >>>> - struct amdgpu_iv_entry *entry) >>>> -{ >>>> - u8 instance_id, queue_id; >>>> - >>>> - instance_id = (entry->ring_id & 0x3) >> 0; >>>> - queue_id = (entry->ring_id & 0xc) >> 2; >>>> - DRM_DEBUG("IH: SDMA trap\n"); >>>> - switch (instance_id) { >>>> - case 0: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - case 1: >>>> - switch (queue_id) { >>>> - case 0: >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); >>>> - break; >>>> - case 1: >>>> - /* XXX compute */ >>>> - break; >>>> - case 2: >>>> - /* XXX compute */ >>>> - break; >>>> - } >>>> - break; >>>> - } >>>> - return 0; >>>> -} >>>> - >>>> static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, >>>> struct amdgpu_irq_src *source, >>>> struct amdgpu_iv_entry *entry) >>>> @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) >>>> >>>> static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { >>>> .set = sdma_v3_0_set_trap_irq_state, >>>> - .process = sdma_v3_0_process_trap_irq, >>>> + .process = amdgpu_sdma_process_trap_irq, >>>> }; >>>> >>>> static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { >>>> -- >>>> 2.53.0 >>>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:31 ` Christian König @ 2026-04-20 18:35 ` Alex Deucher 0 siblings, 0 replies; 8+ messages in thread From: Alex Deucher @ 2026-04-20 18:35 UTC (permalink / raw) To: Christian König Cc: Giovanna Uchoa, alexander.deucher, airlied, simona, amd-gfx, dri-devel On Mon, Apr 20, 2026 at 2:31 PM Christian König <christian.koenig@amd.com> wrote: > > On 4/20/26 20:25, Alex Deucher wrote: > > On Mon, Apr 20, 2026 at 2:23 PM Christian König > > <christian.koenig@amd.com> wrote: > >> > >> Wait a second Alex, this patch actually doesn't make sense at all. > >> > >> The code is only common for a subset of SDMA engines and so shouldn't be moved into a common handler. > > > > It still reduces duplication across the chips where it is shared. I > > can kind of go either way on this. > > Well it was your decision when we started amdgpu that we don't want to do this and as far as I can see it is still the right thing to do. > > We have avoided tons of issues with that approach since the code is independent per generation and modifications doesn't automatically affect all of them. > Fair enough. Patch dropped. Alex > I mean we can clearly cleanup the code, the DRM_DEBUG() is just superflous since we have a general tracepoint for IRQs and I think we can reduce the switch case to just an if. E.g. something like: > > u8 instance_id, queue_id; > > instance_id = (entry->ring_id & 0x3) >> 0; > queue_id = (entry->ring_id & 0xc) >> 2; > if (instance_id <= 1 && queue_id == 0) > amdgpu_fence_process(&adev->sdma.instance[instance_id].ring); > > Regards, > Christian. > > > > > Alex > > > >> > >> Regards, > >> Christian. > >> > >> On 4/20/26 20:18, Alex Deucher wrote: > >>> Applied. Thanks! > >>> > >>> Alex > >>> > >>> On Mon, Apr 20, 2026 at 9:09 AM Giovanna Uchoa <giovannauchoa@usp.br> wrote: > >>>> > >>>> Move the amdgpu_sdma_process_trap_irq handler from version-specific > >>>> implementations (cik_sdma, sdma_v2_4, sdma_v3_0) to the common SDMA > >>>> module (amdgpu_sdma). This eliminates code duplication and centralizes > >>>> the trap interrupt handling logic, which is identical across all SDMA > >>>> versions. > >>>> > >>>> Update the trap_irq_funcs in each version-specific module to reference > >>>> the common handler implementation. > >>>> > >>>> Signed-off-by: Giovanna Uchoa <giovannauchoa@usp.br> > >>>> --- > >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 41 ++++++++++++++++++++++ > >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 ++ > >>>> drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 43 +----------------------- > >>>> drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 42 +---------------------- > >>>> drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 42 +---------------------- > >>>> 5 files changed, 47 insertions(+), 124 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >>>> index 321310ba2..4f15334ce 100644 > >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c > >>>> @@ -147,6 +147,47 @@ int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > >>>> return 0; > >>>> } > >>>> > >>>> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > >>>> + struct amdgpu_irq_src *source, > >>>> + struct amdgpu_iv_entry *entry) > >>>> +{ > >>>> + u8 instance_id, queue_id; > >>>> + > >>>> + instance_id = (entry->ring_id & 0x3) >> 0; > >>>> + queue_id = (entry->ring_id & 0xc) >> 2; > >>>> + DRM_DEBUG("IH: SDMA trap\n"); > >>>> + switch (instance_id) { > >>>> + case 0: > >>>> + switch (queue_id) { > >>>> + case 0: > >>>> + amdgpu_fence_process(&adev->sdma.instance[0].ring); > >>>> + break; > >>>> + case 1: > >>>> + /* XXX compute */ > >>>> + break; > >>>> + case 2: > >>>> + /* XXX compute */ > >>>> + break; > >>>> + } > >>>> + break; > >>>> + case 1: > >>>> + switch (queue_id) { > >>>> + case 0: > >>>> + amdgpu_fence_process(&adev->sdma.instance[1].ring); > >>>> + break; > >>>> + case 1: > >>>> + /* XXX compute */ > >>>> + break; > >>>> + case 2: > >>>> + /* XXX compute */ > >>>> + break; > >>>> + } > >>>> + break; > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static int amdgpu_sdma_init_inst_ctx(struct amdgpu_sdma_instance *sdma_inst) > >>>> { > >>>> uint16_t version_major; > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >>>> index 2bf365609..ca4fd94ac 100644 > >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h > >>>> @@ -203,6 +203,9 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev, > >>>> int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev, > >>>> struct amdgpu_irq_src *source, > >>>> struct amdgpu_iv_entry *entry); > >>>> +int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, > >>>> + struct amdgpu_irq_src *source, > >>>> + struct amdgpu_iv_entry *entry); > >>>> int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance, > >>>> bool duplicate); > >>>> void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev, > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >>>> index 120da838a..1bf1af633 100644 > >>>> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c > >>>> @@ -1141,47 +1141,6 @@ static int cik_sdma_set_trap_irq_state(struct amdgpu_device *adev, > >>>> return 0; > >>>> } > >>>> > >>>> -static int cik_sdma_process_trap_irq(struct amdgpu_device *adev, > >>>> - struct amdgpu_irq_src *source, > >>>> - struct amdgpu_iv_entry *entry) > >>>> -{ > >>>> - u8 instance_id, queue_id; > >>>> - > >>>> - instance_id = (entry->ring_id & 0x3) >> 0; > >>>> - queue_id = (entry->ring_id & 0xc) >> 2; > >>>> - DRM_DEBUG("IH: SDMA trap\n"); > >>>> - switch (instance_id) { > >>>> - case 0: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - case 1: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - } > >>>> - > >>>> - return 0; > >>>> -} > >>>> - > >>>> static int cik_sdma_process_illegal_inst_irq(struct amdgpu_device *adev, > >>>> struct amdgpu_irq_src *source, > >>>> struct amdgpu_iv_entry *entry) > >>>> @@ -1270,7 +1229,7 @@ static void cik_sdma_set_ring_funcs(struct amdgpu_device *adev) > >>>> > >>>> static const struct amdgpu_irq_src_funcs cik_sdma_trap_irq_funcs = { > >>>> .set = cik_sdma_set_trap_irq_state, > >>>> - .process = cik_sdma_process_trap_irq, > >>>> + .process = amdgpu_sdma_process_trap_irq, > >>>> }; > >>>> > >>>> static const struct amdgpu_irq_src_funcs cik_sdma_illegal_inst_irq_funcs = { > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >>>> index 93ec52c1f..545077897 100644 > >>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c > >>>> @@ -1035,46 +1035,6 @@ static int sdma_v2_4_set_trap_irq_state(struct amdgpu_device *adev, > >>>> return 0; > >>>> } > >>>> > >>>> -static int sdma_v2_4_process_trap_irq(struct amdgpu_device *adev, > >>>> - struct amdgpu_irq_src *source, > >>>> - struct amdgpu_iv_entry *entry) > >>>> -{ > >>>> - u8 instance_id, queue_id; > >>>> - > >>>> - instance_id = (entry->ring_id & 0x3) >> 0; > >>>> - queue_id = (entry->ring_id & 0xc) >> 2; > >>>> - DRM_DEBUG("IH: SDMA trap\n"); > >>>> - switch (instance_id) { > >>>> - case 0: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - case 1: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - } > >>>> - return 0; > >>>> -} > >>>> - > >>>> static int sdma_v2_4_process_illegal_inst_irq(struct amdgpu_device *adev, > >>>> struct amdgpu_irq_src *source, > >>>> struct amdgpu_iv_entry *entry) > >>>> @@ -1159,7 +1119,7 @@ static void sdma_v2_4_set_ring_funcs(struct amdgpu_device *adev) > >>>> > >>>> static const struct amdgpu_irq_src_funcs sdma_v2_4_trap_irq_funcs = { > >>>> .set = sdma_v2_4_set_trap_irq_state, > >>>> - .process = sdma_v2_4_process_trap_irq, > >>>> + .process = amdgpu_sdma_process_trap_irq, > >>>> }; > >>>> > >>>> static const struct amdgpu_irq_src_funcs sdma_v2_4_illegal_inst_irq_funcs = { > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >>>> index 3fde9be74..b3eab4e11 100644 > >>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c > >>>> @@ -1373,46 +1373,6 @@ static int sdma_v3_0_set_trap_irq_state(struct amdgpu_device *adev, > >>>> return 0; > >>>> } > >>>> > >>>> -static int sdma_v3_0_process_trap_irq(struct amdgpu_device *adev, > >>>> - struct amdgpu_irq_src *source, > >>>> - struct amdgpu_iv_entry *entry) > >>>> -{ > >>>> - u8 instance_id, queue_id; > >>>> - > >>>> - instance_id = (entry->ring_id & 0x3) >> 0; > >>>> - queue_id = (entry->ring_id & 0xc) >> 2; > >>>> - DRM_DEBUG("IH: SDMA trap\n"); > >>>> - switch (instance_id) { > >>>> - case 0: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[0].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - case 1: > >>>> - switch (queue_id) { > >>>> - case 0: > >>>> - amdgpu_fence_process(&adev->sdma.instance[1].ring); > >>>> - break; > >>>> - case 1: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - case 2: > >>>> - /* XXX compute */ > >>>> - break; > >>>> - } > >>>> - break; > >>>> - } > >>>> - return 0; > >>>> -} > >>>> - > >>>> static int sdma_v3_0_process_illegal_inst_irq(struct amdgpu_device *adev, > >>>> struct amdgpu_irq_src *source, > >>>> struct amdgpu_iv_entry *entry) > >>>> @@ -1601,7 +1561,7 @@ static void sdma_v3_0_set_ring_funcs(struct amdgpu_device *adev) > >>>> > >>>> static const struct amdgpu_irq_src_funcs sdma_v3_0_trap_irq_funcs = { > >>>> .set = sdma_v3_0_set_trap_irq_state, > >>>> - .process = sdma_v3_0_process_trap_irq, > >>>> + .process = amdgpu_sdma_process_trap_irq, > >>>> }; > >>>> > >>>> static const struct amdgpu_irq_src_funcs sdma_v3_0_illegal_inst_irq_funcs = { > >>>> -- > >>>> 2.53.0 > >>>> > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Claude review: Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:18 ` Alex Deucher 2026-04-20 18:23 ` Christian König @ 2026-04-22 23:40 ` Claude Code Review Bot 2026-04-22 23:40 ` Claude Code Review Bot 2 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-04-22 23:40 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Correctness: OK for the three targeted drivers.** The removed code from cik_sdma.c, sdma_v2_4.c, and sdma_v3_0.c is identical to the new common implementation. The extraction is mechanically sound. **Issue 1 — Misleading scope and naming:** The new common function: ```c int amdgpu_sdma_process_trap_irq(struct amdgpu_device *adev, struct amdgpu_irq_src *source, struct amdgpu_iv_entry *entry) ``` is placed in the common `amdgpu_sdma.c` module and exported in `amdgpu_sdma.h`, giving it the appearance of a universal handler. But the logic is specific to pre-SOC15 hardware that uses `entry->ring_id` to encode instance and queue: ```c instance_id = (entry->ring_id & 0x3) >> 0; queue_id = (entry->ring_id & 0xc) >> 2; ``` Newer versions use completely different dispatch mechanisms: - **sdma_v4_0** uses `sdma_v4_0_irq_id_to_seq(entry->client_id)` and handles page queues with IP version checks - **sdma_v5_0** switches on `entry->client_id` (SOC15_IH_CLIENTID_SDMA0/SDMA1) and has MES guard logic - **sdma_v6_0** uses `entry->client_id` with SOC21 client IDs and a different ring_id encoding (`& 0xf` / `& 0xf0`) This isn't truly "consolidation" of a common pattern — it's extracting one of several incompatible variants. **Issue 2 — Incomplete consolidation even within the matching set:** The commit message says the handler "is identical across all SDMA versions," which is factually incorrect. It is only identical across 3 of ~9+ versions. If the author's intent was a partial consolidation, the commit message should state which versions share this logic and why the others differ. **Issue 3 — Minor style nit:** ```c instance_id = (entry->ring_id & 0x3) >> 0; ``` The `>> 0` is a no-op. While this existed in the original code and is simply being moved, a cleanup patch is a reasonable place to drop the superfluous shift. This is minor and optional. **Suggestions:** 1. Rename the function to something that conveys it is specific to the older generations, e.g., `amdgpu_sdma_process_trap_irq_legacy` or keep it static with a comment about which HW versions share it. 2. Fix the commit message to accurately describe which versions share this handler and which don't. 3. Consider whether `si_dma` (which has yet another different handler) was intentionally excluded or overlooked. 4. Optionally drop the `>> 0`. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
* Claude review: Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler 2026-04-20 18:18 ` Alex Deucher 2026-04-20 18:23 ` Christian König 2026-04-22 23:40 ` Claude review: " Claude Code Review Bot @ 2026-04-22 23:40 ` Claude Code Review Bot 2 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-04-22 23:40 UTC (permalink / raw) To: dri-devel-reviews Overall Series Review Subject: Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler Author: Alex Deucher <alexdeucher@gmail.com> Patches: 6 Reviewed: 2026-04-23T09:40:59.375025 --- This is a single-patch cleanup that consolidates the SDMA trap IRQ handler from three version-specific files (cik_sdma, sdma_v2_4, sdma_v3_0) into the common `amdgpu_sdma.c`. The three removed implementations are byte-for-byte identical (modulo a blank line before the `return 0`), so the deduplication is mechanically correct for the three targeted files. However, the patch has a significant **scope problem**: it only consolidates 3 out of ~9 SDMA versions that have their own `process_trap_irq` implementations. The remaining versions (sdma_v4_0, sdma_v4_4_2, sdma_v5_0, sdma_v5_2, sdma_v6_0, sdma_v7_0, sdma_v7_1, si_dma) are left untouched — and importantly, most of them have **different** trap IRQ logic (different instance/queue dispatch, client_id-based switching, MES checks, page queue handling, etc.). This means the "common" handler being added is really only common to the oldest SDMA generations and isn't a general-purpose handler at all. The function name `amdgpu_sdma_process_trap_irq` suggests it is *the* common SDMA trap handler, which is misleading. **Verdict**: The patch is functionally correct for the three drivers it touches, but the approach and naming are questionable. A function named `amdgpu_sdma_process_trap_irq` in the common SDMA module implies universality, but the hardcoded 2-instance / 3-queue structure is specific to CIK/v2.4/v3.0 era hardware. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-04-22 23:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-20 3:28 [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler Giovanna Uchoa 2026-04-20 18:18 ` Alex Deucher 2026-04-20 18:23 ` Christian König 2026-04-20 18:25 ` Alex Deucher 2026-04-20 18:31 ` Christian König 2026-04-20 18:35 ` Alex Deucher 2026-04-22 23:40 ` Claude review: " Claude Code Review Bot 2026-04-22 23:40 ` 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