Skip to content

Conversation

@dorindabassey
Copy link
Collaborator

@dorindabassey dorindabassey commented Oct 14, 2025

Summary of the PR

This PR refactors vhost-device-gpu by separating
virglrenderer from rutabaga, and using gfxstream via
rutabaga, Simplifying future backend development.

This PR introduces a significant refactor of the
virtio-gpu backend architecture:

  • Transition gfxstream support to use Rutabaga
    abstraction.
  • Decouple virglrenderer from Rutabaga, allowing
    it to be used as standalone.
  • Unify backend handling using thread-local storage
    and macro-based runtime dispatch.

Key Changes:
VirglRenderer Backend:

  • virgl.rs is now a standalone backend that
    directly calls libvirglrenderer functions.
  • Removed reliance on rutabaga for virgl path.

Gfxstream Backend via Rutabaga:

  • Introduced gfxstream.rs backend using rutabaga
  • Thread-local GfxstreamAdapter manages its own
    Rutabaga instance, initialized lazily.
  • Preserved internal GfxstreamResource tracking
    with scanout support and memory handling.

Renderer Selection Logic:

  • In device.rs, lazy_init_and_handle_event() now:
    • Dispatches VirglRendererAdapter and
      GfxstreamAdapter using thread-local storage(TLS)
  • Introduced extract_backend_and_vring() helper for
    reusing backend setup logic.

Code Deduplication:

  • Abstracted common logic for both backends to common.rs.
  • Shared helpers reused between gfxstream and virgl.
  • Improved modularity with fewer duplicated error
    handling branches.

Testing and Validation:

  • Replaced virtio_gpu.rs testing paths with new unit
    tests for gfxstream.rs and virgl.rs.
  • Added code coverage for the new refactored crate.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@dorindabassey dorindabassey force-pushed the gpurefactor branch 2 times, most recently from a99c82b to bd23eff Compare October 20, 2025 12:13
@dorindabassey dorindabassey marked this pull request as ready for review October 20, 2025 12:26
@epilys
Copy link
Member

epilys commented Oct 21, 2025

Looks like CI is failing @dorindabassey

@dorindabassey
Copy link
Collaborator Author

dorindabassey commented Oct 21, 2025

Looks like CI is failing @dorindabassey

Yeah, I already fixed it in the rust-vmm container, CI is probably still using an old image.

@stefano-garzarella
Copy link
Member

stefano-garzarella commented Oct 21, 2025

We just merged #892 with latest rust-vmm-ci, so I rebased this PR.
BTW @dorindabassey did you update the container version in rust-vmm-ci ?

@dorindabassey
Copy link
Collaborator Author

We just merged #892 with latest rust-vmm-ci, so I rebased this PR. BTW @dorindabassey did you update the container version in rust-vmm-ci ?

No, I also need to update the container version? Will do thanks!

@stefano-garzarella
Copy link
Member

We just merged #892 with latest rust-vmm-ci, so I rebased this PR. BTW @dorindabassey did you update the container version in rust-vmm-ci ?

No, I also need to update the container version? Will do thanks!

@dorindabassey yep :-( see rust-vmm/rust-vmm-ci#196

@dorindabassey dorindabassey force-pushed the gpurefactor branch 4 times, most recently from 99c5927 to 06e2ef3 Compare October 29, 2025 09:08
@dorindabassey
Copy link
Collaborator Author

It's been over a week since the rust-vmm-ci container changes were updated, @stefano-garzarella any idea why this is still building against the old container?

@stefano-garzarella
Copy link
Member

@dorindabassey I guess you need to update the rust-vmm-ci submodule in this PR or rebase on main if we already updated it.

@dorindabassey dorindabassey force-pushed the gpurefactor branch 2 times, most recently from 267b971 to b53a55f Compare October 31, 2025 09:53
@stefano-garzarella
Copy link
Member

@dorindabassey can you fix the coverage? (increasing tests or just adjusting the current value)

@dorindabassey
Copy link
Collaborator Author

@dorindabassey can you fix the coverage? (increasing tests or just adjusting the current value)

Yes, I'm working on it. Since gfxstream tests cannot run in CI without GPU drivers, excluding them in CI caused the drop I will increase the coverage in other areas and probably adjust the current value to meet expectations.

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

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

Thanks for this work, I did a quick review, but IMO it's too much for a single commit.
Why not splitting it?

I left some comments about SPDX and changes that should be split, but I'd like to know if we can split even more. (not sure if we can, but would be nice, otherwise, at least for me, it's hard to review)

