Re: [PATCH] drm/amdgpu/gmc_v8_0: restore the FB location after a re-POST

["Christian König" <christian.koenig@amd.com>]

On 9/24/26 15:29, Francisco Beltrán Millalén wrote:
> On a MacBookPro14,3 (Radeon Pro 560, POLARIS11) amdgpu has never recovered
> from an ASIC reset: five attempts recorded, zero successes.  Since suspend
> to RAM goes through a reset, S3 fails the same way, and as the internal
> panel hangs off the AMD GPU the machine comes back blind.
> 
> The failure looks like VRAM going write-only-dead: writes are silently
> dropped while reads still work, the driver reports success at every step,
> and then it hands the SMU a pointer to a table that was never written:
> 
>   amdgpu_device_asic_init()  -> 0          (reports success)
>   gmc_v8_0_hw_init()         -> 0          (reports success)
>   memcpy_toio()                             (write silently discarded)
>   send_msg(0x251, ...)                      (SMU parses garbage)
>   smu7_check_fw_load_finish() -> -EINVAL -> black screen
> 
> It is not VRAM dying.  It is the framebuffer moving.
> 
> On this machine the Apple firmware places VRAM at MC address 0 on a cold
> boot, and gmc_v8_0_vram_gtt_location() reads MC_VM_FB_LOCATION once, at
> init, to derive vram_start.  A re-POST -- which is what an ASIC reset and
> an S3 resume both trigger -- lets the VBIOS put the framebuffer back at
> its own default instead, 0xf400_0000 here:
> 
>   cold boot:   MC_VM_FB_LOCATION = 0x007f0000
>   after reset: MC_VM_FB_LOCATION = 0xf47ff400

Mhm, interesting I'm really wondering where those values come from.

> 
> gmc_v8_0_mc_program() programs the system aperture from the stale
> vram_start, but only writes MC_VM_FB_LOCATION and HDP_NONSURFACE_BASE
> under SR-IOV; on bare metal it trusts whatever the VBIOS left behind.
> While the MC is still in pass-through everything appears to work, so the
> mismatch goes unnoticed.  Then gmc_v8_0_gart_enable() sets ENABLE_L1_TLB,
> SYSTEM_ACCESS_MODE=3 and ENABLE_ADVANCED_DRIVER_MODEL, the MC starts
> checking the system aperture, and every access lands outside it -- which
> is why reads return data written before the reset, from a different
> physical place than the writes are going to.
> 
> Write the framebuffer location back when it does not match the one the
> driver is working with, which is what the SR-IOV path already does.  The
> comparison keeps this a no-op on machines where the VBIOS restores the
> same location, so nothing changes for them.

That is still a rather bad idea for multiple reasons.

You often run into suspend/resume and random memory corruption issues when stuff like that is done and we never fully implemented blocking VRAM access during a runtime ASIC reset.

I think the more defensive approach is to do an ASIC reset on driver load and use the values the AtomBIOS init function comes up with.

@Alex what's your take here?

Regards,
Christian.

> 
> This runs after the VGA aperture has been locked out and with the display
> suspended, so the MC does not need to be stopped; only CPU access through
> the BAR could land while the FB and HDP bases disagree, so BIF_FB_EN is
> cleared around the update and re-enabled below.
> 
> With this the GPU survives resets and S3: the machine has since completed
> twelve suspend/resume cycles in a single boot without a failure, and the
> restore is visible on each resume:
> 
>   amdgpu 0000:01:00.0: amdgpu: FB location 0xf47ff400 does not match
>   vram_start, restoring 0x007f0000
> 
> To be precise about what those cycles prove: the kernel they were run on
> also carries unrelated local patches for this machine's Thunderbolt
> controller, which fails separately.  This patch is the one that brings the
> display back -- without it the GPU never recovered from a reset at all.
> 
> Tested on 6.18.49 on a MacBookPro14,3.  I have no other smu7 hardware, so
> this is only known to matter on machines whose firmware boots the GPU at a
> different framebuffer location than the VBIOS default; elsewhere the new
> branch does nothing.
> 
> Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
> ---
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -472,14 +472,42 @@
>         WREG32(mmMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR,
>                adev->mem_scratch.gpu_addr >> 12);
> 
> +       tmp = ((adev->gmc.vram_end >> 24) & 0xFFFF) << 16;
> +       tmp |= ((adev->gmc.vram_start >> 24) & 0xFFFF);
> +
>         if (amdgpu_sriov_vf(adev)) {
> -               tmp = ((adev->gmc.vram_end >> 24) & 0xFFFF) << 16;
> -               tmp |= ((adev->gmc.vram_start >> 24) & 0xFFFF);
>                 WREG32(mmMC_VM_FB_LOCATION, tmp);
>                 /* XXX double check these! */
>                 WREG32(mmHDP_NONSURFACE_BASE, (adev->gmc.vram_start >> 8));
>                 WREG32(mmHDP_NONSURFACE_INFO, (2 << 7) | (1 << 30));
>                 WREG32(mmHDP_NONSURFACE_SIZE, 0x3FFFFFFF);
> +       } else {
> +               u32 fb_loc = RREG32(mmMC_VM_FB_LOCATION);
> +
> +               /*
> +                * On bare metal vram_start is the FB base found at init (see
> +                * gmc_v8_0_vram_gtt_location()).  Normally the VBIOS put it
> +                * there and a later re-POST puts it back in the same place.
> +                * On MacBookPros with switchable graphics VRAM is at 0 at boot
> +                * instead, and a re-POST (S3 resume, ASIC reset) moves it to
> +                * the VBIOS default, away from the addresses the driver
> +                * already uses.  Move it back.
> +                *
> +                * This only happens after a re-POST: the display is suspended
> +                * and the VGA aperture has been locked out above, so there is
> +                * no need to stop the MC.  Only CPU access through the BAR
> +                * could land while the FB and HDP bases disagree, so block it
> +                * here; BIF_FB_EN is enabled again below.
> +                */
> +               if (REG_GET_FIELD(fb_loc, MC_VM_FB_LOCATION, FB_BASE) !=
> +                   REG_GET_FIELD(tmp, MC_VM_FB_LOCATION, FB_BASE)) {
> +                       dev_info(adev->dev,
> +                                "FB location 0x%08x does not match vram_start, restoring 0x%08x\n",
> +                                fb_loc, tmp);
> +                       WREG32(mmBIF_FB_EN, 0);
> +                       WREG32(mmMC_VM_FB_LOCATION, tmp);
> +                       WREG32(mmHDP_NONSURFACE_BASE, (adev->gmc.vram_start >> 8));
> +               }
>         }
> 
>         WREG32(mmMC_VM_AGP_BASE, 0);