On Thu, Jul 16, 2026 at 8:24 AM Tvrtko Ursulin <[email protected]> wrote: > > > 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: >
The interrupts would not have been enabled if get() was never called. I think they were never enabled because radeon didn't implement support and this code was ported from radeon. > 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? An example test would be attempting to access a privileged register from a user IB. Alex > > 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, > > > > > > > > >
