fix: declare the cursor hotspot cap, or a virtualized driver hides its cursor plane (0.5.4) - #50
Conversation
fufesou reported "the cursor does not show" on three VMs, at a greeter and in a session alike, while two physical machines were fine. Reproduced on virtio-gpu and measured end to end. `drmtap_open` asks for `DRM_CLIENT_CAP_ATOMIC` to read a connector's `CRTC_ID`. Since kernel 6.6 a para-virtualized driver (virtio-gpu, vmwgfx, qxl, vboxvideo) answers that cap by hiding its cursor plane from the client, unless the client also sets `DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT` to say it honors the hotspot properties. `find_cursor_plane` therefore found nothing, `drmtap_get_cursor` reported the cursor hidden on every call, and the consumer hid it for the whole session. Measured on virtio-gpu with a standalone probe: `drmModeGetPlaneResources` returns two planes with no atomic cap, one with it, two again with this one. In the live service: 811 of 811 cursor reads found no plane before, and with the cap the remote cursor is drawn again -- the client shows the host's arrow where it showed nothing. On i915 the cap is refused with `EOPNOTSUPP`, as documented, and the plane count is twenty-four either way. Cursor reads already read `HOTSPOT_X` / `HOTSPOT_Y`, which is what the cap asserts, so there is nothing else to honor. The helper binary never had the bug: it does not ask for the atomic cap, so only an in-process privileged caller was affected.
Version stamp, changelog, and the documentation the bug proved wrong. - README known-limitations already explained that only virtualized drivers export the cursor hotspot properties; it now says what those properties cost, which is that the driver hides the whole plane from an atomic client that has not declared it honors them. - The `drmtap_get_cursor` header said it returns `-ENOENT` when there is no cursor plane. It never did: a hidden cursor clears the plane binding, and reporting that as an error would make a consumer keep painting a stale cursor, so it answers success with `visible = 0`. Same class as the `frame->data` header bug in #36. - docs/research/02 listed the universal-planes cap as all a cursor read needs. - `drmtap-report.sh` reports the plane count and how many are bound, so the next cursor report from a tester answers this without a round trip.
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
📝 WalkthroughSummary by CodeRabbit
Walkthroughlibdrmtap 0.5.4 enables ChangesCursor plane support
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant libdrmtap
participant DRMDriver
Client->>libdrmtap: Open DRM context
libdrmtap->>DRMDriver: Enable atomic capability
libdrmtap->>DRMDriver: Enable cursor-plane hotspot capability
DRMDriver-->>libdrmtap: Accept or return EOPNOTSUPP
libdrmtap-->>Client: Continue context initialization
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 `@docs/research/02_drm_kms_mechanism.md`:
- Around line 166-173: Update the cursor-plane paragraph in
02_drm_kms_mechanism.md to include the observation date and turn
DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT into a link to the kernel UAPI documentation
or another upstream source. Preserve any existing file-wide date while ensuring
this research observation has an explicit date and source.
In `@include/drmtap.h`:
- Around line 490-496: Update the cursor regression test in test_capture.c
around drmtap_get_cursor to reject -ENOENT and require a successful return; when
no cursor plane is bound, assert the documented hidden-cursor result with
visible = 0 instead of skipping the test.
In `@tools/drmtap-report.sh`:
- Around line 82-83: Update the bound-plane count in the report logic near the
planes and bound assignments to parse each plane’s crtc= field and exclude only
crtc=(null). Remove the name-prefix matching for “crtc” or “pipe” so
non-prefixed CRTC names are counted correctly.
🪄 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: 6da13786-73a9-4288-8d8d-05551b8ffe2d
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap/Cargo.tomldocs/research/02_drm_kms_mechanism.mdinclude/drmtap.hmeson.buildsrc/drmtap.ctools/drmtap-report.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Static Analysis
- GitHub Check: Build & Test (Ubuntu 22.04)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (6)
meson.build
📄 CodeRabbit inference engine (AGENTS.md)
The project must compile with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
meson.build
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep the version synchronized across the C library, Meson,
libdrmtap-sys, andlibdrmtap; the canonical version is ininclude/drmtap.h, verified bytools/check-version.sh.
Files:
meson.buildbindings/rust/libdrmtap-sys/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drmtap.hREADME.mdCHANGELOG.mdtools/drmtap-report.shsrc/drmtap.cbindings/rust/libdrmtap/Cargo.tomlbindings/rust/libdrmtap-sys/csrc/drmtap.cinclude/drmtap.hdocs/research/02_drm_kms_mechanism.md
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style: 4-space indentation with no tabs, snake_case names, same-line braces (1TBS), and//or/* */comments as appropriate.
Keep lines within a 100-character soft limit and 120-character hard limit.
Every C source and header file must begin with the specified libdrmtap copyright, license,@file, and@briefheader block.
Files:
bindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/drmtap.cbindings/rust/libdrmtap-sys/csrc/drmtap.cinclude/drmtap.h
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Prefix public C API functions withdrmtap_; usesnake_casefor variables,UPPER_SNAKE_CASEwithDRMTAP_prefixes for macros/constants, and appropriate internal/public type naming.
Organize implementation files as grouped includes, private types/constants, static functions, then public functions in header declaration order.
Return negative errno values on failure, never abort, 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 resources during cleanup.
Files:
src/drmtap.c
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
include/drmtap.h: UseDRMTAP_MODULE_H-style header guards and document every public API function with Doxygen comments including parameters and return values.
Update public API comments whenever the public API changes.
Files:
include/drmtap.h
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Write documentation in English; research documents must include dates and sources, and DRM API references must link to kernel documentation or upstream sources.
Files:
docs/research/02_drm_kms_mechanism.md
🪛 LanguageTool
CHANGELOG.md
[style] ~17-~17: Consider using “who” when you are referring to a person instead of an object.
Context: ...or hidden on every call, and a consumer that trusts that answer showed no cursor any...
(THAT_WHO)
[style] ~21-~21: Consider an alternative for the overused word “exactly”.
Context: ...nor HOTSPOT_X / HOTSPOT_Y, which is exactly what the new cap asserts, so nothing el...
(EXACTLY_PRECISELY)
🔇 Additional comments (12)
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
34-34: LGTM!Also applies to: 490-496
include/drmtap.h (1)
34-34: LGTM!meson.build (1)
4-4: 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
bindings/rust/libdrmtap-sys/csrc/drmtap.c (1)
35-39: LGTM!src/drmtap.c (2)
35-39: LGTM!
373-383: 🎯 Functional CorrectnessUse kernel-aware wording for capability rejection.
The compatibility fallback allows builds with pre-6.6 headers, but a pre-6.6 kernel can also reject capability
6. Both implementations currently treat every refusal as evidence of a non-virtualized driver. The upstream UAPI guarantees this capability for atomic-capable virtualized drivers only starting with kernel 6.6. (raw.githubusercontent.com)
src/drmtap.c#L373-L383: change the comment and log to say “unsupported by driver or kernel,” unless the supported kernel floor is confirmed to be 6.6 or newer.bindings/rust/libdrmtap-sys/csrc/drmtap.c#L373-L383: apply the same kernel-aware wording.Confirm the project's supported kernel floor before merge.
Source: MCP tools
README.md (1)
378-378: LGTM!tools/drmtap-report.sh (2)
75-81: LGTM!Also applies to: 84-86
82-83: 🩺 Stability & AvailabilityNo change needed.
grep -cstill returns0as the count whengrep -uo pipefailis active and the command is not used as the final command in a pipeline. The concern only applies withset -e.> Likely an incorrect or invalid review comment.CHANGELOG.md (1)
8-28: LGTM!Also applies to: 660-660
1- the cursor test accepted `-ENOENT` and skipped, so it would have passed against a return to the contract this PR corrects. It now asserts that value never comes back. 2- the research note had no date and no upstream source: it quotes the cap's kerneldoc in `include/uapi/drm/drm.h` and dates both measurements, on virtio-gpu and on i915. 3- the report counted a bound plane by matching CRTC names we happen to have seen (`crtc-`, `pipe `). Anything other than `(null)` is bound.
1 participant
Closes #49.
A para-virtualized driver (virtio-gpu, vmwgfx, qxl, vboxvideo) hides its cursor plane from a client
that enabled
DRM_CLIENT_CAP_ATOMICand did not also enableDRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.We enable the atomic cap to read a connector's
CRTC_ID, so since then the cursor read has found noplane on those drivers and reported the cursor hidden on every call. The full measurement chain is in
the issue.
The fix is one
drmSetClientCapafter the atomic one, with an#ifndefso a build againstpre-6.6 headers still declares it. Cursor reads already honor
HOTSPOT_X/HOTSPOT_Y, which isexactly what the cap asserts.
Also in here, all of it documentation the bug proved wrong:
1- the README known-limitation entry explained that only virtualized drivers export the hotspot
properties, without saying what asking for them costs.
2- the
drmtap_get_cursorheader claimed-ENOENTwhen there is no cursor plane. It never returnedthat: a hidden cursor clears the plane binding, and an error there would make a consumer keep
painting a stale cursor, so it answers success with
visible = 0. Same class as theframe->dataheader bug in #36.
3- docs/research/02 listed the universal-planes cap as all a cursor read needs.
4-
tools/drmtap-report.shnow reports the plane count and how many are bound, so the next cursorreport from a tester answers this without a round trip.
Verified: 11/11 tests,
check-version.shcoherent at 0.5.4, soname stilllibdrmtap.so.0, and on areal virtio-gpu VM the remote cursor is drawn again where 0.5.2 showed none.