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

["Timur Kristóf" <timur.kristof@gmail.com>]

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 <timur.kristof@gmail.com>
> > ---
> > 
> >   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.

Thanks,
Timur




> 
> >   	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;
> >   	
> >   	}