Re: [PATCH 2/3] amdgpu/ih: Don't perturb HW registers when accessing soft IH ring

Tue, 16 Jun 2026 06:33:06 -0700 


On 16/06/2026 13:04, Timur Kristóf wrote:

On Tuesday, June 16, 2026 1:39:56 PM Central European Summer Time Tvrtko
Ursulin wrote:

On 13/05/2026 18:08, Timur Kristóf wrote:

The soft IH ring is implemented entirely in software.
We shouldn't read (or write) and HW registers when accessing it.

Signed-off-by: Timur Kristóf <[email protected]>
---

   drivers/gpu/drm/amd/amdgpu/ih_v6_0.c   | 7 +++++++
   drivers/gpu/drm/amd/amdgpu/ih_v6_1.c   | 7 +++++++
   drivers/gpu/drm/amd/amdgpu/ih_v7_0.c   | 7 +++++++
   drivers/gpu/drm/amd/amdgpu/navi10_ih.c | 4 ++++
   4 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c index 333e9c30c091..65e5d21753f9
100644
--- a/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
@@ -439,6 +439,10 @@ static u32 ih_v6_0_get_wptr(struct amdgpu_device
*adev,>
        struct amdgpu_ih_regs *ih_regs;
        
        wptr = le32_to_cpu(*ih->wptr_cpu);

+
+       if (ih == &adev->irq.ih_soft)
+               goto out;
+


Would it be feasible to move amdgpu_ih_funcs from device global into the
IH rings themselves? Then we could have soft IH ops and it would be very
clean.

Possibly also cleanup all the protoptyes to operate only on ih and not
the adev + ih pair.


Sure, we can do that in the future. The reason I chose not to do that is
because that feels like an extremely intrusive refactor across all GPU
generations with a high chance of introducing regressions and not much value
to end users.

For now I would like to just focus on making the current code work well with
minimal refactoring, ie. just get the current soft IH ring implementation to
work reliably with retry faults.

In the meantime if you have a good idea how to refactor this code to make it
cleaner without breaking it, I'm happy to listen and we can make a plan how to
do that in a future series.


I can play with that refactor later, now that Christian gave a green light.


For this patch - it looks good to me. I am somewhat confused by the 
endianess conversion in amdgpu_ih_ring_write() (why is that needed for a 
software ring?), but even if unecessary it can be improved later so:


Reviewed-by: Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko


        ih_regs = &ih->ih_regs;
        
        if (!REG_GET_FIELD(wptr, IH_RB_WPTR, RB_OVERFLOW))

@@ -514,6 +518,9 @@ static void ih_v6_0_set_rptr(struct amdgpu_device
*adev,>
   {

   
   	struct amdgpu_ih_regs *ih_regs;


+       if (ih == &adev->irq.ih_soft)
+               return;
+

        if (ih->use_doorbell) {
        
                /* XXX check if swapping is necessary on BE */
                *ih->rptr_cpu = ih->rptr;

diff --git a/drivers/gpu/drm/amd/amdgpu/ih_v6_1.c
b/drivers/gpu/drm/amd/amdgpu/ih_v6_1.c index 699c274d357e..9dbc20131410
100644
--- a/drivers/gpu/drm/amd/amdgpu/ih_v6_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/ih_v6_1.c
@@ -410,6 +410,10 @@ static u32 ih_v6_1_get_wptr(struct amdgpu_device
*adev,>
        struct amdgpu_ih_regs *ih_regs;
        
        wptr = le32_to_cpu(*ih->wptr_cpu);

+
+       if (ih == &adev->irq.ih_soft)
+               goto out;
+

        ih_regs = &ih->ih_regs;
        
        if (!REG_GET_FIELD(wptr, IH_RB_WPTR, RB_OVERFLOW))

@@ -481,6 +485,9 @@ static void ih_v6_1_irq_rearm(struct amdgpu_device
*adev,>
   static void ih_v6_1_set_rptr(struct amdgpu_device *adev,

   
   			       struct amdgpu_ih_ring *ih)
   
   {


+       if (ih == &adev->irq.ih_soft)
+               return;
+

        struct amdgpu_ih_regs *ih_regs;
        
        if (ih->use_doorbell) {

diff --git a/drivers/gpu/drm/amd/amdgpu/ih_v7_0.c
b/drivers/gpu/drm/amd/amdgpu/ih_v7_0.c index 6de9e87e04e1..bd332e8cc5bf
100644
--- a/drivers/gpu/drm/amd/amdgpu/ih_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/ih_v7_0.c
@@ -457,6 +457,10 @@ static u32 ih_v7_0_get_wptr(struct amdgpu_device
*adev,>
        struct amdgpu_ih_regs *ih_regs;
        
        wptr = le32_to_cpu(*ih->wptr_cpu);

+
+       if (ih == &adev->irq.ih_soft)
+               goto out;
+

        ih_regs = &ih->ih_regs;
        
        if (!REG_GET_FIELD(wptr, IH_RB_WPTR, RB_OVERFLOW))

@@ -527,6 +531,9 @@ static void ih_v7_0_set_rptr(struct amdgpu_device
*adev,>
   {

   
   	struct amdgpu_ih_regs *ih_regs;


+       if (ih == &adev->irq.ih_soft)
+               return;
+

        if (ih->use_doorbell) {
        
                /* XXX check if swapping is necessary on BE */
                *ih->rptr_cpu = ih->rptr;

diff --git a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c index 4cd325149b63..e7ed37bb48e0
100644
--- a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
@@ -417,6 +417,10 @@ static u32 navi10_ih_get_wptr(struct amdgpu_device
*adev,>
                 */
                
                wptr = le32_to_cpu(*ih->wptr_cpu);

+               if (ih == &adev->irq.ih_soft)
+                       goto out;
+
+

                if (!REG_GET_FIELD(wptr, IH_RB_WPTR, RB_OVERFLOW))
                
                        goto out;
        
        }





























					Reply via email to