| Message ID | 20240201051343.9936-1-ian.forbes@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vmwgfx: Filter modes which exceed 3/4 of graphics memory. | expand |
On Fri, Feb 2, 2024 at 11:58 AM Ian Forbes <ian.forbes@broadcom.com> wrote: > > SVGA requires surfaces to fit within graphics memory (max_mob_pages) which > means that modes with a final buffer size that would exceed graphics memory > must be pruned otherwise creation will fail. Sorry, I didn't notice this originally but that's not quite true. svga doesn't require all mob memory to stay within max_mob_pages (which is SVGA_REG_GBOBJECT_MEM_SIZE_KB). max_mob_pages is really max resident memory or suggested-guest-memory-for-best-performance. we can grow that memory (and we do). I think what's causing problems on systems with low memory is that cursor mobs and the fb's need to be both resident but can't. Now SVGA_REG_MAX_PRIMARY_MEM is the max memory in which our topology needs to fit in (which is max_primary_mem on vmwgfx) but afaict that's not the issue here and it's checked later in vmw_kms_validate_mode_vram > Additionally, device commands which use multiple graphics resources must > have all their resources fit within graphics memory for the duration of the > command. Thus we need a small carve out of 1/4 of graphics memory to ensure > commands likes surface copies to the primary framebuffer for cursor > composition or damage clips can fit within graphics memory. Yes, we should probably rename max_mob_pages to max_resident_memory instead to make this obvious. > This fixes an issue where VMs with low graphics memory (< 64MiB) configured > with high resolution mode boot to a black screen because surface creation > fails. Does this work if you disable gbobjects? Without gbobject's we won't have screen targets and thus won't be offsetting by 1/4 so I wonder if 4mb vram with legacy display would work with 1280x800 resolution. Also, you want to add a "V2" section to your change to describe what changed in v2 vs v1 (and same for any subsequent change). > > Signed-off-by: Ian Forbes <ian.forbes@broadcom.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index cd4925346ed4..84e1b765cda3 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -2858,12 +2858,17 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, > struct vmw_private *dev_priv = vmw_priv(dev); > u32 max_width = dev_priv->texture_max_width; > u32 max_height = dev_priv->texture_max_height; > - u32 assumed_cpp = 4; > - > - if (dev_priv->assume_16bpp) > - assumed_cpp = 2; > + u32 assumed_cpp = dev_priv->assume_16bpp ? 2 : 4; > + u32 pitch = mode->hdisplay * assumed_cpp; > + u64 total = mode->vdisplay * pitch; > + bool using_stdu = dev_priv->active_display_unit == vmw_du_screen_target; > + u64 max_mem_for_st = dev_priv->max_mob_pages * PAGE_SIZE * 3 / 4; > + /* ^^^ Max memory for the mode fb when using Screen Target / MOBs. > + * We need a carveout (1/4) to account for other gfx resources that are > + * required in gfx mem for an fb update to complete with low gfx mem (<64MiB). > + */ Same wording issue as mentioned above and lets use normal comment style (i.e. comments attach to the code below). max_mem_for_st should probably be max_mem_for_mode or max_mem_for_mode_st. > - if (dev_priv->active_display_unit == vmw_du_screen_target) { > + if (using_stdu) { > max_width = min(dev_priv->stdu_max_width, max_width); > max_height = min(dev_priv->stdu_max_height, max_height); > } > @@ -2874,9 +2879,10 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, > if (max_height < mode->vdisplay) > return MODE_BAD_VVALUE; > > - if (!vmw_kms_validate_mode_vram(dev_priv, > - mode->hdisplay * assumed_cpp, > - mode->vdisplay)) > + if (using_stdu && (total > max_mem_for_st || total > dev_priv->max_mob_size)) > + return MODE_MEM; > + > + if (!vmw_kms_validate_mode_vram(dev_priv, pitch, mode->vdisplay)) > return MODE_MEM; It might make sense to just reuse vmw_kms_validate_mode_vram , it does what we're claiming to do here and even though it's called vmw_kms_validate_mode_vram it does actually validate st primary memory. z
So the issue is that SVGA_3D_CMD_DX_PRED_COPY_REGION between 2 surfaces that are the size of the mode fails. Technically for this to work the filter will have to be 1/2 of graphics mem. I was just lucky that the next mode in the list was already less than 1/2. 3/4 is not actually going to work. Also this only happens on X/Gnome and seems more like an issue with the compositor. Wayland/Gnome displays the desktop but it's unusable and glitches even with the 1/2 limit. I don't think wayland even abides by the mode limits as I see it trying to create surfaces larger than the mode. It might be using texture limits instead. On Fri, Feb 2, 2024 at 1:29 PM Zack Rusin <zack.rusin@broadcom.com> wrote: > > On Fri, Feb 2, 2024 at 11:58 AM Ian Forbes <ian.forbes@broadcom.com> wrote: > > > > SVGA requires surfaces to fit within graphics memory (max_mob_pages) which > > means that modes with a final buffer size that would exceed graphics memory > > must be pruned otherwise creation will fail. > > Sorry, I didn't notice this originally but that's not quite true. svga > doesn't require all mob memory to stay within max_mob_pages (which is > SVGA_REG_GBOBJECT_MEM_SIZE_KB). max_mob_pages is really max resident > memory or suggested-guest-memory-for-best-performance. we can grow > that memory (and we do). I think what's causing problems on systems > with low memory is that cursor mobs and the fb's need to be both > resident but can't. Now SVGA_REG_MAX_PRIMARY_MEM is the max memory in > which our topology needs to fit in (which is max_primary_mem on > vmwgfx) but afaict that's not the issue here and it's checked later in > vmw_kms_validate_mode_vram > > > Additionally, device commands which use multiple graphics resources must > > have all their resources fit within graphics memory for the duration of the > > command. Thus we need a small carve out of 1/4 of graphics memory to ensure > > commands likes surface copies to the primary framebuffer for cursor > > composition or damage clips can fit within graphics memory. > > Yes, we should probably rename max_mob_pages to max_resident_memory > instead to make this obvious. > > > This fixes an issue where VMs with low graphics memory (< 64MiB) configured > > with high resolution mode boot to a black screen because surface creation > > fails. > > Does this work if you disable gbobjects? Without gbobject's we won't > have screen targets and thus won't be offsetting by 1/4 so I wonder if > 4mb vram with legacy display would work with 1280x800 resolution. > > Also, you want to add a "V2" section to your change to describe what > changed in v2 vs v1 (and same for any subsequent change). > > > > > Signed-off-by: Ian Forbes <ian.forbes@broadcom.com> > > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > index cd4925346ed4..84e1b765cda3 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > @@ -2858,12 +2858,17 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, > > struct vmw_private *dev_priv = vmw_priv(dev); > > u32 max_width = dev_priv->texture_max_width; > > u32 max_height = dev_priv->texture_max_height; > > - u32 assumed_cpp = 4; > > - > > - if (dev_priv->assume_16bpp) > > - assumed_cpp = 2; > > + u32 assumed_cpp = dev_priv->assume_16bpp ? 2 : 4; > > + u32 pitch = mode->hdisplay * assumed_cpp; > > + u64 total = mode->vdisplay * pitch; > > + bool using_stdu = dev_priv->active_display_unit == vmw_du_screen_target; > > + u64 max_mem_for_st = dev_priv->max_mob_pages * PAGE_SIZE * 3 / 4; > > + /* ^^^ Max memory for the mode fb when using Screen Target / MOBs. > > + * We need a carveout (1/4) to account for other gfx resources that are > > + * required in gfx mem for an fb update to complete with low gfx mem (<64MiB). > > + */ > > Same wording issue as mentioned above and lets use normal comment > style (i.e. comments attach to the code below). max_mem_for_st should > probably be max_mem_for_mode or max_mem_for_mode_st. > > > - if (dev_priv->active_display_unit == vmw_du_screen_target) { > > + if (using_stdu) { > > max_width = min(dev_priv->stdu_max_width, max_width); > > max_height = min(dev_priv->stdu_max_height, max_height); > > } > > @@ -2874,9 +2879,10 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, > > if (max_height < mode->vdisplay) > > return MODE_BAD_VVALUE; > > > > - if (!vmw_kms_validate_mode_vram(dev_priv, > > - mode->hdisplay * assumed_cpp, > > - mode->vdisplay)) > > + if (using_stdu && (total > max_mem_for_st || total > dev_priv->max_mob_size)) > > + return MODE_MEM; > > + > > + if (!vmw_kms_validate_mode_vram(dev_priv, pitch, mode->vdisplay)) > > return MODE_MEM; > > It might make sense to just reuse vmw_kms_validate_mode_vram , it does > what we're claiming to do here and even though it's called > vmw_kms_validate_mode_vram it does actually validate st primary > memory. > > z
On Tue, Feb 6, 2024 at 4:30 PM Ian Forbes <ian.forbes@broadcom.com> wrote: > > So the issue is that SVGA_3D_CMD_DX_PRED_COPY_REGION between 2 > surfaces that are the size of the mode fails. Technically for this to > work the filter will have to be 1/2 of graphics mem. I was just lucky > that the next mode in the list was already less than 1/2. 3/4 is not > actually going to work. Also this only happens on X/Gnome and seems > more like an issue with the compositor. Wayland/Gnome displays the > desktop but it's unusable and glitches even with the 1/2 limit. I > don't think wayland even abides by the mode limits as I see it trying > to create surfaces larger than the mode. It might be using texture > limits instead. So the SVGA_3D_CMD_DX_PRED_COPY_REGION is only available with dx contexts/3d enabled/gb surfaces. With 3d or gb objects disabled we should fall back to legacy display and that command shouldn't have been used. Is that the case? Does it work with 3d/gb objects disabled? There's a few bugs there: - SVGA_3D_CMD_DX_PRED_COPY_REGION should only come from userspace, the userspace should validate that the max amount of resident memory hasn't been exceeded before issuing those copies - vmwgfx should be a lot better about determining whether the amount of resident memory required by the current command buffers hasn't been exceeded - In case of high memory pressure vmwgfx should explicitly disable 3d support. There's no way to run 3d workloads with anything less than 64mb of ram especially given that we do not adjust our texture limits and they will remain either 4k, 8k or more depending on what we're running on. But those are secondary to making resolution switch work correctly on basic system, i.e.: 1) Disable 3D and gb objects 2) Check if in the kernel log vmwgx says that it's using "legacy display" 3) Check if the resolution switching works correctly 4) If not lets fix that first (fix #1) 5) Disable 3D and keep gb objects active 6) Check that the kernel log select "screen target display unit" and have 3d disabled (i.e. no SVGA_3D_CMD_DX_PRED_COPY_REGION is coming through) 7) If that doesn't work lets fix that next (fix #2) 8) Enabled 3d and gb objects (your current default) 9) Check if max_mob_pages (i.e. max_resident_memory) is smaller than what we'd need to hold even a single a texture limits * 4bpp, print a warning and disable 3d (this should bring us in line with what we fixed in point #7) (fix #3) So basically we want to make sure that on vmwgfx all three configurations work: 1) 3d and gb objects disabled, 2) 3d disabled, gb objects enabled, 3) 3d and gb object enabled. z
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index cd4925346ed4..84e1b765cda3 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -2858,12 +2858,17 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, struct vmw_private *dev_priv = vmw_priv(dev); u32 max_width = dev_priv->texture_max_width; u32 max_height = dev_priv->texture_max_height; - u32 assumed_cpp = 4; - - if (dev_priv->assume_16bpp) - assumed_cpp = 2; + u32 assumed_cpp = dev_priv->assume_16bpp ? 2 : 4; + u32 pitch = mode->hdisplay * assumed_cpp; + u64 total = mode->vdisplay * pitch; + bool using_stdu = dev_priv->active_display_unit == vmw_du_screen_target; + u64 max_mem_for_st = dev_priv->max_mob_pages * PAGE_SIZE * 3 / 4; + /* ^^^ Max memory for the mode fb when using Screen Target / MOBs. + * We need a carveout (1/4) to account for other gfx resources that are + * required in gfx mem for an fb update to complete with low gfx mem (<64MiB). + */ - if (dev_priv->active_display_unit == vmw_du_screen_target) { + if (using_stdu) { max_width = min(dev_priv->stdu_max_width, max_width); max_height = min(dev_priv->stdu_max_height, max_height); } @@ -2874,9 +2879,10 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector, if (max_height < mode->vdisplay) return MODE_BAD_VVALUE; - if (!vmw_kms_validate_mode_vram(dev_priv, - mode->hdisplay * assumed_cpp, - mode->vdisplay)) + if (using_stdu && (total > max_mem_for_st || total > dev_priv->max_mob_size)) + return MODE_MEM; + + if (!vmw_kms_validate_mode_vram(dev_priv, pitch, mode->vdisplay)) return MODE_MEM; return MODE_OK;
SVGA requires surfaces to fit within graphics memory (max_mob_pages) which means that modes with a final buffer size that would exceed graphics memory must be pruned otherwise creation will fail. Additionally, device commands which use multiple graphics resources must have all their resources fit within graphics memory for the duration of the command. Thus we need a small carve out of 1/4 of graphics memory to ensure commands likes surface copies to the primary framebuffer for cursor composition or damage clips can fit within graphics memory. This fixes an issue where VMs with low graphics memory (< 64MiB) configured with high resolution mode boot to a black screen because surface creation fails. Signed-off-by: Ian Forbes <ian.forbes@broadcom.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)