0.5.3: unbalanced DMA_BUF sync on the fast path, Nvidia deswizzler vendor byte, three GEM handle leaks - #48
Conversation
…sh the changelog cppcheck 2.19 asks for vdisplay's declaration to move down to its only use, a log line further along. Taking that advice would read freed memory: both hdisplay and vdisplay are copied out while crtc is alive because drmModeFreeCrtc runs on the next line. The CI image carries an older cppcheck that does not report it, so this was a build break waiting for the image to update. Suppressed inline, with the reason, which the CI invocation already honours via --inline-suppr. The [Unreleased] changelog section was written before the audit found the last four things, so it described three fixes out of twelve commits. It now covers the tiled-scanout-relabelled-linear fix and its measurement, the third and fourth instances of the unpaired dma-buf sync, the twenty dead EGL diagnostics, and the documentation work including the "0.3" dependency line that 0.5.2 was released to fix and did not.
…and three GEM handle leaks Two findings from the pre-release audit, both older than this branch. The Nvidia branch tested vendor 0x10. That is not a vendor: DRM_FORMAT_MOD_VENDOR_NVIDIA is 0x03 (drm_fourcc.h:470), and 0x10 is the low byte of the block-linear encoding, fourcc_mod_code(NVIDIA, 0x10 | ...). So no modifier any driver emits ever reached deswizzle_nvidia_x_tiled: it has never run on a single frame. It survived because tests/test_deswizzle.c hard-coded the same wrong constant and asserted the roundtrip succeeded, so the test certified the bug rather than catching it. The decoder is removed rather than switched on. Its tile geometry has never been checked against a Tegra scanout, and enabling an untried decoder to satisfy a table in the README would trade a clean failure for pixels nobody has confirmed, which is the exact defect this branch spent the day removing. Nvidia now fails closed, the same as AMD, and both READMEs say EGL-only instead of claiming a CPU fallback that does not exist. git log -S deswizzle_nvidia_x_tiled has the implementation for whoever validates it. Worth stating plainly: before this branch a real Nvidia or AMD modifier fell through to the linear memcpy and returned 0, so those hosts were getting garbage reported as success on the CPU path. This converts that into an honest error. Second, do_grab leaked the GEM handle drmModeGetFB2 mints, on the privileged path, on three returns: the geometry rejection, drmPrimeHandleToFD failing for anything other than EACCES/EPERM, and the priv calloc failing. The last two reach the cleanup label before priv exists, and that label only closed the handle `if (priv)`. The fast path already held the handle in a local for exactly this reason. It does now too. Verified here: unit 9/9, integration 2/2 on i915, cppcheck clean under the CI invocation. Restoring the permissive tail puts test_formats red, which is the control that matters. Note the Nvidia test pins the CONTRACT, not the constant: with the vendor byte wrong it still passes, because 0x03 falls through to the same fail-closed default. Not verified here: nothing exercises the Nvidia path on this hardware, which is how it stayed broken.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change publishes version 0.5.3, disables unvalidated Nvidia CPU deswizzling with ChangesNvidia handling and 0.5.3 release
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 `@src/drm_grab.c`:
- Around line 837-843: Replace the single pending_gem ownership tracker with a
collection of all unique non-zero handles returned by drmModeGetFB2, and close
only handles that were not adopted while transferring every adopted handle to
priv; update cleanup and ownership transitions at src/drm_grab.c ranges 837-843,
853, 884, 1123, and 1244-1253. Mirror the same handle tracking and cleanup
behavior in bindings/rust/libdrmtap-sys/csrc/drm_grab.c at ranges 837-843, 853,
884, 1123, and 1244-1253, using drmtap_gem_close without double-closing handles.
In `@src/gpu_nvidia.c`:
- Around line 36-39: Update the Nvidia modifier handling branch identified by
NV_VENDOR to log that CPU decoding/deswizzle is unsupported and return -ENOTSUP
immediately. Remove the allocation and drmtap_deswizzle path for these modifiers
so allocation failures cannot produce -ENOMEM.
In `@src/pixel_convert.c`:
- Around line 162-171: Update the relevant research document and gotcha
checklist to record that DRM_FORMAT_MOD_VENDOR_NVIDIA is 0x03, that treating
0x10 as the vendor byte is invalid, and that Nvidia modifiers now fail closed.
Reference the removed deswizzler and affected roundtrip test context where
appropriate, without changing implementation behavior.
🪄 Autofix
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: 8648f49e-9e9b-4d05-b04e-af6caeffe587
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap/Cargo.tomlinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.c
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Rust crate (libdrmtap-sys + libdrmtap)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (c-cpp)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Every change must compile with Meson using-Wall -Wextra -Werrorwithout warnings and must pass the existing test suite.
Public API changes require updatinginclude/drmtap.hcomments; newly discovered gotchas require updating the relevant research document; architecture changes require updatingdocs/research/05_api_and_architecture.md.
Use commit messages with a component prefix and imperative short description, optionally followed by rationale and issue references.
Use[component] Short descriptionfor pull request titles and follow the documented PR description structure.
Files:
meson.buildbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap/Cargo.tomlAGENTS.mdsrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/drmtap.hREADME.mdCHANGELOG.mdbindings/rust/libdrmtap-sys/csrc/drmtap.csrc/drmtap.cinclude/drmtap.hsrc/pixel_convert.ctests/test_deswizzle.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drm_grab.csrc/drm_grab.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation, no tabs, snake_case names, same-line braces (1TBS), and//or/* */comments as appropriate.
Every C source and header file must begin with the specified libdrmtap copyright, license, URL, and file documentation header block.
Files:
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.csrc/drmtap.cinclude/drmtap.hsrc/pixel_convert.ctests/test_deswizzle.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drm_grab.csrc/drm_grab.c
bindings/rust/**
📄 CodeRabbit inference engine (AGENTS.md)
Maintain the shared project version consistently across the C library, Meson,
libdrmtap-sys, andlibdrmtap; the canonical version is ininclude/drmtap.hand is checked bytools/check-version.sh.
Files:
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Return negative errno values for errors, never abort, check allocations, and release resources on all cleanup paths.
Use agoto cleanuperror-handling pattern; set human-readable context errors withset_error(ctx, ...)and clean up mappings, file descriptors, and other resources.
Files:
src/gpu_nvidia.csrc/drmtap.csrc/pixel_convert.csrc/drm_grab.c
src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{c,h}: Detecthandles[0] == 0as missingCAP_SYS_ADMINand trigger the privilege helper.
Check the DRM modifier;DRM_FORMAT_MOD_LINEARrequires no deswizzle.
Files:
src/gpu_nvidia.csrc/drmtap.csrc/pixel_convert.csrc/drm_grab.c
include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
include/**/*.h: Public API functions ininclude/drmtap.hmust use Doxygen comments documenting purpose, parameters, return values, and relevant error codes.
Use header guards in the form#ifndef DRMTAP_MODULE_Hand#define DRMTAP_MODULE_H.
Files:
include/drmtap.h
src/{pixel_convert,gpu_egl}.c
📄 CodeRabbit inference engine (AGENTS.md)
When HDR metadata indicates HDR/PQ, tone-map supported 10-bit and 16-bit RGB scanouts from PQ BT.2020 to SDR BT.709, then sRGB-encode to 8-bit in both CPU and EGL paths; do not claim P010 or HLG support. Plain SDR 10-bit uses straight bit-depth reduction.
Files:
src/pixel_convert.c
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: Use simple assert-based tests without an external test framework.
Keep tests in exactly two suites:unitfor hardware-independent tests andintegrationfor DRM-device tests; there is no separategpusuite.
Files:
tests/test_deswizzle.c
tests/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
New functions should have corresponding tests when possible; document exceptions for GPU-specific code requiring real hardware.
Files:
tests/test_deswizzle.c
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
src/drm_grab.c: Never cachefb_id; refresh it withdrmModeGetPlane()for every frame.
Use the Prime DMA-BUF path rather than GEM_FLINK, because GEM_FLINK does not work with vkms.
Files:
src/drm_grab.c
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T20:42:56.222Z
Learning: Keep architecture layers separate; do not mix grab, enumerate, convert, and helper concerns.
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T20:42:56.222Z
Learning: Keep each pull request focused on one concern and include test results, relevant research references, and checklist confirmations.
🪛 Clang (14.0.6)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 842-842: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1250-1250: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1252-1252: +1, nesting level increased to 1
(clang)
src/drm_grab.c
[warning] 842-842: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1250-1250: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1252-1252: +1, nesting level increased to 1
(clang)
🔇 Additional comments (16)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
703-709: LGTM!src/drm_grab.c (1)
703-709: LGTM!bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
include/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
src/drmtap.c (1)
240-240: LGTM!bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!bindings/rust/libdrmtap/Cargo.toml (1)
3-3: LGTM!Also applies to: 16-16
meson.build (1)
4-4: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c (1)
36-39: 🗄️ Data Integrity & IntegrationNo change needed.
The Nvidia branch is limited to real Nvidia modifiers, and each supported backend handles only the modifiers it can decode.
NV_VENDOR = 0x03does not change the active CPU deswizzle path here.> Likely an incorrect or invalid review comment.bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
162-171: LGTM!Also applies to: 320-338
src/pixel_convert.c (1)
320-338: LGTM!tests/test_deswizzle.c (1)
20-20: LGTM!Also applies to: 177-199
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
240-240: LGTM!README.md (1)
84-85: LGTM!Also applies to: 115-115
AGENTS.md (1)
388-388: LGTM!CHANGELOG.md (1)
8-112: LGTM!Also applies to: 594-594
…way to -ENOTSUP Both from the review of the release PR, both real. `drmModeGetFB2` creates a fresh GEM handle for every plane it reports, on every call. Only `handles[0]` is used here -- the dma-buf export and the EGL import both go through the single fd derived from it -- so on a scanout whose planes live in separate BOs the rest were leaked, one per grab, forever. CCS is where `num_planes >= 2` comes from. They are closed as soon as they arrive now, on the slow path and the fast path, deduplicated first: planes that share a BO come back as the SAME handle and closing it twice would free one the caller still owns. And with the vendor byte corrected to 0x03, real Nvidia modifiers now reach a branch that logged "CPU deswizzle", allocated `stride * height`, called a deswizzler that answers -ENOTSUP for this vendor, and threw the buffer away -- reporting -ENOMEM instead of the truth if the allocation failed. It says what it means and returns -ENOTSUP directly. The gotcha document gains both, as the review asked: the vendor-byte rule (read it against `drm_fourcc.h`, never a literal, and distrust a test that asserts a literal) and the GetFB2 handle-ownership rule. meson test 11/11, check-version clean, real capture through examples/screenshot on Intel Meteor Lake.
The entry was written before the last two commits landed and never caught up, so it under-described its own release. Audited against the diff rather than against the previous text, which is the only way this class shows up. - The Nvidia vendor byte had no section at all. It is arguably the most visible item here for an Nvidia user: the backend matched `0x10` where `DRM_FORMAT_MOD_VENDOR_NVIDIA` is `0x03`, so it was dead in every release, and a unit test asserted the wrong constant rather than catching it. - The three GEM handle leaks on the grab path had none either. That section now carries the auxiliary-plane leak too, since they are the same subject. - The count in the opening line said four and is now gone entirely. A number there has to be re-derived on every edit and buys nothing. - "No API or ABI change" was true of the ABI and misleading about the contract: `drmtap_deswizzle` fails closed where it used to copy out and report success. Said plainly, since a caller that checks the return value will see the change. - The section titled after the review that found two of the fixes is gone; they are filed under the subjects they belong to. Who found a bug is not changelog material. meson test 11/11, check-version clean.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/research/06_github_issues_analysis.md`:
- Around line 169-185: Add upstream kernel-source links in the section
describing DRM_FORMAT_MOD_VENDOR_NVIDIA and fourcc_mod_code, pointing directly
to their DRM definitions so readers can verify the vendor-byte and
modifier-encoding rules. Keep the existing analysis unchanged and link both
referenced symbols.
In `@src/drm_grab.c`:
- Around line 79-88: Replace the lengthy block comment above
close_auxiliary_gem_handles with a concise // comment stating that auxiliary GEM
handles are deduplicated and closed, while handles[0] is retained because it
remains caller-owned. Apply the same comment-only change in src/drm_grab.c at
lines 79-88 and bindings/rust/libdrmtap-sys/csrc/drm_grab.c at lines 79-88.
In `@src/pixel_convert.c`:
- Around line 320-338: Replace the raw Nvidia vendor value in the
src/pixel_convert.c and bindings/rust/libdrmtap-sys/csrc/pixel_convert.c vendor
checks with DRM_FORMAT_MOD_VENDOR_NVIDIA. In tests/test_deswizzle.c, construct
both Nvidia modifiers using fourcc_mod_code(DRM_FORMAT_MOD_VENDOR_NVIDIA, ...)
while preserving their existing modifier payloads.
🪄 Autofix
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: 3d2d4b33-8ac4-4243-8f87-e8185715cd2a
📒 Files selected for processing (18)
AGENTS.mdCHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap/Cargo.tomldocs/research/06_github_issues_analysis.mdinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation and no tabs, snake_case names, same-line braces (1TBS), 100-character soft and 120-character hard line limits, and////* */comments as appropriate.
Prefix public functions withdrmtap_; use snake_case variables,UPPER_SNAKE_CASEDRMTAP_macros/constants,snake_case_tinternal types, anddrmtap_*public types.
Return negative errno values for errors, never abort, always check allocation results, and release resources on cleanup paths.
Every C source and header file must begin with the specified libdrmtap copyright, SPDX, and file documentation header block.
Use thegoto cleanuppattern for multi-resource error paths, set human-readable context errors withset_error(ctx, ...), and close or unmap all acquired resources.
Keep architecture concerns separated between grab, enumerate, convert, and helper layers.
Files:
include/drmtap.hsrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.ctests/test_deswizzle.csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/drm_grab.c
include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use
DRMTAP_MODULE_H-style header guards for public and internal headers.
Files:
include/drmtap.h
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
include/drmtap.h: Document every public API function with Doxygen comments including its behavior, parameters, return value, and relevant error values.
Update public API comments whenever the public API changes.
Files:
include/drmtap.h
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep the version as one shared value across the C library, Meson,
libdrmtap-sys, andlibdrmtap; useinclude/drmtap.has canonical and validate withtools/check-version.sh.
Files:
include/drmtap.hmeson.buildbindings/rust/libdrmtap-sys/Cargo.tomlsrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/pixel_convert.ctests/test_deswizzle.csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/drmtap.cAGENTS.mddocs/research/06_github_issues_analysis.mdREADME.mdbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.hCHANGELOG.mdsrc/drm_grab.c
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
The project must build with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
meson.build
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Organize implementation files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Document internal static functions with concise//comments rather than public-API Doxygen blocks.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper; use the Prime path rather than GEM_FLINK.
Files:
src/pixel_convert.csrc/gpu_nvidia.csrc/drmtap.csrc/drm_grab.c
src/{gpu_egl,pixel_convert}.c
📄 CodeRabbit inference engine (AGENTS.md)
src/{gpu_egl,pixel_convert}.c: Use EGL ingpu_egl.cas the primary detile path for tiled or compressed framebuffers; CPU deswizzle inpixel_convert.cis only a fallback.DRM_FORMAT_MOD_LINEARrequires no deswizzle.
For HDR connectors, tone-map PQ HDR10 AR30/XR30 and 16-bit XR48/AR48/XB48/AB48 scanouts to 8-bit SDR using PQ decoding, BT.2020-to-BT.709 mapping, tone mapping, and sRGB encoding; do not claim P010 or HLG support.
Files:
src/pixel_convert.c
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: Use simple assert-based tests without an external test framework; add a corresponding test for each new function when practical.
Do not test tiling or deswizzle behavior against VKMS because VKMS is LINEAR-only; use real hardware for GPU-specific tests and document that requirement.
Files:
tests/test_deswizzle.c
docs/research/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Write documentation in English, include dates and sources in research documents, link kernel or upstream sources for DRM APIs, and update the gotcha checklist when discovering issues.
Files:
docs/research/06_github_issues_analysis.md
docs/research/{05_api_and_architecture,06_github_issues_analysis}.md
📄 CodeRabbit inference engine (AGENTS.md)
Update architecture documentation for architecture changes and update the known-gotchas document when new gotchas are discovered.
Files:
docs/research/06_github_issues_analysis.md
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()for every frame.
Files:
src/drm_grab.c
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T22:07:32.516Z
Learning: Use imperative commit messages formatted as `component: short description`, with an optional explanation and issue reference; use the documented component prefixes.
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T22:07:32.516Z
Learning: Keep one concern per PR and include test results, even when testing only with VKMS.
🪛 Clang (14.0.6)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 90-90: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 91-91: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 96-96: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 871-871: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1280-1280: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1282-1282: +1, nesting level increased to 1
(clang)
src/drm_grab.c
[warning] 90-90: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 91-91: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 96-96: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 871-871: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1280-1280: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1282-1282: +1, nesting level increased to 1
(clang)
🔇 Additional comments (17)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
732-738: LGTM!Also applies to: 866-883, 914-914, 1153-1153, 1274-1283, 2024-2025
src/drm_grab.c (1)
732-738: LGTM!Also applies to: 866-883, 914-914, 1153-1153, 1274-1283, 2024-2025
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
include/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c (1)
36-39: LGTM!Also applies to: 59-64, 75-85
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
240-240: LGTM!src/drmtap.c (1)
240-240: LGTM!src/gpu_nvidia.c (1)
36-39: LGTM!Also applies to: 59-64, 75-85
bindings/rust/libdrmtap/Cargo.toml (1)
3-3: LGTM!Also applies to: 16-16
meson.build (1)
4-4: LGTM!bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
162-171: LGTM!tests/test_deswizzle.c (1)
20-20: LGTM!src/pixel_convert.c (1)
162-171: LGTM!README.md (1)
84-85: LGTM!Also applies to: 115-115
AGENTS.md (1)
388-388: LGTM!CHANGELOG.md (1)
8-152: LGTM!Also applies to: 634-634
bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!
Three from the review, all against conventions that are ours, checked in AGENTS.md rather than taken on the reviewer's word. The vendor byte is the one that stings: the gotcha this release adds says to compare it against a `DRM_FORMAT_MOD_VENDOR_*` macro and never a literal, and the code doing the comparing wrote `0x03`. It now uses the macros, in `pixel_convert.c`, in `gpu_nvidia.c`, and in the test, which builds its modifiers with `fourcc_mod_code(NVIDIA, ...)` instead of hand-packed constants of exactly the kind that made this bug survive a passing test. That needs libdrm on the test target, so meson.build gains it with the reason attached. The research note now links the upstream and kernel `drm_fourcc.h` so a reader can check the encoding instead of trusting the page (AGENTS.md:356), and the new static helper uses `//` like the convention says (AGENTS.md:121) -- shorter is fine here because the full reasoning now lives in the gotcha document and the changelog, which are the files that can settle it. meson test 11/11, check-version clean.
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/gpu_nvidia.c`:
- Around line 77-87: The Nvidia block-linear unsupported-modifier path must set
a human-readable context error before returning -ENOTSUP, while retaining the
existing debug log and errno behavior. Update the return path at
src/gpu_nvidia.c:77-87 and apply the identical change at
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c:77-87, using set_error to describe
the unsupported modifier.
🪄 Autofix
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: 5dfed604-625f-44b0-a3a3-d457c84d0768
📒 Files selected for processing (18)
AGENTS.mdCHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap/Cargo.tomldocs/research/06_github_issues_analysis.mdinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Every C source and header file must begin with the libdrmtap copyright, project, license, and file documentation header block.
Public API changes must update comments ininclude/drmtap.h; newly discovered gotchas must updatedocs/research/06_github_issues_analysis.md; architecture changes must updatedocs/research/05_api_and_architecture.md.
Files:
bindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/pixel_convert.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.csrc/gpu_nvidia.csrc/drmtap.csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use
UPPER_SNAKE_CASEmacros prefixed withDRMTAP_, and protect headers with guards such asDRMTAP_MODULE_H.
Files:
include/drmtap.h
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
Document public API functions with Doxygen comments including purpose, parameters, return values, and relevant error codes.
Files:
include/drmtap.h
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Use C11 style with 4-space indentation, no tabs, snake_case names, same-line braces, and a 100-character soft/120-character hard line limit in source files.
Public C functions must use thedrmtap_prefix; internal types usesnake_case_t, and public types use thedrmtap_*convention.
Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Internal static functions should use concise//comments rather than full public-API Doxygen documentation.
Return negative errno values on failure, never abort or crash, set human-readable context errors withset_error, and clean up resources on all error paths using agoto cleanuppattern.
Always check allocation results and release allocated or acquired resources during cleanup.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper.
Use the PRIME path rather than GEM_FLINK, because GEM_FLINK does not work with vkms.
Check the DRM modifier;DRM_FORMAT_MOD_LINEARrequires no deswizzle.
Files:
src/pixel_convert.csrc/gpu_nvidia.csrc/drmtap.csrc/drm_grab.c
src/{pixel_convert,gpu_egl}.c
📄 CodeRabbit inference engine (AGENTS.md)
For HDR connectors reporting
HDR_OUTPUT_METADATAor PQ, tone-map AR30/XR30 and 16-bit XR48/AR48/XB48/AB48 scanouts from PQ BT.2020 to tone-mapped sRGB BT.709 8-bit output in both CPU and EGL paths; plain SDR 10-bit uses bit-depth reduction, while P010 and HLG are not tone-mapped.
Files:
src/pixel_convert.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Documentation must be written in English; research documents must include dates and sources, and DRM API references should link to kernel documentation or upstream sources.
Files:
docs/research/06_github_issues_analysis.md
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: New functions should have corresponding tests when possible; GPU-specific exceptions must be documented when real hardware is required.
Use simple assert-based tests without an external test framework.
Do not hardcode/dev/dri/card0; integration tests must use theDRM_DEVICEenvironment variable.
Do not test tiling or deswizzle behavior against vkms because vkms is linear-only; account for the requirement that vkms needs a compositor and active planes.
Keep exactly two test suites,unitandintegration; use unit tests for hardware-independent functionality and integration tests for DRM devices, vkms, or real GPUs.
Files:
tests/test_deswizzle.c
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()for every frame.
Files:
src/drm_grab.c
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
The project must compile with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
meson.build
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T23:20:15.983Z
Learning: Each pull request should address one concern, include test results, and reference research documentation when the change is based on a finding.
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-07T23:20:15.983Z
Learning: Before starting work, read the relevant research documents, check the known-gotcha checklist, and understand the architecture layers without mixing grab, enumerate, convert, and helper concerns.
🪛 Clang (14.0.6)
tests/test_deswizzle.c
[warning] 190-190: variable 'mod_nvidia' is not initialized
(cppcoreguidelines-init-variables)
src/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
🔇 Additional comments (18)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 877-877, 908-908, 1147-1147, 1268-1277, 2018-2019
src/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 877-877, 908-908, 1147-1147, 1268-1277, 2018-2019
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c (1)
32-41: LGTM!Also applies to: 61-66
src/gpu_nvidia.c (1)
32-41: LGTM!Also applies to: 61-66
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
include/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
240-240: LGTM!src/drmtap.c (1)
240-240: LGTM!bindings/rust/libdrmtap/Cargo.toml (1)
3-3: LGTM!Also applies to: 16-16
meson.build (1)
4-4: LGTM!Also applies to: 313-316
bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
src/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
tests/test_deswizzle.c (1)
20-23: LGTM!Also applies to: 178-201
README.md (1)
84-85: LGTM!Also applies to: 115-115
AGENTS.md (1)
388-388: LGTM!docs/research/06_github_issues_analysis.md (1)
169-192: LGTM!Also applies to: 274-275
CHANGELOG.md (1)
8-152: LGTM!Also applies to: 634-634
bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!
The review found one; the sweep found four. `gpu_nvidia.c`, `gpu_intel.c`, `gpu_generic.c` and the missing-EGL-procs check in `gpu_egl.c` all returned -ENOTSUP after a `drmtap_debug_log`, so a caller with debug logging off got the errno and an empty `drmtap_error()`. The equivalent returns in `drm_grab.c` already used `drmtap_set_error`; these are the ones that did not. Same class as the diagnostics work already in this release, and the same shape as the reason for it: a failure that is only visible if you already knew to turn on logging is a failure nobody will diagnose. meson test 11/11, check-version clean.
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/gpu_egl.c`:
- Around line 577-579: Update load_egl_procs() and the egl_init() path to
preserve a distinct result for load_gl_libraries() failures instead of
collapsing it into the missing-procedure error. Have egl_init() distinguish
library-loading failures from unavailable EGL symbols and set an appropriate
message covering both cases.
In `@src/gpu_nvidia.c`:
- Around line 37-41: Rename the NV_VENDOR macro to DRMTAP_NV_VENDOR in
src/gpu_nvidia.c lines 37-41 and update its comparison around line 76. Apply the
same macro rename and comparison update in
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c lines 37-41, preserving the
existing Nvidia vendor matching behavior.
🪄 Autofix
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: 8f4ec8bc-2ecb-40f2-92ff-de371bdbcb06
📒 Files selected for processing (24)
AGENTS.mdCHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_egl.cbindings/rust/libdrmtap-sys/csrc/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap/Cargo.tomldocs/research/06_github_issues_analysis.mdinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/gpu_egl.csrc/gpu_generic.csrc/gpu_intel.csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation and no tabs, snake_case names, same-line braces (1TBS), 100-character soft and 120-character hard line limits, and the specified comment conventions.
Every C source and header file must begin with the project copyright, SPDX license, and file documentation header block.
Ensure code compiles with Meson using-Wall -Wextra -Werrorwith zero warnings, and ensure all existing tests pass before merge.
Files:
src/gpu_generic.csrc/gpu_intel.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/drmtap.ctests/test_deswizzle.csrc/gpu_egl.csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/gpu_nvidia.csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/gpu_egl.csrc/drm_grab.c
**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
Prefix public C API functions with
drmtap_; usesnake_casefor variables,UPPER_SNAKE_CASEwith aDRMTAP_prefix for macros/constants, andsnake_case_tordrmtap_*for types as appropriate.
Files:
src/gpu_generic.csrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/drmtap.ctests/test_deswizzle.csrc/gpu_egl.csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.csrc/gpu_nvidia.csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/gpu_egl.csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Organize implementation files into grouped system/library/project includes, private types and constants, static functions, and public functions in header declaration order.
Use simple//comments for internal static functions.
Always check allocation results and release allocated resources during cleanup.
Use the Prime path rather than GEM_FLINK, because GEM_FLINK does not work with vkms.
Files:
src/gpu_generic.csrc/gpu_intel.csrc/gpu_egl.csrc/drmtap.csrc/gpu_nvidia.csrc/pixel_convert.csrc/drm_grab.c
docs/research/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Write documentation in English, include dates and sources in research documents, link to kernel or upstream sources for DRM API references, and update the gotcha checklist when discovering issues.
Files:
docs/research/06_github_issues_analysis.md
**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use header guards in the form
#ifndef DRMTAP_MODULE_H/#define DRMTAP_MODULE_H.
Files:
include/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.h
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
include/drmtap.h: Document every public API function with Doxygen comments including its purpose, parameters, return value, and relevant error codes.
Update public API comments whenever the public API changes.
Files:
include/drmtap.h
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: Tests must use simple assert-based validation without an external test framework.
New functions should have corresponding tests when possible; document exceptions for GPU-specific code requiring real hardware.
Do not test tiling or deswizzle against vkms because vkms is linear-only; useDRM_DEVICErather than hardcoding/dev/dri/card0, and account for vkms requiring a compositor.
Files:
tests/test_deswizzle.c
src/{drm_grab,pixel_convert,gpu_egl}.c
📄 CodeRabbit inference engine (AGENTS.md)
Check the framebuffer modifier;
DRM_FORMAT_MOD_LINEARrequires no deswizzle. Tiled or compressed framebuffers should usegpu_egl.cas the primary detile path, with CPU deswizzle only as a fallback for supported formats.
Files:
src/gpu_egl.csrc/pixel_convert.csrc/drm_grab.c
src/{pixel_convert,gpu_egl}.c
📄 CodeRabbit inference engine (AGENTS.md)
For HDR connectors reporting
HDR_OUTPUT_METADATAor PQ, tone-map supported 10-bit and 16-bit PQ scanouts to 8-bit sRGB, including PQ decode and BT.2020-to-BT.709 gamut mapping. Do not claim P010 or HLG support; plain SDR 10-bit uses straight bit-depth reduction.
Files:
src/gpu_egl.csrc/pixel_convert.c
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()on every frame.
Files:
src/drm_grab.c
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-08T03:19:57.629Z
Learning: Before coding, read the relevant research documents, check the gotcha checklist, and understand the separation between grab, enumerate, convert, and helper architecture layers.
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-08T03:19:57.629Z
Learning: Return negative errno values for errors, never abort or crash, set a human-readable error with `set_error(ctx, ...)`, and clean up resources on every error path using a `goto cleanup` pattern.
Learnt from: CR
Repo: fxd0h/libdrmtap
Timestamp: 2026-08-08T03:19:57.629Z
Learning: Keep pull requests focused on one concern; include test results and relevant research-document references.
🪛 Clang (14.0.6)
tests/test_deswizzle.c
[warning] 190-190: variable 'mod_nvidia' is not initialized
(cppcoreguidelines-init-variables)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
src/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
🔇 Additional comments (23)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 877-877, 908-908, 1147-1147, 1268-1277, 2018-2019
src/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 877-877, 908-908, 1147-1147, 1268-1277, 2018-2019
src/gpu_generic.c (1)
69-71: LGTM!src/gpu_intel.c (1)
114-116: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c (1)
61-66: LGTM!Also applies to: 77-87
src/gpu_nvidia.c (1)
61-66: LGTM!Also applies to: 77-87
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
include/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
240-240: LGTM!src/drmtap.c (1)
240-240: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_egl.c (1)
578-578: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_generic.c (1)
69-69: LGTM!bindings/rust/libdrmtap/Cargo.toml (1)
3-3: LGTM!Also applies to: 16-16
meson.build (1)
4-4: LGTM!Also applies to: 313-316
bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
src/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
tests/test_deswizzle.c (1)
20-23: LGTM!Also applies to: 178-201
README.md (1)
84-85: LGTM!Also applies to: 115-115
AGENTS.md (1)
388-388: LGTM!docs/research/06_github_issues_analysis.md (1)
169-192: LGTM!Also applies to: 274-275
CHANGELOG.md (1)
8-157: LGTM!Also applies to: 639-639
bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_intel.c (1)
114-114: LGTM!
…the macro Two from the review. `egl_init` reported "required EGL procs not available" for both "libEGL and libGLESv2 are not installed" and "they are, but a symbol is missing". Those are a missing package and a driver too old, and they need different actions from whoever reads the error. `load_egl_procs` returns -2 for the first now, and the message names the packages. And `NV_VENDOR` gets the `DRMTAP_` prefix the naming rule asks for (AGENTS.md:58), since this release changes that macro anyway. Noted, not done: about twenty file-local macros in `src/` predate that rule and still lack the prefix (`AMD_VENDOR`, `INTEL_*`, `I915_MOD_*`, `NV_TILE_*`, `RESP_OK`, ...). A release PR is the wrong place for that rename, and it wants a decision first about whether the rule means file-local macros at all. meson test 11/11, check-version clean.
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/drm_grab.c`:
- Line 877: In the geometry validation error path around drmtap_gem_close,
update both src/drm_grab.c:877-877 and
bindings/rust/libdrmtap-sys/csrc/drm_grab.c:877-877 to set ret and jump to the
shared cleanup label instead of manually releasing the GEM/framebuffer and
returning. Preserve the existing error reporting and ensure both mirrored
implementations use the common cleanup path.
In `@src/gpu_egl.c`:
- Around line 337-339: Preserve distinct EGL loader failure statuses instead of
collapsing every negative result to -2. Update load_gl_libraries() handling in
src/gpu_egl.c:337-339 and bindings/rust/libdrmtap-sys/csrc/gpu_egl.c:337-339 so
load_egl_procs() returns the original negative status unchanged, allowing
egl_init() to distinguish missing libraries from missing core symbols in both
implementations.
🪄 Autofix
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: 1cca7219-f891-4a56-9bf9-682fab1b83e5
📒 Files selected for processing (24)
AGENTS.mdCHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_egl.cbindings/rust/libdrmtap-sys/csrc/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap/Cargo.tomldocs/research/06_github_issues_analysis.mdinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/gpu_egl.csrc/gpu_generic.csrc/gpu_intel.csrc/gpu_nvidia.csrc/pixel_convert.ctests/test_deswizzle.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (16)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep one shared version line across the C library, Meson,libdrmtap-sys, andlibdrmtap; useinclude/drmtap.hDRMTAP_VERSION_*as canonical and validate it withtools/check-version.sh.
Before coding, read the architecture and gotcha research documents and keep concerns separated between grab, enumerate, convert, and helper layers.
Each pull request should address one concern and include test results, with references to research documents when applicable.
Files:
bindings/rust/libdrmtap-sys/Cargo.tomlsrc/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/drmtap.hmeson.buildAGENTS.mdsrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/gpu_intel.csrc/gpu_nvidia.csrc/drmtap.ctests/test_deswizzle.csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/gpu_generic.csrc/gpu_egl.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/gpu_egl.cCHANGELOG.mdsrc/drm_grab.cREADME.mddocs/research/06_github_issues_analysis.md
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style: 4-space indentation, never tabs; snake_case for variables and functions;drmtap_prefix for public functions; UPPER_SNAKE_CASE withDRMTAP_prefix for public macros/constants; same-line braces; 100-character soft and 120-character hard line limits; and//or/* */comments as appropriate.
Return negative errno values for errors, never abort, check allocation results, and release allocated resources during cleanup.
Every C source and header file must begin with the specified libdrmtap copyright, SPDX,@file, and@briefheader block.
Usegoto cleanuperror paths, return negative errno values, set a human-readable error withset_error(ctx, ...), and clean up all resources when errors occur.
Changes must compile with Meson using-Wall -Wextra -Werrorwithout warnings and must preserve passing tests.
Files:
src/gpu_generic.cbindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/gpu_intel.cbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/gpu_intel.csrc/gpu_nvidia.csrc/drmtap.ctests/test_deswizzle.csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/gpu_generic.csrc/gpu_egl.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_nvidia.cbindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/gpu_egl.csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Document internal static functions with concise//comments.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper.
Files:
src/gpu_generic.csrc/gpu_intel.csrc/gpu_nvidia.csrc/drmtap.csrc/pixel_convert.csrc/gpu_egl.csrc/drm_grab.c
src/{drm_grab,gpu_egl,pixel_convert,gpu_intel,gpu_amd,gpu_nvidia,gpu_generic}.c
📄 CodeRabbit inference engine (AGENTS.md)
Use the Prime path rather than GEM_FLINK, because GEM_FLINK does not work with vkms.
Files:
src/gpu_generic.csrc/gpu_intel.csrc/gpu_nvidia.csrc/pixel_convert.csrc/gpu_egl.csrc/drm_grab.c
src/{gpu_egl,pixel_convert,gpu_intel,gpu_amd,gpu_nvidia,gpu_generic}.c
📄 CodeRabbit inference engine (AGENTS.md)
Check the framebuffer modifier;
DRM_FORMAT_MOD_LINEARrequires no deswizzle.
Files:
src/gpu_generic.csrc/gpu_intel.csrc/gpu_nvidia.csrc/pixel_convert.csrc/gpu_egl.c
tests/*.c
📄 CodeRabbit inference engine (AGENTS.md)
New functions should have corresponding tests when possible; GPU-specific exceptions must document the hardware requirement.
Files:
tests/test_deswizzle.c
tests/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Use simple assert-based tests without an external test framework.
Files:
tests/test_deswizzle.c
tests/test_{deswizzle,formats,helper}.c
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests for formats, deswizzle, and helper behavior must not require hardware.
Files:
tests/test_deswizzle.c
tests/test_{capture,enumerate,deswizzle}.c
📄 CodeRabbit inference engine (AGENTS.md)
Do not test tiling or deswizzle against vkms because vkms is linear-only; bare vkms also needs a compositor to provide active planes.
Files:
tests/test_deswizzle.c
src/{pixel_convert,gpu_egl}.c
📄 CodeRabbit inference engine (AGENTS.md)
When HDR metadata indicates HDR output, PQ-decode supported 10/16-bit scanouts, map BT.2020 to BT.709, tone-map, and sRGB-encode to 8-bit in both CPU and EGL paths; do not claim P010 or HLG support. Plain SDR 10-bit uses straight bit-depth reduction.
Files:
src/pixel_convert.csrc/gpu_egl.c
src/gpu_egl.c
📄 CodeRabbit inference engine (AGENTS.md)
EGL is the primary detile path: import tiled or compressed DMA-BUFs as EGLImages, render, and use
glReadPixelsto produce linear RGBA; CPU deswizzle is only a fallback for supported formats.
Files:
src/gpu_egl.c
include/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use header guards in the form
#ifndef DRMTAP_MODULE_H/#define DRMTAP_MODULE_Hfor headers.
Files:
include/drmtap.h
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
include/drmtap.h: Document public API functions with Doxygen comments describing behavior, parameters, return values, and relevant error codes.
Update public API comments whenever the public API changes.
Files:
include/drmtap.h
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()on every frame.
Files:
src/drm_grab.c
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Write documentation in English; research documents must include dates and sources, link kernel or upstream sources for DRM APIs, and update the gotcha checklist when new issues are found.
Files:
docs/research/06_github_issues_analysis.md
docs/research/06_github_issues_analysis.md
📄 CodeRabbit inference engine (AGENTS.md)
Document newly discovered gotchas in the gotcha analysis document.
Files:
docs/research/06_github_issues_analysis.md
🪛 Clang (14.0.6)
tests/test_deswizzle.c
[warning] 190-190: variable 'mod_nvidia' is not initialized
(cppcoreguidelines-init-variables)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
src/drm_grab.c
[warning] 84-84: loop variable name 'p' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 85-85: variable name 'h' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 90-90: loop variable name 'q' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 865-865: variable 'pending_gem' is not initialized
(cppcoreguidelines-init-variables)
[note] 1274-1274: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1276-1276: +1, nesting level increased to 1
(clang)
🔇 Additional comments (24)
bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!bindings/rust/libdrmtap/Cargo.toml (1)
3-3: LGTM!Also applies to: 16-16
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 908-908, 1147-1147, 1268-1277, 2018-2019
src/drm_grab.c (1)
79-101: LGTM!Also applies to: 726-732, 860-867, 908-908, 1147-1147, 1268-1277, 2018-2019
bindings/rust/libdrmtap-sys/csrc/gpu_nvidia.c (1)
32-41: LGTM!Also applies to: 61-66, 76-87
src/gpu_nvidia.c (1)
32-41: LGTM!Also applies to: 61-66, 76-87
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
240-240: LGTM!src/drmtap.c (1)
240-240: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_egl.c (1)
579-583: LGTM!src/gpu_egl.c (1)
579-583: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_generic.c (1)
69-71: LGTM!bindings/rust/libdrmtap-sys/csrc/gpu_intel.c (1)
114-116: LGTM!src/gpu_generic.c (1)
69-71: LGTM!src/gpu_intel.c (1)
114-116: LGTM!bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
src/pixel_convert.c (1)
162-171: LGTM!Also applies to: 283-283, 320-338
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
include/drmtap.h (1)
34-34: LGTM!Also applies to: 596-603
tests/test_deswizzle.c (1)
20-23: LGTM!Also applies to: 178-201
README.md (1)
84-85: LGTM!Also applies to: 115-115
meson.build (1)
4-4: LGTM!Also applies to: 313-316
AGENTS.md (1)
388-388: LGTM!docs/research/06_github_issues_analysis.md (1)
169-192: LGTM!Also applies to: 274-275
CHANGELOG.md (1)
8-157: LGTM!Also applies to: 639-639
…t the caller The previous commit claimed to distinguish "libEGL is not installed" from "it is, but a symbol is missing", and did not: `load_gl_libraries()` returns the same -1 for both, so mapping every negative to -2 in `load_egl_procs()` reported the missing-package remedy for a driver that was merely too old. The same conflation the commit was written to remove, one function further up. `load_gl_libraries()` now returns -1 when the libraries are absent and -2 once they have loaded and only a symbol failed to resolve, and the caller propagates the value instead of rewriting it. Also routes the framebuffer-geometry rejection through `cleanup:` like the rest of that function, per the guideline. It is not only tidier: the manual path returned without clearing `frame->_priv` and `frame->dma_buf_fd`, which the label does, so a caller that defensively releases on a non-zero return was relying on those already being unset. meson test 11/11, check-version clean, real capture on Intel Meteor Lake.
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…in four places Audited before cutting, against the code rather than the previous text. `AGENTS.md` and the README both said the CPU decoder was "unvalidated and not wired up", which understates it now: it is gone. And the DRM/KMS research document still told a reader that Nvidia "requires X-TILED deswizzle (16×128)" and listed "Manual CPU deswizzle" as the Nvidia strategy -- describing the exact function this release deleted, and describing it as the way to do it. That one is worth the words rather than a silent edit: the 16x128 decoder tested vendor byte 0x10, which is not a vendor, so it never ran on a real modifier and was never validated. A reader following that table would have reimplemented it. Code untouched: this commit is four markdown files.
The previous commit rewrote the Nvidia row of the modifier table to say "EGL only -- CPU returns -ENOTSUP". That row is in a survey of what the ECOSYSTEM does -- the other five say VAAPI, SDMA copy, INTEL_DEBUG=noccs -- so it now claimed one thing about this library in a column that describes everyone else, and left the table internally inconsistent. Row restored to the survey form it belongs to, with the modifier encoding corrected. What libdrmtap itself does is stated once, below the table, where the page already does that: the CPU path decodes only Intel X/Y/Yf-tiled and every other modifier returns -ENOTSUP. The reader risk that prompted the first edit is addressed there instead, and more directly: do not implement a row of that table from this page. The Nvidia one was implemented once and never ran. Third doc edit today that needed a second pass. Code untouched.
1 participant
Cuts 0.5.3. No API or ABI change; the CHANGELOG entry has the detail.
0x10, whereDRM_FORMAT_MOD_VENDOR_NVIDIAis0x03), so it was unreachable and every Nvidia-vendor modifier fell through to the linear memcpy. Routed to-ENOTSUPand the dead function removed.drm_grab.c, one per early return betweendrmModeGetFB2and the close.variableScopefinding that would have gone red on the next CI bump.Verified here:
meson test11/11,tools/check-version.shclean across the six version sites, and a real capture throughexamples/screenshoton Intel Meteor Lake.Publishing to crates.io is deliberately NOT part of this PR.