[PATCH v2] drm/amd: Disable S/G for APUs when 64GB or more host memory

[Alex Deucher]

On Thu, Jul 27, 2023 at 2:14 PM Mario Limonciello
<mario.limonciello at amd.com> wrote:
>
> On 7/27/2023 13:02, Alex Deucher wrote:
> > On Thu, Jul 27, 2023 at 1:29 PM Mario Limonciello
> > <mario.limonciello at amd.com> wrote:
> >>
> >> Users report a white flickering screen on multiple systems that
> >> is tied to having 64GB or more memory.  When S/G is enabled pages
> >> will get pinned to both VRAM carve out and system RAM leading to
> >> this.
> >>
> >> Until it can be fixed properly, disable S/G when 64GB of memory or
> >> more is detected.  This will force pages to be pinned into VRAM.
> >> This should fix white screen flickers but if VRAM pressure is
> >> encountered may lead to black screens.  It's a trade-off for now.
> >>
> >> Fixes: 81d0bcf990093 ("drm/amdgpu: make display pinning more flexible (v2)")
> >> Cc: Hamza Mahfooz <Hamza.Mahfooz at amd.com>
> >> Cc: Roman Li <roman.li at amd.com>
> >> Cc: <stable at vger.kernel.org> # 6.1.y: bf0207e172703 ("drm/amdgpu: add S/G display parameter")
> >> Cc: <stable at vger.kernel.org> # 6.4.y
> >> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2735
> >> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> >> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> >> ---
> >> v1->v2:
> >>   * Fix updating mode_info as well
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 25 +++++++++++++++++++
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  5 ++--
> >>   3 files changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> index 93d0f4c7b560e..2e3c7c15cb8e3 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> @@ -1313,6 +1313,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> >>   void amdgpu_device_pci_config_reset(struct amdgpu_device *adev);
> >>   int amdgpu_device_pci_reset(struct amdgpu_device *adev);
> >>   bool amdgpu_device_need_post(struct amdgpu_device *adev);
> >> +bool amdgpu_sg_display_supported(struct amdgpu_device *adev);
> >>   bool amdgpu_device_pcie_dynamic_switching_supported(void);
> >>   bool amdgpu_device_should_use_aspm(struct amdgpu_device *adev);
> >>   bool amdgpu_device_aspm_support_quirk(void);
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index dc0e5227119b1..a4e36b178d86c 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -1296,6 +1296,31 @@ bool amdgpu_device_need_post(struct amdgpu_device *adev)
> >>          return true;
> >>   }
> >>
> >> +/*
> >> + * On APUs with >= 64GB white flickering has been observed w/ SG enabled.
> >> + * Disable S/G on such systems until we have a proper fix.
> >> + * https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> >> + * https://gitlab.freedesktop.org/drm/amd/-/issues/2735
> >> + */
> >> +bool amdgpu_sg_display_supported(struct amdgpu_device *adev)
> >> +{
> >> +       switch (amdgpu_sg_display) {
> >> +       case -1:
> >> +               break;
> >> +       case 0:
> >> +               return false;
> >> +       case 1:
> >> +               return true;
> >> +       default:
> >> +               return false;
> >> +       }
> >> +       if (totalram_pages() << (PAGE_SHIFT - 10) >= 64000000) {
> >
> > Does totalram_pages() return the amount of physical ram or the amount
> > of usable ram (i.e., minus carveout, firmware reservations, etc.)?
>
> It's a good question.  I don't have a system w/ 64GB in front of me so I
> just double checked on a Phoenix system that has 16GB soldered down.
>
> It returns 15520248, so it's missing the VRAM carveout.  amdgpu does
> know this already via adev->gmc.real_vram_size.
> So I think the logic should be:
>
> if ((totalram_pages() << (PAGE_SHIFT - 10)) + (adev->gmc.real_vram_size
> / 1024) >= 64000000)
>
> That should bring it to ~16GB.

Yeah, that's probably good enough.

Alex

>
> > Assuming it does the right thing here:
> > Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> >
> >
> >> +               DRM_WARN("Disabling S/G due to >=64GB RAM\n");
> >> +               return false;
> >> +       }
> >> +       return true;
> >> +}
> >> +
> >>   /*
> >>    * Intel hosts such as Raptor Lake and Sapphire Rapids don't support dynamic
> >>    * speed switching. Until we have confirmation from Intel that a specific host
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index 77d970a2ee693..26c3eb7a9f882 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -1639,9 +1639,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
> >>                  }
> >>                  break;
> >>          }
> >> -       if (init_data.flags.gpu_vm_support &&
> >> -           (amdgpu_sg_display == 0))
> >> -               init_data.flags.gpu_vm_support = false;
> >> +       if (init_data.flags.gpu_vm_support)
> >> +               init_data.flags.gpu_vm_support = amdgpu_sg_display_supported(adev);
> >>
> >>          if (init_data.flags.gpu_vm_support)
> >>                  adev->mode_info.gpu_vm_support = true;
> >> --
> >> 2.25.1
> >>
>