[PATCH] drm/vmwgfx: Filter modes which exceed 3/4 of graphics memory.

[Thomas Zimmermann]

Hi Zack

Am 30.01.24 um 20:38 schrieb Zack Rusin:
> On Fri, Jan 12, 2024 at 4:20 PM Ian Forbes <ian.forbes at 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.
>>
>> 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 at broadcom.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> index 28ff30e32fab..39d6d17fc488 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> @@ -2854,12 +2854,12 @@ 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;
>> +       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;
>>
>> -       if (dev_priv->assume_16bpp)
>> -               assumed_cpp = 2;
>> -
>> -       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);
>>          }
>> @@ -2870,9 +2870,13 @@ 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 > (dev_priv->max_mob_pages * PAGE_SIZE * 3 / 4) ||
> 
> Could you export that computation somewhere where we could document
> why we're doing this? Just to not leave the awkward "* 3 /4" that
> everyone reading this code will wonder about?

There's code in VRAM helpers that does a similar test. [1] But the VRAM 
helpers are obsolete. I guess that TTM could contain such a test 
somewhere by testing against a max_bo_size for each TTM resource. 
Whether that is feasible in practice is a different question. Maybe ask 
the TTM maintainers.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem_vram_helper.c#L1156

> And also make sure you indent this correctly, "dim checkpatch" should
> warn about this.
> 
> z

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240131/137b6032/attachment.sig>