The code was trying to keep a strict limit on the amount of mob memory that was used in the guest by making it match the host settings. There's technically no reason to do that (guests can certainly use more than the host can have resident in renderers at the same time).

In particular this is problematic because our userspace is not great at handling OOM conditions and running out of MOB space results in GL apps crashing, e.g. gnome-shell likes to allocate huge surfaces (~61MB for the desktop on 2560x1600 with two workspaces) and running out of memory there means that the gnome-shell crashes on startup taking us back to the login and resulting in a system where one can not login in graphically anymore.

Instead of letting the userspace crash we can extend available MOB space, we just don't want to use all of the RAM for graphics, so we're going to limit it to half of RAM.

With the addition of some extra logging this should make the "guest has been configured with not enough graphics memory" errors a lot easier to diagnose in cases where the automatic expansion of MOB space fails.

Signed-off-by: Zack Rusin zackr@vmware.com Reviewed-by: Martin Krastev krastevm@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 8 ++++- drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 36 +++++++++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 359f2e6f3693..a9195c472b75 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -948,6 +948,13 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id) dev_priv->texture_max_height = 8192; dev_priv->max_primary_mem = dev_priv->vram_size; } + DRM_INFO("Legacy memory limits: VRAM = %llu kB, FIFO = %llu kB, surface = %u kB\n", + (u64)dev_priv->vram_size / 1024, + (u64)dev_priv->fifo_mem_size / 1024, + dev_priv->memory_size / 1024); + + DRM_INFO("MOB limits: max mob size = %u kB, max mob pages = %u\n", + dev_priv->max_mob_size / 1024, dev_priv->max_mob_pages);

vmw_print_capabilities(dev_priv->capabilities); if (dev_priv->capabilities & SVGA_CAP_CAP2_REGISTER) @@ -1094,7 +1101,6 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id) DRM_INFO("SM4_1 support available.\n"); if (dev_priv->sm_type == VMW_SM_4) DRM_INFO("SM4 support available.\n"); - DRM_INFO("Running without reservation semaphore\n");

