Skip to content

fix: Make surface definitions transactional - #562

Merged
kroese merged 5 commits into
masterfrom
work
Oct 5, 2026
Merged

kroese merged 5 commits into
masterfrom
work

Conversation

@kroese

@kroese kroese commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Screen-transition preservation needs human validation, and the legacy memory-budget regression remains unresolved.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Refactors VMware SVGA surface definitions to prepare replacements before publishing them, preserving existing surfaces on failure.

Changes:

  • Separates surface preparation, object-table publication, and commit.
  • Uses the guest-backed memory budget on capable devices.
  • Adds detailed D3D11 resource-creation failure traces.
File Description
hw/​display/​vmware_vga_dxvk.c Adds resource-creation failure diagnostics.
hw/​display/​vmware_vga_3d.c Introduces transactional surface preparation and commit, and changes budget selection.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread hw/display/vmware_vga_3d.c
Copilot AI balanced review requested due to automatic review settings October 5, 2026 14:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Failure handling across guest metadata and live scanout needs correction and human validation.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restore direct scanout binding when surface redefinition fails

hw/​display/​vmware_vga_3d.c:3776

When redefining the active direct-scanout surface, vmsvga3d_surface_prepare() detaches its direct binding before this OTable write. If the write fails, this path frees the candidate but does not restore that binding. Detach preserves the current pixels in a copy, but the rejected redefine still changes the live scanout source while leaving the old surface installed. Stage the fallible materialization before publishing and defer clearing the binding until commit, or restore the prior binding on failure.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 15:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Budget inconsistencies remain unresolved, and rollback and scanout storage lifetimes require human validation.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Separate GB and legacy surface memory accounting

hw/​display/​vmware_vga_3d.c:3228

GB and legacy commits both update state->surface_bytes, but GB allocations now check that total against 1 GiB while legacy definitions still check it against 512 MiB. With 576 MiB of GB shadows installed, a 64 MiB legacy DEFINE is rejected; creating that legacy surface first and then the same GB surfaces succeeds. Allocation order therefore determines whether the same surface set fits. Track GB and legacy usage separately, including replacements between the two kinds and destruction, so each advertised budget applies to the corresponding surfaces.

Medium severity Use GB memory sizing during guest-backed surface recovery

hw/​display/​vmware_vga_3d.c:3481

vmsvga3d_d3d10_recover_gb_surface_live() still calls this installer, so guest-backed surface recovery uses 512 MiB while GB DEFINE now allows 1 GiB. When a valid OTable entry has no host surface, recovering a 64 MiB depth surface with 512 MiB already installed fails the budget check, although DEFINE would accept it; vmsvga3d_d3d10_dsv_realize_live() then returns false. Route GB recovery through preparation/commit with vmsvga3d_gb_surface_memory_size(s), preserving the legacy limit for legacy DEFINE calls.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 15:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Surface lifetime, scanout transitions, and publication ordering require human review and runtime validation of failure paths.

Review effort: Balanced
Findings: None

@kroese
kroese merged commit 44a151d into master Oct 5, 2026
2 checks passed
@kroese
kroese deleted the work branch October 5, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants