From: Christian König <christian.koenig@amd.com>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: Giovanna Uchoa <giovannauchoa@usp.br>,
alexander.deucher@amd.com, airlied@gmail.com, simona@ffwll.ch,
amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/amdgpu: consolidate SDMA trap IRQ handler
Date: Mon, 20 Apr 2026 20:31:34 +0200 [thread overview]
Message-ID: <5d32ad1b-5788-44b1-bb21-5f37891b6c82@amd.com> (raw)
In-Reply-To: <CADnq5_MTSC4ehVTOaeJ2ui3LwD8=em1+Kgu7yCh68aLgXa1dKQ@mail.gmail.com>
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
>>>>
>>
next prev parent reply other threads:[~2026-04-20 18:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5d32ad1b-5788-44b1-bb21-5f37891b6c82@amd.com \
--to=christian.koenig@amd.com \
--cc=airlied@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=alexdeucher@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=giovannauchoa@usp.br \
--cc=simona@ffwll.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox