[PATCH] drm/amdgpu/sdma5.2: Update wptr registers as well as doorbell

[Alex Deucher]

On Tue, Jul 16, 2024 at 2:30 PM Friedrich Vock <friedrich.vock at gmx.de> wrote:
>
> On 16.07.24 17:54, Alex Deucher wrote:
> > We seem to have a case where SDMA will sometimes miss a doorbell
> > if GFX is entering the powergating state when the doorbell comes in.
> > To workaround this, we can update the wptr via MMIO, however,
> > this is only safe because we disallow gfxoff in begin_ring() for
> > SDMA 5.2 and then allow it again in end_ring().
> >
> > Enable this workaround while we are root causing the issue with
> > the HW team.
> >
> > Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/3440
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>
> Looks like it works for me.
> Tested-by: Friedrich Vock <friedrich.vock at gmx.de>
>
> Is there a particular reason you chose to still go with the doorbell
> path plus updating the wptr via MMIO instead of setting
> ring->use_doorbell to false? The workaround shipping in SteamOS does
> that - if that has some adverse effects or something like that we should
> probably stop :)

Either way would work I think.  I just wanted to call out in the patch
that any access to SDMA or GFX MMIO needs to be done while gfxoff is
disallowed (via ring begin_use in this case), otherwise, you will hang
if gfx is in the off state.  If you want to go with disabling the
doorbell, we should double check that there are not any other places
where we access MMIO registers directly in the !doorbell case.  I
don't think there are, but I didn't look too closely.

Alex

>
> Thanks,
> Friedrich
>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> > index 7e475d9b554e..3c37e3cd3cbf 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> > @@ -225,6 +225,14 @@ static void sdma_v5_2_ring_set_wptr(struct amdgpu_ring *ring)
> >               DRM_DEBUG("calling WDOORBELL64(0x%08x, 0x%016llx)\n",
> >                               ring->doorbell_index, ring->wptr << 2);
> >               WDOORBELL64(ring->doorbell_index, ring->wptr << 2);
> > +             /* SDMA seems to miss doorbells sometimes when powergating kicks in.
> > +              * Updating the wptr directly will wake it. This is only safe because
> > +              * we disallow gfxoff in begin_use() and then allow it again in end_use().
> > +              */
> > +             WREG32(sdma_v5_2_get_reg_offset(adev, ring->me, mmSDMA0_GFX_RB_WPTR),
> > +                    lower_32_bits(ring->wptr << 2));
> > +             WREG32(sdma_v5_2_get_reg_offset(adev, ring->me, mmSDMA0_GFX_RB_WPTR_HI),
> > +                    upper_32_bits(ring->wptr << 2));
> >       } else {
> >               DRM_DEBUG("Not using doorbell -- "
> >                               "mmSDMA%i_GFX_RB_WPTR == 0x%08x "
> > @@ -1707,6 +1715,10 @@ static void sdma_v5_2_ring_begin_use(struct amdgpu_ring *ring)
> >        * but it shouldn't hurt for other parts since
> >        * this GFXOFF will be disallowed anyway when SDMA is
> >        * active, this just makes it explicit.
> > +      * sdma_v5_2_ring_set_wptr() takes advantage of this
> > +      * to update the wptr because sometimes SDMA seems to miss
> > +      * doorbells when entering PG.  If you remove this, update
> > +      * sdma_v5_2_ring_set_wptr() as well!
> >        */
> >       amdgpu_gfx_off_ctrl(adev, false);
> >   }