On 15/07/2026 20:23, Timur Kristóf wrote:
On 2026. július 15., szerda 13:22:21 közép-európai nyári idő Tvrtko Ursulin
wrote:
On 15/07/2026 11:53, Timur Kristóf wrote:
On 2026. július 15., szerda 12:19:40 közép-európai nyári idő Tvrtko
Ursulin
wrote:
On 13/07/2026 14:07, Timur Kristóf wrote:
These were used without ever calling get()/put() on them.
Implement it like on GFX7-8:
Used as in how? Are they even enabled without this change and if not
then does this patch fixes something other than being prep work for soft
reset?
If you open gfx_v6_0.c and search for priv_reg or priv_inst, you can see
that the interrupts are used in the same manner as gfx7 and newer, but
without get() and put().
Yes, they are used in code. Are they used in reality was my question. :)
Looking at the code I think the original intention was to wire up these
interrupts. I think the hardware actually supports these interrupts and the
author of the code simply forgot to call get() and put().
If you are not convinced, maybe Alex or Christian can confirm whether the
interrupt actually exists on this HW generation. If it turns out it doesn't
exist then we should just delete this code.
I ask because it appears that without amdgpu_irq_get() they may not even
get enabled so never received. Yes or no? Consequences if yes?
Yes, that sounds correct. The consequence is that the interrupt will now be
enabled and the HW will tell us about some illegal register access and some
illegal instructions when it happens.
Right, AFAIU assuming the CP hangs, instead of waiting for the TDR
timeout with this patch it will be instant via drm_sched_fault() called
from either irq handler. Or if CP does not hang but somehow ends up
going over it with corruption with this patch it will insta GPU hang.
In any case I think it is needed to have a definitive answer whether the
interrupts were not enabled until now, and if so, also exercise the
until now unused code paths just to make sure it all works as expected.
Presumably there are test cases which trigger both conditions?
Then I would suggest putting this info in the commit message. That is,
instead of saying "These were used without ever calling get()/put() on
them." expand with the full story so the whole situation is clear both
before and after the patch.
Assuming interrupts were not enabled until now, that the unused irq
handlers work as expected, and with the improved commit message:
Reviewed-by: Tvrtko Ursulin <[email protected]>
Unless you don't have the hardware to test it and there are no existing
test cases to verify it?
Regards,
Tvrtko
To give you additional context: for the purpose of diagnosing issues related
to GPU hangs and recovery, all additional information helps. If the CP has
interrupts to tell us about some error cases, we should absolutely use that.
Side note: from the register definitions it looks like the HW actually supports
other interrupts which would be interesting to enable in the future.
In this patch I just wanted to fix up the two that were already in the code.
* Call amdgpu_irq_get() from gfx_v6_0_late_init()
* Call amdgpu_irq_put() from gfx_v6_0_hw_fini()
Signed-off-by: Timur Kristóf <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c index 5b570a4b5c01..1c7cd265fbca
100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
@@ -3131,6 +3131,22 @@ static int gfx_v6_0_early_init(struct
amdgpu_ip_block *ip_block)>
return 0;
}
+static int gfx_v6_0_late_init(struct amdgpu_ip_block *ip_block)
+{
+ struct amdgpu_device *adev = ip_block->adev;
+ int r;
+
+ r = amdgpu_irq_get(adev, &adev->gfx.priv_reg_irq, 0);
+ if (r)
+ return r;
+
+ r = amdgpu_irq_get(adev, &adev->gfx.priv_inst_irq, 0);
+ if (r)
+ return r;
+
+ return 0;
+}
+
static int gfx_v6_0_sw_init(struct amdgpu_ip_block *ip_block)
{
struct amdgpu_ring *ring;
@@ -3243,6 +3259,8 @@ static int gfx_v6_0_hw_fini(struct amdgpu_ip_block
*ip_block)>
{
struct amdgpu_device *adev = ip_block->adev;
+ amdgpu_irq_put(adev, &adev->gfx.priv_reg_irq, 0);
+ amdgpu_irq_put(adev, &adev->gfx.priv_inst_irq, 0);
gfx_v6_0_cp_enable(adev, false);
adev->gfx.rlc.funcs->stop(adev);
gfx_v6_0_fini_pg(adev);
@@ -3532,6 +3550,7 @@ static void gfx_v6_0_emit_mem_sync(struct
amdgpu_ring *ring)>
static const struct amd_ip_funcs gfx_v6_0_ip_funcs = {
.name = "gfx_v6_0",
.early_init = gfx_v6_0_early_init,
+ .late_init = gfx_v6_0_late_init,
.sw_init = gfx_v6_0_sw_init,
.sw_fini = gfx_v6_0_sw_fini,
.hw_init = gfx_v6_0_hw_init,