vmw_host_printf("vmwgfx: Module Version: %d.%d.%d (kernel: %s)", VMWGFX_DRIVER_MAJOR, VMWGFX_DRIVER_MINOR, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c index 28ceb749a733..b2c4af331c9d 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c @@ -71,8 +71,40 @@ static int vmw_gmrid_man_get_node(struct ttm_resource_manager *man,

if (gman->max_gmr_pages > 0) { gman->used_gmr_pages += (*res)->num_pages; - if (unlikely(gman->used_gmr_pages > gman->max_gmr_pages)) - goto nospace; + /* + * Because the graphics memory is a soft limit we can try to + * expand it instead of letting the userspace apps crash. + * We're just going to have a sane limit (half of RAM) + * on the number of MOB's that we create and will try to keep + * the system running until we reach that. + */ + if (unlikely(gman->used_gmr_pages > gman->max_gmr_pages)) { + const unsigned long max_graphics_pages = totalram_pages() / 2; + uint32_t new_max_pages = 0; + + DRM_WARN("vmwgfx: mob memory overflow. Consider increasing guest RAM and graphicsMemory.\n"); + vmw_host_printf("vmwgfx, warning: mob memory overflow. Consider increasing guest RAM and graphicsMemory.\n"); + + if (gman->max_gmr_pages > (max_graphics_pages / 2)) { + DRM_WARN("vmwgfx: guest requires more than half of RAM for graphics.\n"); + new_max_pages = max_graphics_pages; + } else + new_max_pages = gman->max_gmr_pages * 2; + if (new_max_pages > gman->max_gmr_pages && new_max_pages >= gman->used_gmr_pages) { + DRM_WARN("vmwgfx: increasing guest mob limits to %u kB.\n", + ((new_max_pages) << (PAGE_SHIFT - 10))); + + gman->max_gmr_pages = new_max_pages; + } else { + char buf[256]; + snprintf(buf, sizeof(buf), + "vmwgfx, error: guest graphics is out of memory (mob limit at: %ukB).\n", + ((gman->max_gmr_pages) << (PAGE_SHIFT - 10))); + vmw_host_printf(buf); + DRM_WARN("%s", buf); + goto nospace; + } + } }

(*res)->start = id;

Hi

Am 14.07.21 um 06:14 schrieb Zack Rusin:

From: Martin Krastev krastevm@vmware.com

• Add support for CursorMob
• Add support for CursorBypass 4

Reviewed-by: Zack Rusin zackr@vmware.com Signed-off-by: Martin Krastev krastevm@vmware.com Signed-off-by: Zack Rusin zackr@vmware.com

---

drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 45 +++++++++++++++- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 6 +++ drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 79 +++++++++++++++++++++++++++-- 3 files changed, 125 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 086dc75e7b42..7d8cc2f6b04e 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 OR MIT /**************************************************************************

• • Copyright 2009-2016 VMware, Inc., Palo Alto, CA., USA

• • Copyright 2009-2021 VMware, Inc., Palo Alto, CA., USA
  • Permission is hereby granted, free of charge, to any person obtaining a
  • copy of this software and associated documentation files (the

@@ -301,8 +301,12 @@ static void vmw_print_capabilities2(uint32_t capabilities2) DRM_INFO(" Grow oTable.\n");

These macros have been out of fashion for a while. There's drm_info(), drm_warn(), drm_err(), etc as replacements. They also print device information. Applis here and for the rest of the patchset.

if (capabilities2 & SVGA_CAP2_INTRA_SURFACE_COPY) DRM_INFO(" IntraSurface copy.\n");

• if (capabilities2 & SVGA_CAP2_CURSOR_MOB)

• DRM_INFO("  Cursor Mob.\n");
  

  if (capabilities2 & SVGA_CAP2_DX3) DRM_INFO(" DX3.\n");

• if (capabilities2 & SVGA_CAP2_EXTRA_REGS)

• DRM_INFO("  Extra Regs.\n");
  

  }

  static void vmw_print_capabilities(uint32_t capabilities)

@@ -505,6 +509,7 @@ static int vmw_request_device_late(struct vmw_private *dev_priv) static int vmw_request_device(struct vmw_private *dev_priv) { int ret;

• size_t i;

  ret = vmw_device_init(dev_priv); if (unlikely(ret != 0)) {

@@ -526,6 +531,37 @@ static int vmw_request_device(struct vmw_private *dev_priv) if (unlikely(ret != 0)) goto out_no_query_bo;

• /* Set up mobs for cursor updates */
• if (dev_priv->has_mob && dev_priv->capabilities2 & SVGA_CAP2_CURSOR_MOB) {

• const uint32_t cursor_max_dim = vmw_read(dev_priv, SVGA_REG_CURSOR_MAX_DIMENSION);
  

• for (i = 0; i < ARRAY_SIZE(dev_priv->cursor_mob); i++) {
  

• 	struct ttm_buffer_object **const bo = &dev_priv->cursor_mob[i];
  

• 	ret = vmw_bo_create_kernel(dev_priv,
  

• 		cursor_max_dim * cursor_max_dim * sizeof(u32) + sizeof(SVGAGBCursorHeader),
  

• 		&vmw_mob_placement, bo);
  

• 	if (ret != 0) {
  

• 		DRM_ERROR("Unable to create CursorMob array.\n");
  

• 		break;
  

• 	}
  

• 	BUG_ON((*bo)->resource->mem_type != VMW_PL_MOB);
  

BUG_ON() crashes the kernel. The prefered way is to use drm_WARN_*() and return.

• 	/* Fence the mob creation so we are guarateed to have the mob */
  

• 	ret = ttm_bo_reserve(*bo, false, true, NULL);
  

• 	BUG_ON(ret);
  

I'm not quite sure, but this line is probably a no-go wrt to best practices. See the comment above.

• 	vmw_bo_fence_single(*bo, NULL);
  

• 	ttm_bo_unreserve(*bo);
  

• 	DRM_INFO("Using CursorMob mobid %lu, max dimension %u\n",
  

• 		 (*bo)->resource->start, cursor_max_dim);
  

IIRC anything *_info() is just radom info into the log. Most of the time, no one cares. Better use one of the drm_dbg_() calls.

• }
  

• }

• return 0;

  out_no_query_bo:

@@ -556,6 +592,8 @@ static int vmw_request_device(struct vmw_private *dev_priv) */ static void vmw_release_device_early(struct vmw_private *dev_priv) {

• size_t i;
• /*
  • Previous destructions should've released
  • the pinned bo.

@@ -570,6 +608,11 @@ static void vmw_release_device_early(struct vmw_private *dev_priv) if (dev_priv->has_mob) { struct ttm_resource_manager *man;

• for (i = 0; i < ARRAY_SIZE(dev_priv->cursor_mob); i++) {
  

• 	if (dev_priv->cursor_mob[i] != NULL)
  

• 		ttm_bo_put(dev_priv->cursor_mob[i]);
  

• }
  

• man = ttm_manager_type(&dev_priv->bdev, VMW_PL_MOB); ttm_resource_manager_evict_all(&dev_priv->bdev, man); vmw_otables_takedown(dev_priv);

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 356f82c26f59..46bf54f6169a 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -642,6 +642,12 @@ struct vmw_private { u8 mksstat_kern_top_timer[MKSSTAT_CAPACITY]; atomic_t mksstat_kern_pids[MKSSTAT_CAPACITY]; #endif

• /*

• * CursorMob buffer objects
  

• */
  

• struct ttm_buffer_object *cursor_mob[2];
• atomic_t cursor_mob_idx;

That's something like page-flipping with alternating BO's and shadow buffering?

You really want a cursor plane to hold this information.

I briefly looked through vmwgfx and it has all these fail-able code in its atomic-update path. The patches here only make things worse. With cursor planes, you can do all the preparation in atomic_check and prepare_fb, and store the intermediate state/mappings/etc in the plane state.

The ast driver started with a design like this one here and then we moved it to cursor planes. Ast has now none of the mentioned problems. Relevant code is at [1][2].

Best regards Thomas

[1] https://elixir.bootlin.com/linux/v5.13.1/source/drivers/gpu/drm/ast/ast_drv....

[2] https://elixir.bootlin.com/linux/v5.13.1/source/drivers/gpu/drm/ast/ast_mode...