virgl: capture host-rendered virtio-gpu scanouts via GPU EGL readback#20
Conversation
A virgl (virtio-gpu 3D) scanout is rendered on the host GPU. The KMS framebuffer handle the guest gets from GetFB2 points at a host-side resource, so any guest CPU read of it (dumb-map V2 or a CPU mmap of the exported DMA-BUF) comes back black, and TRANSFER_FROM_HOST does not populate a guest-readable buffer for it. The pixels are reachable on the guest GPU, though: importing the DMA-BUF as an EGLImage and glReadPixels() resolves the real host-rendered content. Helper: when virtio 3D features are present, export the scanout as a DMA-BUF (V3, as for plain virtio) and additionally tag the frame FLAG_VIRGL. The export condition is simplified to 'any virtio or a tiled non-virtio framebuffer'; plain virtio keeps the existing zero-copy direct-map path (no FLAG_VIRGL). Library: thread a force_egl flag into gpu_auto_process(). On FLAG_VIRGL the linear-modifier fast path is bypassed and the frame is run through the EGL import + glReadPixels path (the same backend used to detile tiled buffers), which reads the host-rendered pixels. Plain virtio and all other paths pass force_egl=0 and are unchanged. Verified end to end in a virgl VM (qemu virtio-gpu-gl + egl-headless, weston --renderer=gl -> 'GL renderer: virgl'): a non-root capture through the helper now returns the live weston desktop (helper logs 'DMA-BUF + GPU readback (virgl)', library logs EGL convert ret=0) instead of a black frame. Re-checked plain virtio-gpu (weston --renderer=pixman): still the zero-copy direct-map path (helper logs 'zero-copy DMA-BUF', no EGL convert), no regression. Closes #15
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds virgl-specific helper flags, marks virgl DMA-BUF exports, and updates framebuffer processing so virgl scanouts can force EGL-based readback even when CPU mapping is unavailable. ChangesVirgl scanout capture flow
Sequence Diagram(s)sequenceDiagram
participant helper as helper/drmtap-helper.c
participant grab as src/drm_grab.c
participant auto as gpu_auto_process
participant egl as EGL import/convert path
helper->>grab: send FLAG_VIRGL in helper V3 metadata
grab->>auto: pass force_egl=is_virgl for V3 DMA-BUF frames
auto->>egl: run even when data is NULL or modifier is linear
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
484-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake forced EGL readback fail closed and propagate the error.
Line 499 can enter
gpu_auto_process()withdata == NULL. If EGL is unavailable ordrmtap_gpu_egl_convert()fails, the current path can still return success with no usable pixels. Return an error whenforce_eglcannot be satisfied, and check it at the helper V3 call sites.Proposed direction
- if (do_mmap) { - gpu_auto_process(ctx, mapped, frame, is_virgl); + if (do_mmap) { + ret = gpu_auto_process(ctx, mapped, frame, is_virgl); + if (ret != 0) { + drmtap_frame_release(ctx, frame); + drmModeFreeFB2(fb2); + return ret; + } } @@ - if (do_mmap) { - gpu_auto_process(ctx, NULL, frame, is_virgl); + if (do_mmap) { + ret = gpu_auto_process(ctx, NULL, frame, is_virgl); + if (ret != 0) { + drmtap_frame_release(ctx, frame); + drmModeFreeFB2(fb2); + return ret; + } } @@ - if (!data && !force_egl) return 0; + if (!data && !force_egl) { + return 0; + } @@ if (ret == 0 && egl_data) { @@ return 0; } + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback failed: %d", ret); + return ret < 0 ? ret : -EIO; + } drmtap_debug_log(ctx, "auto-process: EGL failed (%d), trying CPU", ret); } +#else + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback requested but EGL support is unavailable"); + return -ENOTSUP; + } `#endif` + + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback requested but EGL is unavailable"); + return -ENOTSUP; + } + if (!data) { + return 0; + }Also applies to: 627-717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c` around lines 484 - 499, The helper V3 fallback path in drm_grab.c can call gpu_auto_process() with NULL data and still continue as success, which leaves force_egl without usable pixels. Update the gpu_auto_process/drmtap_gpu_egl_convert flow so that when force_egl is requested and EGL conversion is unavailable or fails, an error is returned instead of falling through, and make the helper V3 call sites that reach this path check and propagate that failure. Use the gpu_auto_process(), drmtap_gpu_egl_convert(), and helper V3 logic to locate the affected branches.bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c (1)
594-611: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not fall back to CPU pixels for confirmed virgl exports.
Line 600 should only tag virtio frames, and Line 608 should fail closed for confirmed virgl. Falling through to the dumb-map pixel path can return the same black frame this PR is fixing.
Proposed fix
- if (virtio_virgl) { + if (is_virtio && virtio_virgl) { meta.flags |= FLAG_VIRGL; /* parent must GPU-read it back */ } @@ - /* Export failed — fall through to the dumb-map pixel path. */ + if (is_virtio && virtio_virgl) { + return send_error_errno(sock, "virgl DMA-BUF export failed"); + } + /* Export failed — fall through to the dumb-map pixel path. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c` around lines 594 - 611, The fallback in drmtap-helper’s DMA-buf export path is too permissive for confirmed virgl cases. Update the conditional around the drmPrimeHandleToFD export in drmtap-helper so that only virtio frames are tagged for DMABUF/FLAG_VIRGL, and if a confirmed virgl export fails, return an error or fail closed instead of falling through to the CPU pixel mapping path. Keep the existing DMA-buf success handling in the same block, but guard the fallback in the code path around the gem_handle export logic and the send_fd/prime_fd handling.helper/drmtap-helper.c (1)
594-611: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not fall back to CPU pixels for confirmed virgl exports.
Line 600 should only tag virtio frames, and Line 608 should fail closed for confirmed virgl. Falling through to the dumb-map pixel path can return the same black frame this PR is fixing.
Proposed fix
- if (virtio_virgl) { + if (is_virtio && virtio_virgl) { meta.flags |= FLAG_VIRGL; /* parent must GPU-read it back */ } @@ - /* Export failed — fall through to the dumb-map pixel path. */ + if (is_virtio && virtio_virgl) { + return send_error_errno(sock, "virgl DMA-BUF export failed"); + } + /* Export failed — fall through to the dumb-map pixel path. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helper/drmtap-helper.c` around lines 594 - 611, The fallback in the drmtap send path is too broad: the drmPrimeHandleToFD branch in drmtap-helper.c currently falls through to the CPU pixel path even for confirmed virgl exports. Update the logic around drmPrimeHandleToFD, meta.flags, and the virtio_virgl handling so only true virtio frames are tagged, and for confirmed virgl cases fail closed instead of continuing to the dumb-map pixel path. Keep the existing send_fd path for successful exports, but prevent the fallback for virgl-specific frames and preserve the current pixel fallback only for non-virgl failures.src/drm_grab.c (1)
484-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake forced EGL readback fail closed and propagate the error.
Line 499 can enter
gpu_auto_process()withdata == NULL. If EGL is unavailable ordrmtap_gpu_egl_convert()fails, the current path can still return success with no usable pixels. Return an error whenforce_eglcannot be satisfied, and check it at the helper V3 call sites.Proposed direction
- if (do_mmap) { - gpu_auto_process(ctx, mapped, frame, is_virgl); + if (do_mmap) { + ret = gpu_auto_process(ctx, mapped, frame, is_virgl); + if (ret != 0) { + drmtap_frame_release(ctx, frame); + drmModeFreeFB2(fb2); + return ret; + } } @@ - if (do_mmap) { - gpu_auto_process(ctx, NULL, frame, is_virgl); + if (do_mmap) { + ret = gpu_auto_process(ctx, NULL, frame, is_virgl); + if (ret != 0) { + drmtap_frame_release(ctx, frame); + drmModeFreeFB2(fb2); + return ret; + } } @@ - if (!data && !force_egl) return 0; + if (!data && !force_egl) { + return 0; + } @@ if (ret == 0 && egl_data) { @@ return 0; } + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback failed: %d", ret); + return ret < 0 ? ret : -EIO; + } drmtap_debug_log(ctx, "auto-process: EGL failed (%d), trying CPU", ret); } +#else + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback requested but EGL support is unavailable"); + return -ENOTSUP; + } `#endif` + + if (force_egl) { + drmtap_set_error(ctx, "virgl EGL readback requested but EGL is unavailable"); + return -ENOTSUP; + } + if (!data) { + return 0; + }Also applies to: 627-717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/drm_grab.c` around lines 484 - 499, The helper V3 path can call gpu_auto_process() with NULL data and still succeed when force_egl is requested, which leaves no usable pixels; update the error handling so forced EGL readback fails closed. In the gpu_auto_process()/drmtap_gpu_egl_convert() flow, propagate an error whenever EGL is unavailable or the conversion fails under force_egl, instead of falling back to success. Then update the helper V3 call sites that currently pass NULL on mmap failure and the related flow in the 627-717 range to check and return that error immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c`:
- Around line 529-538: Update the stale scanout comment in drmtap-helper.c so it
matches the current virtio detection logic: replace references to the removed
virtio_plain state with the actual virtio_detected and virtio_virgl behavior
used by the scanout handling in the relevant helper path. Keep the explanation
aligned with the existing DMA-BUF export and virgl/GPU readback flow, and ensure
the fallback wording reflects how the code now treats undetected virtio cases.
In `@helper/drmtap-helper.c`:
- Around line 529-538: Update the stale comment in drmtap helper to match the
current virtio-gpu detection logic: the code now uses virtio_detected and
virtio_virgl instead of virtio_plain. Edit the explanatory block near the
scanout handling logic so it describes the current plain vs virgl behavior using
the actual symbols in the code, and remove any reference to the removed
virtio_plain state.
---
Outside diff comments:
In `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c`:
- Around line 484-499: The helper V3 fallback path in drm_grab.c can call
gpu_auto_process() with NULL data and still continue as success, which leaves
force_egl without usable pixels. Update the
gpu_auto_process/drmtap_gpu_egl_convert flow so that when force_egl is requested
and EGL conversion is unavailable or fails, an error is returned instead of
falling through, and make the helper V3 call sites that reach this path check
and propagate that failure. Use the gpu_auto_process(),
drmtap_gpu_egl_convert(), and helper V3 logic to locate the affected branches.
In `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c`:
- Around line 594-611: The fallback in drmtap-helper’s DMA-buf export path is
too permissive for confirmed virgl cases. Update the conditional around the
drmPrimeHandleToFD export in drmtap-helper so that only virtio frames are tagged
for DMABUF/FLAG_VIRGL, and if a confirmed virgl export fails, return an error or
fail closed instead of falling through to the CPU pixel mapping path. Keep the
existing DMA-buf success handling in the same block, but guard the fallback in
the code path around the gem_handle export logic and the send_fd/prime_fd
handling.
In `@helper/drmtap-helper.c`:
- Around line 594-611: The fallback in the drmtap send path is too broad: the
drmPrimeHandleToFD branch in drmtap-helper.c currently falls through to the CPU
pixel path even for confirmed virgl exports. Update the logic around
drmPrimeHandleToFD, meta.flags, and the virtio_virgl handling so only true
virtio frames are tagged, and for confirmed virgl cases fail closed instead of
continuing to the dumb-map pixel path. Keep the existing send_fd path for
successful exports, but prevent the fallback for virgl-specific frames and
preserve the current pixel fallback only for non-virgl failures.
In `@src/drm_grab.c`:
- Around line 484-499: The helper V3 path can call gpu_auto_process() with NULL
data and still succeed when force_egl is requested, which leaves no usable
pixels; update the error handling so forced EGL readback fails closed. In the
gpu_auto_process()/drmtap_gpu_egl_convert() flow, propagate an error whenever
EGL is unavailable or the conversion fails under force_egl, instead of falling
back to success. Then update the helper V3 call sites that currently pass NULL
on mmap failure and the related flow in the 627-717 range to check and return
that error immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35dd3e15-8315-4c80-b618-4abdfcee8ddc
📒 Files selected for processing (6)
bindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap-helper.cbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hhelper/drmtap-helper.csrc/drm_grab.csrc/drmtap_internal.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
**/*.h: Use 4-space indentation, never tabs, and 1TBS brace style in C header files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Use#ifndef DRMTAP_MODULE_H/#define DRMTAP_MODULE_Hstyle header guards.
Public API functions ininclude/drmtap.hmust use Doxygen comments describing purpose, parameters, return values, and error codes.
Files:
bindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/drmtap_internal.h
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Keep lines to a 100-character soft limit and 120-character hard limit.
Use//for single-line comments and/* */for multi-line comments.
Always checkmallocreturns and free resources in cleanup paths.
Files:
bindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/drmtap_internal.hhelper/drmtap-helper.cbindings/rust/libdrmtap-sys/csrc/drmtap-helper.csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Use 4-space indentation, never tabs, and 1TBS brace style in C source files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Return negative errno values for errors and never abort.
Every.cfile must start with the standard libdrmtap copyright/license header block, followed by a Doxygen@filecomment.
Organize C source files as includes first, then private types/constants, then static functions, then public functions in header-declaration order.
Use simple//comments for internal static functions instead of full Doxygen blocks.
Files:
src/drm_grab.c
🪛 Clang (14.0.6)
helper/drmtap-helper.c
[warning] 545-545: variable name 'gp' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: variable 'prime_ret' is not initialized
(cppcoreguidelines-init-variables)
bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
[warning] 545-545: variable name 'gp' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: variable 'prime_ret' is not initialized
(cppcoreguidelines-init-variables)
src/drm_grab.c
[warning] 633-633: statement should be inside braces
(readability-braces-around-statements)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 633-633: statement should be inside braces
(readability-braces-around-statements)
🔇 Additional comments (6)
bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c (1)
129-132: LGTM!helper/drmtap-helper.c (1)
129-132: LGTM!bindings/rust/libdrmtap-sys/csrc/drmtap_internal.h (1)
122-124: LGTM!src/drmtap_internal.h (1)
122-124: LGTM!bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
62-62: LGTM!Also applies to: 528-528, 605-605, 974-974, 1069-1069
src/drm_grab.c (1)
62-62: LGTM!Also applies to: 528-528, 605-605, 974-974, 1069-1069
Addresses review feedback. A virgl scanout has no CPU-readable pixels, so on every path where the GPU readback cannot happen we now return an error instead of emitting an all-black frame as success: - Helper: if drmPrimeHandleToFD fails for a confirmed-virgl frame, send_error instead of falling through to the dumb-map pixel path (which is black). - Library: gpu_auto_process() returns -ENOTSUP when force_egl is set but the EGL path did not produce pixels (EGL unavailable or convert failed), and the helper-V3 call sites propagate that error for virgl frames (non-virgl frames keep the prior best-effort behaviour). Also fixes a stale comment that still referenced the removed virtio_plain variable. Happy path is unchanged: a successful EGL readback returns before these checks, as verified in the virgl VM.
|
Good calls, all three addressed in the latest push:
The happy path is untouched (a successful EGL readback returns before these checks), so the virgl VM capture still works. release/asan/plain + meson test 5/5 + cppcheck all clean. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
455-510: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up helper V3 resources before returning the virgl error.
By Line 509,
dmabuf_fd,priv, and possiblymappedare owned byframe; returningpreton a failed grab skips normal frame release and leaks those resources. Clean up the partially initialized frame before returning. As per coding guidelines,**/*.{c,h}files must “Always checkmallocreturns and free resources in cleanup paths.”Proposed cleanup
if (is_virgl && pret != 0) { + if (priv->sync_started) { + dmabuf_sync_end(dmabuf_fd); + } + if (priv->mapped != MAP_FAILED) { + munmap(priv->mapped, priv->mapped_size); + } + close(dmabuf_fd); + free(priv); + frame->data = NULL; + frame->dma_buf_fd = -1; + frame->_priv = NULL; return pret; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c` around lines 455 - 510, The virgl error return path in the DMA-BUF grab flow exits before the frame-owned resources are released, leaving the partially initialized `frame`, `priv`, and `dmabuf_fd`/`mapped` state leaked. Update the `drm_grab` helper V3 cleanup path so the failed `is_virgl && pret != 0` branch runs the same release/cleanup logic used for normal frame teardown before returning the error, using the existing `priv`, `frame`, and `dmabuf_fd` ownership handling.Source: Coding guidelines
src/drm_grab.c (1)
455-510: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up helper V3 resources before returning the virgl error.
By Line 509,
dmabuf_fd,priv, and possiblymappedare owned byframe; returningpreton a failed grab skips normal frame release and leaks those resources. Clean up the partially initialized frame before returning. As per coding guidelines,**/*.{c,h}files must “Always checkmallocreturns and free resources in cleanup paths.”Proposed cleanup
if (is_virgl && pret != 0) { + if (priv->sync_started) { + dmabuf_sync_end(dmabuf_fd); + } + if (priv->mapped != MAP_FAILED) { + munmap(priv->mapped, priv->mapped_size); + } + close(dmabuf_fd); + free(priv); + frame->data = NULL; + frame->dma_buf_fd = -1; + frame->_priv = NULL; return pret; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/drm_grab.c` around lines 455 - 510, The virgl error return path in the helper V3 grab flow skips the normal frame cleanup, leaving resources owned through the frame state leaked. Before returning the `pret` failure from this block, make sure the partially initialized frame is torn down and its owned resources are released, including the DMA-BUF fd, any `mapped`/`priv->mapped` mapping, and the `priv` state. Update the cleanup path around the `gpu_auto_process`, `dmabuf_sync_start`, and `frame->_priv` setup so failed virgl grabs exit through a release path rather than returning immediately.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c`:
- Around line 455-510: The virgl error return path in the DMA-BUF grab flow
exits before the frame-owned resources are released, leaving the partially
initialized `frame`, `priv`, and `dmabuf_fd`/`mapped` state leaked. Update the
`drm_grab` helper V3 cleanup path so the failed `is_virgl && pret != 0` branch
runs the same release/cleanup logic used for normal frame teardown before
returning the error, using the existing `priv`, `frame`, and `dmabuf_fd`
ownership handling.
In `@src/drm_grab.c`:
- Around line 455-510: The virgl error return path in the helper V3 grab flow
skips the normal frame cleanup, leaving resources owned through the frame state
leaked. Before returning the `pret` failure from this block, make sure the
partially initialized frame is torn down and its owned resources are released,
including the DMA-BUF fd, any `mapped`/`priv->mapped` mapping, and the `priv`
state. Update the cleanup path around the `gpu_auto_process`,
`dmabuf_sync_start`, and `frame->_priv` setup so failed virgl grabs exit through
a release path rather than returning immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46cb5c77-3c07-4c5e-9f28-fd203c043d4c
📒 Files selected for processing (4)
bindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap-helper.chelper/drmtap-helper.csrc/drm_grab.c
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Keep lines to a 100-character soft limit and 120-character hard limit.
Use//for single-line comments and/* */for multi-line comments.
Always checkmallocreturns and free resources in cleanup paths.
Files:
helper/drmtap-helper.cbindings/rust/libdrmtap-sys/csrc/drmtap-helper.csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Use 4-space indentation, never tabs, and 1TBS brace style in C source files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Return negative errno values for errors and never abort.
Every.cfile must start with the standard libdrmtap copyright/license header block, followed by a Doxygen@filecomment.
Organize C source files as includes first, then private types/constants, then static functions, then public functions in header-declaration order.
Use simple//comments for internal static functions instead of full Doxygen blocks.
Files:
src/drm_grab.c
🪛 Clang (14.0.6)
helper/drmtap-helper.c
[note] 611-611: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
[note] 611-611: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
src/drm_grab.c
[note] 509-509: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 509-509: +1
(clang)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[note] 509-509: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 509-509: +1
(clang)
🔇 Additional comments (4)
bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c (1)
537-555: LGTM!Also applies to: 608-615
helper/drmtap-helper.c (1)
537-555: LGTM!Also applies to: 608-615
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
536-536: LGTM!Also applies to: 613-613, 635-647, 727-736
src/drm_grab.c (1)
536-536: LGTM!Also applies to: 613-613, 635-647, 727-736
1 participant
Fixes #15: a virgl-rendered virtio-gpu scanout used to capture as an all-black frame.
Why it was black
A virgl (virtio-gpu 3D) scanout is rendered on the host GPU. The KMS framebuffer handle the guest gets from
GetFB2points at a host-side resource, so any guest CPU read of it (dumb-map V2, or a CPUmmapof the exported DMA-BUF) comes back black, andTRANSFER_FROM_HOSTdoes not populate a guest-readable buffer for it. The pixels are reachable on the guest GPU: importing the DMA-BUF as anEGLImageandglReadPixels()resolves the real host-rendered content (the same backend already used to detile tiled buffers).Change
VIRTGPU_PARAM_3D_FEATURES != 0), export the scanout as a DMA-BUF (V3) and tag the frameFLAG_VIRGL. The export condition is simplified to any virtio, or a tiled non-virtio framebuffer. Plain virtio keeps the existing zero-copy direct-map path (noFLAG_VIRGL).force_eglflag is threaded intogpu_auto_process(). OnHELPER_FLAG_VIRGLthe linear-modifier fast path is bypassed and the frame is run through the EGL import +glReadPixelspath. Every other path passesforce_egl=0and is unchanged.Cost on virgl: one GPU textured-quad draw +
glReadPixelsper frame, the same as the Intel/Nvidia detile path. Plain virtio is untouched (still direct-map, no EGL).Verification (real virgl VM)
QEMU
virtio-gpu-gl-pci+egl-headless, Ubuntu guest,weston --renderer=glreportingGL renderer: virgl (Mesa Intel Arc):virtio 3D features=1 -> DMA-BUF + GPU readback (virgl); library logsEGL convert: ret=0.No-regression checks:
weston --renderer=pixman): still the zero-copy direct-map path (helper logszero-copy DMA-BUF, no EGL convert), capture correct.Note: this fixes the helper capture path (the supported unprivileged deployment). Direct/root-mode virgl capture (the library opening virtio-gpu itself) would want the same EGL-readback treatment on its own paths and is left as a follow-up.
Closes #15