@dorindabassey
Copy link
Collaborator Author

Thanks for this work, I did a quick review, but IMO it's too much for a single commit. Why not splitting it?

I left some comments about SPDX and changes that should be split, but I'd like to know if we can split even more. (not sure if we can, but would be nice, otherwise, at least for me, it's hard to review)

I considered splitting the commit, but given the scope of the changes, it would have been quite time-consuming due to how closely related the changes are. But you are right, to make it easy for review I will split it.

@dorindabassey dorindabassey force-pushed the gpurefactor branch 2 times, most recently from f92b3b3 to 4abf678 Compare November 4, 2025 10:58
@stefano-garzarella
Copy link
Member

Thanks for this work, I did a quick review, but IMO it's too much for a single commit. Why not splitting it?
I left some comments about SPDX and changes that should be split, but I'd like to know if we can split even more. (not sure if we can, but would be nice, otherwise, at least for me, it's hard to review)

I considered splitting the commit, but given the scope of the changes, it would have been quite time-consuming due to how closely related the changes are. But you are right, to make it easy for review I will split it.

If splitting doesn't make sense, you can avoid that. I just found 2 cases where IMO they must be split.

@stefano-garzarella
Copy link
Member

@dorindabassey what package I should install on Fedora to build this?

Dorinda Bassey added 4 commits November 4, 2025 13:25
Rename the 'gfxstream' feature flag to
'backend-gfxstream' for consistency with
the new modular backend architecture.

Signed-off-by: Dorinda Bassey <[email protected]>
Add virglrenderer-rs as an optional dependency
and introduce the backend-virgl feature flag
to enable the virglrenderer backend.

Signed-off-by: Dorinda Bassey <[email protected]>
This commit refactors vhost-device-gpu by separating
virglrenderer from rutabaga, and using gfxstream via
rutabaga, Simplifying future backend development.

This commit introduces a significant refactor of the
virtio-gpu backend architecture:
- Transition `gfxstream` support to use `Rutabaga`
  abstraction.
- Decouple `virglrenderer` from `Rutabaga`, allowing
  it to be used as standalone.
- Unify backend handling using thread-local storage
  and macro-based runtime dispatch.

Key Changes:
VirglRenderer Backend:
   - `virgl.rs` is now a standalone backend that
     directly calls `libvirglrenderer` functions.
   - Removed reliance on `rutabaga` for virgl path.

Gfxstream Backend via Rutabaga:
   - Introduced `gfxstream.rs` backend using `rutabaga`
   - Thread-local `GfxstreamAdapter` manages its own
     `Rutabaga` instance, initialized lazily.
   - Preserved internal `GfxstreamResource` tracking
     with scanout support and memory handling.

Renderer Selection Logic:
   - In `device.rs`, `lazy_init_and_handle_event()` now:
     - Dispatches `VirglRendererAdapter` and
       `GfxstreamAdapter` using thread-local storage(TLS)
   - Introduced `extract_backend_and_vring()` helper for
     reusing backend setup logic.

Code Deduplication:
   - Abstracted common logic for both backends to common.rs.
   - Shared helpers reused between gfxstream and virgl.
   - Improved modularity with fewer duplicated error
     handling branches.

Testing and Validation:
   - Replaced `virtio_gpu.rs` testing paths with new unit
     tests for `gfxstream.rs` and `virgl.rs`.
   - Added code coverage for the new refactored crate.
   - Update coverage file to reflect the drop in coverage
     due to exclusion of some gfxstream tests from CI
     since they can't run in CI without GPU drivers.

Signed-off-by: Dorinda Bassey <[email protected]>
This setup is no longer needed since:
- The current build system and GPU backend
  integration have been restructured.
- CI now works with the virglrenderer crate.
- Environment variable overrides are no
  longer necessary to avoid CI failures.

Signed-off-by: Dorinda Bassey <[email protected]>
@dorindabassey
Copy link
Collaborator Author

@dorindabassey what package I should install on Fedora to build this?

you need virglrenderer, virglrenderer-devel, gfxstream and gfxstream-devel from fedora package. For gfxstream you could also install from source this tag v0.1.2-gfxstream-release

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.

3 participants