Migrate to rutabaga_gfx v0.1.76-libkrun.0 from crates.io#560
Migrate to rutabaga_gfx v0.1.76-libkrun.0 from crates.io#560dorindabassey wants to merge 3 commits intocontainers:mainfrom
Conversation
Summary of ChangesHello @dorindabassey, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the project's GPU virtualization stack by migrating to an upstream version of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request migrates rutabaga_gfx to version 0.1.76-libkrun.0 from crates.io and removes the vendored version. It correctly adopts the new RutabagaBuilder API and updates resource mapping and transfer calls to match the upstream changes. However, there are two significant issues: the capset_mask is hardcoded to 0, which will likely prevent the guest from discovering 3D capabilities, and the export_table functionality is currently disabled due to type mismatch issues with the VirtioFsKey workaround, resulting in a regression for VirtioFS/GPU resource sharing.
|
To continue on the discussion from #558 Personally I also think we need to continue to accept important changes to rutabaga until we use the upstream. Actually this will help us close the gap (but the things we merge should also go upstream!). |
|
Finally testing this since I needed to test magma-gpu/rutabaga_gfx#42 and also maybe avoid contributing D-Bus support into the fork when we're so close to unforking… :) Other than the flags/capset control issue above, one thing missing compared to before is gbm (referenced in the discussion in magma-gpu/rutabaga_gfx#40), I ported over the upstream gbm support hacks from the vendored fork but might've screwed something up since software-rendered apps crash the proxy with that enabled (investigating). But without the gbm feature they work fine. (FWIW this gbm gralloc thing seems to only be an optimization, to make sure the SHM copy-out in the guest proxy goes directly to GPU-managed memory) |
dorindabassey
force-pushed
the
fixkrun
branch
3 times, most recently
from
7d945d6 to
88cff0a
Compare
+1
Fixed the flags/capset issue, For gbm - I see from magma-gpu/rutabaga_gfx#40 that there's a plan to move from minigbm to upstream GBM. Since the gbm feature isn't currently enabled in our Cargo.toml and you said it crashes with software rendering, maybe let's land this PR first and add gbm support as a follow-up once the upstream rutabaga_gfx gbm migration (magma-gpu/rutabaga_gfx#40) is done? I don't know the timeline for that, I just want to make sure we unblock the migration without getting stuck on an optimization feature. wdyt? |
|
/gemini review |
dorindabassey
requested review from
MatiasVara,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
There was a problem hiding this comment.
Code Review
This pull request migrates from a vendored rutabaga_gfx to an upstream version from a git repository. This is a significant and positive change, removing vendored code. The changes primarily involve adapting to the new rutabaga_gfx API, especially around the RutabagaBuilder and resource handling. The code modifications look correct and handle the API changes well. I have one minor suggestion to improve code style.
Definitely +1
Ohhh… huh. I was so sure it was being used, but that seems to be true!
Thanks a lot, will take a look now |
Migrate to new RutabagaBuilder API while preserving virgl_flags
parameter to properly translate GPU mode selection into capset masks.
Key changes:
- Replace RutabagaBuilder::new(component, virgl_flags, capset_mask)
with new(capset_mask, fence_handler) signature
- Translate virgl_flags to capset_mask respecting mutually exclusive
GPU modes
- Explicitly set EGL flag to match old behavior
- Rename RutabagaChannel to RutabagaPath (API change)
- Update handle type constants (e.g. RUTABAGA_MEM_HANDLE_TYPE_* to
RUTABAGA_HANDLE_TYPE_MEM_*)
- Plumb export_table through RutabagaBuilder for cross-domain fd
sharing between virtio-fs and virtio-gpu
Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Migrate to git-based rutabaga_gfx dependency and implement the VirtioFsLookup trait for virtio-fs file descriptor export to GPU contexts. Simplifies ExportTable to use (u64, u64) tuple keys. Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
Using rutabaga_gfx from crates.io version 0.1.76-libkrun.0 instead of vendored source. Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
3 participants
As part of the efforts to remove vendored rutabaga_gfx and use upstream Rutabaga_gfx, we are adopting the new RutabagaBuilder API with capset_mask, Add temporary workaround for VirtioFsKey not being publicly exported(marking as draft pending magma-gpu/rutabaga_gfx#43 merge).