Report the scanned-out width, and let the caller supply the output buffer - #42
Conversation
A driver may allocate the scanout framebuffer wider than the mode to satisfy a pitch alignment. Measured on an Apple T2 MacBook: the Touch Bar card (appletbdrm) drives a 60x2170 mode out of a 64x2170 framebuffer with a 256-byte pitch, and those four columns are padding that is never scanned out. Reporting the framebuffer width hands the caller an image wider than the display it asked to capture. In rustdesk, whose display list carries the CRTC mode, that read as "this display never matched its advertised geometry": every frame was rejected, the display was demoted to PipeWire, and the client sat on "waiting for image". Narrow the reported width to the mode. It only ever shrinks, and only the width: - A framebuffer narrower than the mode means the CRTC is scaling a smaller buffer up. That is a different image, not padding, so it is left as it is. - A CRTC whose viewport does not start at the framebuffer origin (several heads scanning out of one big framebuffer) needs an offset crop as well. drmtap_dmabuf_desc has a frozen layout with nowhere to carry a crop origin, so such a frame is reported whole and the reason is logged once. No hardware here has that layout, so it is deliberately not guessed at. Applied at the four places that report geometry: the direct path, the helper (V2) path, the fast path's EGL fallback, and the fast path's cached slot, so a cache hit replays the narrowed width without paying another drmModeGetCrtc on the hot path. Buffer-size arithmetic keeps using fb2->pitches[0] * fb2->height: the mapping is still of the whole padded framebuffer, only the reported width narrows, and the caller reads rows out of it at the unchanged stride. The decision itself is a pure function, so it is asserted on any machine rather than only on the one laptop in the world that pads: tests/test_scanout.c covers the measured Touch Bar and amdgpu numbers, the upscaling CRTC, the offset viewport, and an invalid mode (which must never yield a zero-width frame). No ABI change: drmtap_scanout_width_of is internal, kept out of the exported set by libdrmtap.map, and the exported symbol count is unchanged.
A converted frame landed in a library-owned buffer, so a consumer that has to end up with the pixels in specific memory paid a full-frame memcpy per frame: 14.7 MB at 2560x1440, every frame, for nothing (issue #36). Nothing required it. The EGL detile ends in a glReadPixels and the CPU converters take a destination pointer, and both write wherever they are pointed. drmtap_set_output_buffer(ctx, dst, len) points them at the caller's memory. Every producer of converted pixels now resolves its destination through one drmtap_ensure_out(), so the EGL detile, the CPU deswizzle, the 10/16-bit and HDR reductions and the padded-linear repack all behave identically: a caller must not have to know which path ran to know where its pixels went. It works on an unprivileged drmtap_open_render() context too, so the converting half of the split can write straight into the consumer's buffer. Where there is nothing to materialize -- a linear 8-bit scanout, handed back as a direct pointer into the mapped scanout -- there is no intermediate buffer to redirect, and the header says so rather than implying a promise it cannot keep. The bound is the whole safety story, since the pointer and the length come from outside the library: - A frame that needs more than len is REFUSED with -ENOSPC, leaving the buffer untouched and naming the required byte count through drmtap_error(). A short frame would be indistinguishable from a good one. - Framebuffer DIMENSIONS are now bounded at every entry point. validate_fb_size bounds stride*height and never constrained width, yet every converted output is sized width*height*4 -- a product that can wrap size_t and give a large write a small destination, reachable with a width from the helper wire or an IPC-supplied descriptor. Each dimension is bounded before anything multiplies it, so the product cannot wrap even where size_t is 32-bit, and the pixel count is bounded after. This closes the same hole for the library's own buffer, where it also existed. - The frame cap applies to a caller-supplied buffer as well, so owning a large mapping cannot unlock a frame larger than this library handles. Also stops allocating the CPU-deswizzle shadow buffer before the EGL attempt: the CPU path keeps the source stride and needs stride*height, which on a padded scanout exceeds the width*height*4 that the detile produces, so sizing it up front would refuse a caller buffer the EGL path would have filled perfectly -- and on the EGL path it allocated a buffer that was then never used. tests/test_outbuf.c covers the refusal and its off-by-one either way, the argument checks, the reset, the frame cap and the wrap-proof dimension bound. Verified on hardware against a tiled XR30 scanout: -ENOSPC with the contents intact on a buffer one byte short, then a correctly sized buffer receiving the frame with guard bytes either side untouched, clean under ASan and UBSan.
tools/check-version.sh was reporting csrc/ drift. libdrmtap-sys embeds and statically compiles those copies, so publishing without syncing would have shipped a crate with neither the scanout-width fix nor the output buffer in it, while the meson build had both -- the two would silently disagree. Also declares drmtap_set_output_buffer in the sys crate, since the whole point of it is for a consumer that owns the destination memory, which is exactly what a Rust consumer of this crate is doing. README: the AMD backend is now verified on two generations by two different people, so say which is which -- Vega 64 confirmed by an outside tester, RX560 (Polaris) verified here -- rather than one flat "verified on real hardware". Drops the claim that a multi-card host with several display-driving GPUs is unverified for lack of hardware: an Apple T2 MacBook has three cards across two vendors with two of them scanning out at once, and each was captured from its own card. Records the two cases that machine is the only cover for: a display-only card with no render node, and a scanout framebuffer padded wider than its mode.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
Walkthroughlibdrmtap 0.5.0 adds caller-owned conversion output buffers, tighter framebuffer and helper-wire validation, and scanout-width reporting based on primary-plane geometry. CPU, EGL, fast, and IPC paths use the resolved destination, with new unit tests and release documentation. ChangesCapture safety and output routing
Estimated code review effort: 4 (Complex) | ~60 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 |
Two defects found by tracing the new signals into pre-existing code rather than
re-reading the diff, which is how the last two rounds of real bugs were found.
1. Narrowing the reported width broke the CPU deswizzle of a padded TILED scanout.
deswizzle_nvidia_x_tiled and its siblings derive the tile grid from the WIDTH and
use it to address the SOURCE:
tiles_x = (width + tile_w - 1) / tile_w;
tile_idx = tile_row * tiles_x + tx;
src_off = tile_idx * tile_size + tile_y * tile_w_bytes;
They ignore src_stride entirely. So a width short by a tile or more shrinks
tiles_x and mis-addresses every tile row after the first: a silently mangled
image, on a path that worked before this feature. The visible width and the width
the tiling was laid out for are two different quantities -- the same distinction
this function exists for, one level further down.
Narrowing is now confined to a LINEAR (or INVALID, handled downstream as linear)
layout, where the only width-dependent consumer is the OUTPUT and rows are
addressed through the stride. A padded tiled scanout keeps the previous behaviour
and says why once. No padded tiled scanout has been observed on any hardware here,
so this is deliberately not narrowed on a guess -- and it also removes an
unverified claim, since an EGL import of a narrowed width against a tiled
allocation was never exercised either.
The measured case is unaffected: appletbdrm reports modifier 0x0, and re-verified
on that machine after the change -- 64x2170 in, 60x2170 stride=256 out, the same
475038 of 520800 bytes non-black. The tiled i915 path here is untouched (fb width
already equals the mode) and still detiles to 5242876 of 5242880 non-black.
Also names fb2_effective_modifier(): the DRM_MODE_FB_MODIFIERS rule was open-coded
identically at three sites and the width decision needs the same answer.
2. The output-buffer contract promised more than it delivers on the helper path.
The header said a frame with no conversion "points directly at the mapped scanout".
True on the direct path, false on the privileged-helper pixel path, where the
pixels arrive over the socket into a LIBRARY buffer -- so a caller that must own
the memory still copies there. Says which is which now, rather than implying a
guarantee it cannot keep on that path.
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
A framebuffer wider than its mode looks IDENTICAL whether those extra columns are
pitch padding that is never scanned out or a primary plane that genuinely
downscales the whole framebuffer onto a smaller mode. The mode cannot tell them
apart. Narrowing on that signal alone silently returned the left part of the image
as if it were the whole screen, for anyone whose compositor scales a plane.
The plane rect settles it. Measured on the machine this came from:
plane 35 crtc 37 PRIMARY
mode 60x2170 fb 64x2170 pitch 256
SRC 60.0000 x 2170 at +0+0 CRTC_W/H 60 x 2170 -> 1:1
The plane reads 60 columns and puts them down 1:1, so the four extra columns are
provably not scanned out. A downscale declares SRC 3840 onto CRTC_W 1920 and is
refused. The mode is now only the cheap gate that says "worth looking closer".
A pitch test cannot substitute for this, which is worth recording because it looks
like it should: fb_width * bpp == pitches[0] holds for a 3840-wide downscaled
framebuffer exactly as it does for the padded 64-wide one.
Every branch that declines to narrow is now a case where the framebuffer width is
the right answer, and each reports a distinct reason so the log says which:
scaling plane, non-zero source origin (several heads out of one framebuffer, which
needs an offset crop the frozen drmtap_dmabuf_desc cannot carry), tiled layout, and
an unreadable plane rect. That last one is deliberately NOT folded into "no
scaling": without the rect the two situations are indistinguishable, so the safe
answer is the pre-existing behaviour.
Reading the rect needs DRM_CLIENT_CAP_ATOMIC. The SRC and CRTC properties are
hidden from a non-atomic client, measured both ways on the same plane: SRC_W
present with the cap, absent without it. The cap is requested LAZILY, only once a
framebuffer has turned out to be wider than its mode, so the common path never
touches it. It is a per-fd flag, needs no privilege and no DRM master, no atomic
commit is ever issued, and no other client is affected.
Re-verified after the change. The Mac still narrows, now through the plane:
"scans out 60 of the 64-pixel-wide scanout fb (pitch padding; plane reads SRC
60x2170 1:1)", 64x2170 in, 60x2170 stride=256 out, the same 475038 of 520800 bytes
non-black. The tiled i915 path here never reaches the plane query (its fb already
equals the mode) and still detiles to 5242876 of 5242880. 10/10 tests including the
rewritten scanout suite, and 10/10 again under ASan and UBSan; the output-buffer
hardware probe is unchanged, guard bytes intact.
tests/test_scanout.c is rewritten around the plane rect: the measured padded case,
the amdgpu 1:1 case, a downscaling plane, an upscaling plane, an offset source, a
padded tiled scanout, an unreadable and a NULL rect, and a zero source width that
must never yield a zero-width frame.
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The helper wire and the IPC descriptor are the two places where geometry is attacker-influenced, and the claim was that both are bounded on everything that feeds a size computation. They were not bounded alike. validate_fb_size bounds stride * height and validate_fb_dims bounds the dimensions, but neither implies that the stride covers width pixels. A wire frame claiming width * bpp > stride reached the per-pixel CPU converters, which read y * stride + width * bpp per row with no size argument, so the last rows ran off the end of the receive buffer. drmtap_convert_dmabuf has applied exactly this check to its descriptor since 0.4.12; the wire now applies it as well. Pre-existing rather than introduced here, but it contradicts what the previous commit asserts, and the helper being our own setcap binary is a reason to bound it, not to trust it. Also fixes three comments that still described the old ownership model and would mislead the next person tracing these paths: two claimed a conversion reads back into ctx->deswizzle_buf, and the internal doc for drmtap_gpu_egl_convert said *out_data points at that buffer. Any of them can be the caller's output buffer now. And documents that the output buffer must not overlap the frame source: the CPU converters read the scanout and write the destination at different strides, so aiming it at a mapping of the dma-buf being captured would have them read bytes they had already written. Nothing detects that, and before this API no such aliasing was possible. Re-verified on hardware after the change: the Mac still narrows through the plane rect, 60x2170 stride=256, the same 475038 of 520800 bytes non-black. 10/10 tests.
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
A minor bump, not a patch, for one reason: the scanout-width fix changes the SEMANTICS of an existing call. Any consumer reading drmtap_frame_info.width can now get a different number for the same hardware. Nothing was removed and the soname is unchanged (soversion stays '0', so the file becomes libdrmtap.so.0.5.0 behind the same libdrmtap.so.0), which means a dlopen-by-soname consumer keeps loading it. That is exactly why the version has to say so: a consumer built against 0.4.x semantics would otherwise pick up different widths with no signal at all. rustdesk pins the accepted ABI minor for this reason and will correctly refuse this .so until that pin is moved deliberately -- the gate working, not breakage. The new entry point (drmtap_set_output_buffer) is additive and would not on its own have justified more than a patch, and the AMD coverage note is documentation only. Stamped with tools/set-version.sh across the five code sites; tools/check-version.sh reports coherence and csrc/ in sync; 10/10 C tests and both Rust crates build and test at 0.5.0; 24 exported symbols unchanged from the previous commit.
An output buffer too small for the frame is a caller configuration error, not an EGL failure, but gpu_auto_process logged the detile return and fell through to the CPU deswizzle. That silently broke the contract drmtap_set_output_buffer documents, and it broke it on exactly the hardware the detile exists for. The CPU path needs stride*height, never less than the width*height*4 the detile just refused, so falling through can only fail again. Where there IS a CPU mapping it fails with -ENOSPC anyway and the caller sees the right errno for the wrong reason. Where there is NOT one -- amdgpu GFX9 discrete VRAM, the case that depends on the detile -- it fails with -ENOTSUP and a message about a missing CPU mapping, so the caller is told its GPU is unsupported when in truth its buffer was a few bytes short, and the byte count -ENOSPC would have named is gone. Worth recording how this got past the probe: the hardware here masked it. The output-buffer probe asserted -ENOSPC on an i915 tiled scanout and passed, because that scanout IS CPU-mappable, so the fallthrough reached the CPU path and that path raised -ENOSPC itself. The reporter GPU would have taken the other branch. Passing on the hardware you have is not the same as passing on the hardware that matters.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@CHANGELOG.md`:
- Around line 39-56: Update the affected CHANGELOG entries by wrapping each
multiplication expression containing `*` in inline code spans, including `stride
* height`, `width * height * 4`, `width * bpp`, and `y * stride + width * bpp`,
while preserving the surrounding wording and formatting.
In `@src/drm_grab.c`:
- Around line 591-604: The read_primary_plane_rect implementations in
src/drm_grab.c lines 591-604 and bindings/rust/libdrmtap-sys/csrc/drm_grab.c
lines 591-604 must stop using find_primary_plane because it mutates cur_hdr_eotf
and cur_hdr_max_nits through HDR metadata reads. Extract or reuse a
side-effect-free primary-plane lookup that omits read_hdr_metadata_direct, and
call it from read_primary_plane_rect in both copies while preserving the
existing helper path’s HDR values.
- Around line 651-681: The per-frame scanout-width probe in drmtap_scanout_width
must be memoized by crtc_id, fb_width, and modifier, with the cached decision
invalidated by the existing modeset/hotplug signal; apply this identically in
src/drm_grab.c lines 651-681 and bindings/rust/libdrmtap-sys/csrc/drm_grab.c
lines 651-681, preserving the existing result and logging behavior while
avoiding repeated drmModeGetCrtc and read_primary_plane_rect calls.
- Around line 2013-2018: The fast-slot width change incorrectly uses scanned-out
geometry for virtio transfers. In src/drm_grab.c lines 2013-2018 and
bindings/rust/libdrmtap-sys/csrc/drm_grab.c lines 2013-2018, retain padded
fb2->width in fast_slots[].width (or add a separate transfer-width field), and
apply drmtap_scanout_width only to frame->width so both cache-hit transfer paths
copy the full framebuffer.
- Around line 1219-1226: The reduction-site drmtap_ensure_out failure returns a
bare errno instead of translating -EFBIG into an actionable drmtap_set_error
message. In src/drm_grab.c lines 1219-1226 and
bindings/rust/libdrmtap-sys/csrc/drm_grab.c lines 1219-1226, update the failure
handling in the reduction path to match the -EFBIG-to-drmtap_set_error logic
used by the deswizzle site near lines 1402-1408, preserving other errno returns.
In `@src/drmtap_internal.h`:
- Around line 284-288: Re-wrap the EGL/GLES2 universal detiling comment in
src/drmtap_internal.h lines 284-288 so no line exceeds 120 characters, ending
the caller-owned buffer sentence as its own paragraph and starting the fb_id
EGLImage-cache note on a new line. Apply the identical formatting change in
bindings/rust/libdrmtap-sys/csrc/drmtap_internal.h lines 284-288.
- Around line 275-282: Rename the externally linked validate_fb_dims symbol to
drmtap_validate_fb_dims throughout the implementation and callers. Update the
declaration in src/drmtap_internal.h and
bindings/rust/libdrmtap-sys/csrc/drmtap_internal.h, the definition in
drm_grab.c, uses in gpu_egl.c, and call sites in tests/test_outbuf.c so all
translation units use the prefixed name.
In `@src/gpu_egl.c`:
- Around line 1294-1299: Update the drmtap_ensure_out fallback in src/gpu_egl.c
at lines 1294-1299 to call drmtap_set_error with a human-readable allocation
failure message before propagating the negative errno, then mirror the same
change in bindings/rust/libdrmtap-sys/csrc/gpu_egl.c at lines 1294-1299.
Preserve the existing cleanup and return behavior.
In `@tests/test_outbuf.c`:
- Around line 80-81: Replace the integer-to-pointer sentinels in the tests
around drmtap_ensure_out with pointers derived from live local objects, such as
existing stack variables or dedicated local sentinel objects. Update both
affected test locations while preserving the assertions and sentinel behavior.
🪄 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: efcc7374-f58b-4904-abe9-cab3a5f065a3
⛔ Files ignored due to path filters (1)
libdrmtap.mapis excluded by!**/*.map
📒 Files selected for processing (18)
CHANGELOG.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/drmtap_internal.hbindings/rust/libdrmtap-sys/csrc/gpu_egl.cbindings/rust/libdrmtap-sys/src/lib.rsbindings/rust/libdrmtap/Cargo.tomlinclude/drmtap.hmeson.buildsrc/drm_grab.csrc/drmtap.csrc/drmtap_internal.hsrc/gpu_egl.ctests/test_outbuf.ctests/test_scanout.c
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (9)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Changes must compile with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
bindings/rust/libdrmtap/Cargo.tomlbindings/rust/libdrmtap-sys/Cargo.tomltests/test_scanout.cREADME.mdbindings/rust/libdrmtap-sys/csrc/drmtap.cmeson.buildsrc/drmtap.cbindings/rust/libdrmtap-sys/src/lib.rsCHANGELOG.mdinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_egl.ctests/test_outbuf.cbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/gpu_egl.csrc/drmtap_internal.hbindings/rust/libdrmtap-sys/csrc/drm_grab.csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.h
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation, no tabs, snake_case names, same-line braces, 100-character soft and 120-character hard line limits, and the specified comment conventions.
Every C source and header file must begin with the specified libdrmtap copyright, license, and file documentation header block.
Files:
tests/test_scanout.cbindings/rust/libdrmtap-sys/csrc/drmtap.csrc/drmtap.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/gpu_egl.ctests/test_outbuf.cbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/gpu_egl.csrc/drmtap_internal.hbindings/rust/libdrmtap-sys/csrc/drm_grab.csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.h
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: New functions should have corresponding tests when possible; hardware-dependent GPU code may be excepted only when the hardware requirement is documented.
Use simple assert-based tests without an external framework; separate tests intounitandintegrationsuites.
Files:
tests/test_scanout.ctests/test_outbuf.c
tests/test_*.c
📄 CodeRabbit inference engine (AGENTS.md)
Do not test tiling or deswizzle against vkms because vkms is linear-only; integration tests requiring an active plane need a compositor, and DRM devices must be selected through
DRM_DEVICE, never hardcoded.
Files:
tests/test_scanout.ctests/test_outbuf.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Prefix public C API functions withdrmtap_; use negative errno values for errors, never abort, check allocations, and free resources during cleanup.
Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Use simple//comments for internal static functions.
On errors, return negative errno values, set a human-readable error withset_error, and clean up resources on every error path, preferably using agoto cleanuppattern.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper; use the Prime path rather than GEM_FLINK.
Files:
src/drmtap.csrc/gpu_egl.csrc/drm_grab.c
src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{c,h}: Check modifiers for tiling:DRM_FORMAT_MOD_LINEARrequires no deswizzle; tiled or compressed framebuffers usegpu_egl.cas the primary detile path, with CPU deswizzle only as a fallback.
For HDR output metadata with PQ, tone-map supported 10-bit and 16-bit scanouts to 8-bit SDR using PQ decoding, BT.2020-to-BT.709 mapping, tone mapping, and sRGB encoding in both CPU and EGL paths; do not claim P010 or HLG support.
Files:
src/drmtap.csrc/gpu_egl.csrc/drmtap_internal.hsrc/drm_grab.c
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
Document public API functions with Doxygen comments, including behavior, parameters, return values, and relevant error codes.
Files:
include/drmtap.h
src/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_case_tfor internal types,drmtap_*for public types,DRMTAP_UPPER_SNAKE_CASEfor macros/constants, and conventionalDRMTAP_MODULE_Hheader guards.
Files:
src/drmtap_internal.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
🪛 Clang (14.0.6)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[warning] 558-558: variable name 'w' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 660-660: variable 'worth_looking' is not initialized
(cppcoreguidelines-init-variables)
[warning] 662-662: variable 'hdisplay' is not initialized
(cppcoreguidelines-init-variables)
[warning] 663-663: variable 'vdisplay' is not initialized
(cppcoreguidelines-init-variables)
[warning] 673-673: variable 'linear' is not initialized
(cppcoreguidelines-init-variables)
[note] 776-776: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 899-899: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 902-902: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 902-902: +1
(clang)
[warning] 1221-1221: variable name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 1222-1222: statement should be inside braces
(readability-braces-around-statements)
[warning] 1223-1223: integer literal has suffix 'u', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1225-1225: variable 'ret' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1248-1248: statement should be inside braces
(readability-braces-around-statements)
[note] 1402-1402: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1403-1403: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1411-1411: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1412-1412: +1
(clang)
[note] 1459-1459: +1, nesting level increased to 4
(clang)
[note] 1870-1870: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 2103-2103: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 2281-2281: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
src/drm_grab.c
[warning] 558-558: variable name 'w' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 660-660: variable 'worth_looking' is not initialized
(cppcoreguidelines-init-variables)
[warning] 662-662: variable 'hdisplay' is not initialized
(cppcoreguidelines-init-variables)
[warning] 663-663: variable 'vdisplay' is not initialized
(cppcoreguidelines-init-variables)
[warning] 673-673: variable 'linear' is not initialized
(cppcoreguidelines-init-variables)
[note] 776-776: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 899-899: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 902-902: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 902-902: +1
(clang)
[warning] 1221-1221: variable name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 1222-1222: statement should be inside braces
(readability-braces-around-statements)
[warning] 1223-1223: integer literal has suffix 'u', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1225-1225: variable 'ret' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1248-1248: statement should be inside braces
(readability-braces-around-statements)
[note] 1402-1402: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1403-1403: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1411-1411: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1412-1412: +1
(clang)
[note] 1459-1459: +1, nesting level increased to 4
(clang)
[note] 1870-1870: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 2103-2103: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 2281-2281: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
🪛 LanguageTool
README.md
[style] ~119-~119: Consider using “who” when you are referring to a person instead of an object.
Context: ...t, not the framebuffer's, so a consumer that enumerated > the mode is not handed a...
(THAT_WHO)
CHANGELOG.md
[style] ~13-~13: Consider an alternative for the overused word “exactly”.
Context: ...e consumer keeps loading it -- which is exactly why the version has to say so: a consum...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.23.1)
CHANGELOG.md
[warning] 40-40: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 41-41: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 50-50: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 52-52: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 53-53: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 53-53: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (18)
bindings/rust/libdrmtap-sys/csrc/gpu_egl.c (1)
1064-1075: LGTM!Also applies to: 1317-1317
src/gpu_egl.c (1)
1064-1075: LGTM!Also applies to: 1317-1317
tests/test_outbuf.c (1)
1-79: LGTM!Also applies to: 82-132, 137-180
tests/test_scanout.c (1)
1-186: LGTM!meson.build (1)
4-4: LGTM!Also applies to: 326-335, 351-352
bindings/rust/libdrmtap-sys/Cargo.toml (1)
3-3: LGTM!bindings/rust/libdrmtap/Cargo.toml (1)
16-16: LGTM!CHANGELOG.md (1)
7-38: LGTM!Also applies to: 57-97, 437-437
README.md (1)
97-121: LGTM!bindings/rust/libdrmtap-sys/csrc/drmtap_internal.h (1)
36-38: LGTM!Also applies to: 114-137, 322-351
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
356-375: LGTM!Also applies to: 377-412, 491-500, 554-581, 773-778, 878-915, 1055-1056, 1337-1340, 1390-1416, 1438-1438, 1455-1463, 1867-1872, 1958-1959, 1977-1980, 2101-2105, 2190-2191, 2279-2292
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
774-796: LGTM!src/drmtap.c (1)
774-796: LGTM!src/drmtap_internal.h (1)
36-38: LGTM!Also applies to: 114-137, 322-351
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
33-34: LGTM!Also applies to: 536-585
bindings/rust/libdrmtap-sys/src/lib.rs (1)
161-177: LGTM!include/drmtap.h (1)
33-34: LGTM!Also applies to: 536-585
src/drm_grab.c (1)
356-375: LGTM!Also applies to: 377-412, 491-500, 554-581, 773-778, 878-915, 1055-1056, 1337-1340, 1390-1416, 1438-1438, 1455-1463, 1867-1872, 1958-1959, 1977-1980, 2101-2105, 2190-2191, 2279-2292
…geometry Nine findings, all in one push this time rather than one push each. REAL, and the same shape as the others: a plane LOOKUP had an HDR side effect. find_primary_plane ended by calling read_hdr_metadata_direct, so the new scanout-width query -- a pure geometry question -- silently rewrote ctx->cur_hdr_eotf/cur_hdr_max_nits. On the helper path those values arrive over the wire, and only the order of two assignments kept them from being replaced by a direct read an unprivileged process cannot even make: tone-mapping would have turned itself off on an HDR display, invisibly, after any reordering. The read is no longer a side effect of looking up a plane; the two paths that want it call read_hdr_for_current_crtc explicitly, at the same points as before, so behaviour is unchanged. REAL, performance: the scanout-width decision was recomputed from scratch on every grab of a padded scanout -- a plane-resource sweep plus one drmModeGetProperty per property -- with only the log line latched. Memoized on (crtc_id, hdisplay, fb_width, modifier). hdisplay is in the key deliberately: a modeset that kept an identical framebuffer width and modifier would otherwise be served the stale answer. The cheap drmModeGetCrtc gate still runs per grab, since it is the ioctl this code always did and it supplies hdisplay. REAL, latent: the fast-path slot cached the SCANNED-OUT width and then handed it to virtio_transfer_from_host on both cache-hit branches. A transfer box describes the buffer being made coherent, not the visible sub-region, so a padded virtio scanout would transfer fewer columns than the mapping later exposes. The slot now keeps fb_width beside width and the transfers use it. The two non-cached call sites already used fb2->width. Also: drmtap_ensure_out failures are now named at every call site (the reduction and the EGL readback returned a bare errno, leaving drmtap_error holding an older message -- for the caller-buffer refusal ensure_out already names it, so only the allocation failures needed it); validate_fb_dims takes the drmtap_ prefix every other internal cross-module symbol carries; a 120-column comment overrun; the CHANGELOG wraps its arithmetic in code spans (markdownlint MD037); and the two integer-to-pointer sentinels in test_outbuf are real addresses now, with the refusal path additionally asserting that *out is left untouched. One rationale corrected rather than accepted: the review said the unprefixed symbol "pollutes the shared library export table". It does not -- libdrmtap.map keeps every internal symbol local, and the export count is unchanged at 24. The rename is for consistency with drmtap_ensure_buf and friends, not to fix a leak. Verified: 10/10 tests, 10/10 under ASan and UBSan, csrc in sync, 24 exported symbols, both Rust crates build. On hardware after the batch -- Mac padded scanout still narrows through the plane rect (60x2170 stride=256, 475038 of 520800 non-black) and holds over 500 grab_mapped plus 300 grab_mapped_fast frames with zero drift, now through the memoized path; Sigma tiled scanout unchanged, output buffer still refuses one byte short with contents intact and guard bytes untouched, clean under ASan on the live EGL path.
1 participant
Two changes that both came out of testing on an Apple T2 MacBook Pro, plus the security work the second one made necessary.
1. a grab reported the framebuffer width, not the scanned-out width
A driver may allocate the scanout framebuffer wider than the mode to satisfy a pitch alignment. Measured on that machine,
appletbdrmdrives the Touch Bar strip as a 60x2170 mode out of a 64x2170 framebuffer with a 256-byte pitch, and those four columns are padding that is never scanned out:Reporting the framebuffer width hands the caller an image wider than the display it asked to capture. A consumer whose display list carries the CRTC mode reads that as a display that never matches its own geometry, rejects every frame and falls back. Reported geometry is now narrowed to the mode.
It only ever shrinks, and only the width:
drmtap_dmabuf_deschas a frozen layout with nowhere to carry a crop origin, so such a frame is reported whole and the reason is logged once. No hardware here has that layout, so it is deliberately not guessed at.Buffer-size arithmetic is unchanged: the mapping is still the whole padded framebuffer, only the reported width narrows and the caller reads rows at the unchanged stride.
2. drmtap_set_output_buffer
Closes the second half of #36. A converted frame landed in a library-owned buffer, so a consumer that has to end up with the pixels in specific memory paid a full-frame memcpy per frame: 14.7 MB at 2560x1440, every frame, for nothing. Nothing required it. The EGL detile ends in a
glReadPixelsand the CPU converters take a destination pointer, and both write wherever they are pointed.Every producer of converted pixels now resolves its destination through one
drmtap_ensure_out(), so the EGL detile, the CPU deswizzle, the 10/16-bit and HDR reductions and the padded-linear repack all behave the same. A caller must not have to know which path ran to know where the pixels went. It works on an unprivilegeddrmtap_open_render()context too, so the converting half of a split can write straight into the consumer buffer.It does NOT apply where there is nothing to materialize: a linear 8-bit scanout is handed back as a direct pointer into the mapped scanout, with no intermediate buffer to redirect. The header says so rather than implying a promise it cannot keep.
security
The pointer and the length come from outside the library, so the bound is the whole story.
lenis refused with-ENOSPC, the buffer is left untouched, anddrmtap_error()names the required byte count. A short frame would be indistinguishable from a good one.validate_fb_sizeboundsstride * heightand never constrainedwidth, yet every converted output is sizedwidth * height * 4- a product that can wrapsize_tand give a large write a small destination. Reachable with a width from the helper wire or an IPC-supplied descriptor. It already existed against the library own buffer; this feature would have turned it into an overflow of a caller buffer. Newvalidate_fb_dims()bounds each dimension before anything multiplies it, so the product cannot wrap even wheresize_tis 32-bit, then bounds the pixel count. Wired into all four entry points.drmtap_frame_releasewas audited: it freespriv->mapped, neverframe->data, andpriv->mappedis only ever the mmap orMAP_FAILED. Caller memory is never freed. No change needed.Additive ABI only: 24 exported symbols, up from 23, and the only addition is the public function.
validate_fb_dims,drmtap_ensure_outanddrmtap_scanout_width_ofstay local through the version script.verified, and where
Here:
tests/test_scanout.c(6 cases) andtests/test_outbuf.c(9 cases). Both decisions are pure functions on purpose: the padding case needs one laptop in the world to observe, and a rule that easy to get subtly wrong should not be checkable only there.csrc/is back in sync. That drift mattered:libdrmtap-syscompiles those copies, so a release would have shipped a crate with neither change while meson had both.On hardware:
FB2: 64x2170in,60x2170 stride=256out, 475038 of 520800 bytes non-black, so real pixels read at the unchanged stride. The amdgpu panel is untouched at 2880x1800. Repeated after a reboot onto a different kernel, so the padding is not a quirk of one kernel.XR30scanout on i915 (a real conversion): a buffer one byte short gives-ENOSPCwith its contents intact, a correctly sized buffer receives the frame with 4 KB guard bytes either side untouched, and clearing goes back to the library buffer. Clean under ASan and UBSan on that same path.Not verified: the consumer in #36, whose validation belongs to whoever runs it, and the offset-viewport case, for which no hardware here qualifies.