G8: a re-import or second handle no longer detaches a buffer (GEM hooks once per file and object) #11

Merged

toasterson merged 3 commits from g8-reimport into main 2026-10-05 16:47:04 +00:00

Conversation 0 Commits 3 Files changed 11 +2016 -185

toasterson commented 2026-10-03 18:58:26 +00:00

Owner

Copy link

Anima work item: G8 01a102ed-3645-7ce3-a093-c9d22212d5d1

Stacked on #9 (r5a-facade @ 2306e85). Importing or opening an object that a file already holds no longer detaches it from the GPU.

Root cause

GemCore::handle_find_or_create (the PRIME_FD_TO_HANDLE path) called the driver's handle_opened hook before looking for the file's handle. When it found one, it undid the hook with handle_closed (gem.rs at 2306e85, lines 1838–1863). On virtio_gpu these hooks are CTX_ATTACH_RESOURCE and CTX_DETACH_RESOURCE, and the host does not count attachments. So re-importing a buffer the file already held detached the resource from that file's virgl context while its existing handle was still using it. That is G3's DTrace finding (attach then detach for the same context) and G4's black screen: after Xorg's DRI3 PixmapFromBuffer of its own front buffer, glamor's context lost the buffer ("Illegal resource").

Linux's per-handle callbacks would have the same effect in one more case: a second handle from GEM_OPEN or MODE_GETFB, closed again (Linux's virtio-gpu detaches on every close).

Fix

• drm_core::objects::FileTable / Locked. A file's handle table now calls the driver's open hook when the file gets its first handle to an object, and the close hook when its last handle goes. A per-file hook lock is held across the decision, the hook and the table change; it plays the role of Linux's per-file PRIME lock. Lock order: hook lock, then names, then the file's handle table.
  • A handle creation that fails undoes only an open hook that this same call ran.
  • The table is scanned only when the object has handles somewhere else, so creating and closing a file's own buffers costs no scan.
• GemCore::import_handle replaces handle_find_or_create. It looks for the file's handle under the hook lock first and runs no hook if it finds one. This covers same-device buffers and another driver's buffers (R5a). For another driver's buffer, the import (gem_prime_import) now also happens under the lock, so two threads of one file importing the same buffer get one import (before, this was a documented race).
• GEM_OPEN, MODE_GETFB and drm_gem_handle_create() still create a second handle, but run no hook for it, so closing that handle leaves the attachment alone.
  • Deviation from the item text, deliberate: the item says Linux's GEM_OPEN returns the existing handle. It does not. drm_gem_open_ioctl goes through drm_gem_handle_create_tail, which always allocates a new handle.
  • R1's test checks for the second handle (drm_r1_test.c:687).
  • Userland that opens a name, reads the size and closes the handle would otherwise close its own buffer.
• Facade contract. funcs->open and funcs->close (and drm-facade's GemObject::open/close) now run once per file and object; this is documented in include/drm/drm_gem.h and in the README as an R5a deviation. Drivers that count their calls, as Linux's amdgpu, radeon, nouveau and cdrmnull do, see the same balance.
• Undo-path audit. Every close hook now goes through FileHooks::closed, called by Locked::add's undo (only after its own open) or Locked::released (only once the file holds no handle to the object). The GETFB2 error path closes only handles that the same call created. handle_find_or_create is gone.
• kstat. The gem kstat gains attached (file and object pairs currently open) and opens (open-hook calls so far).
• Docs. README: the decision Driver hooks per file and object (G8), the R2, R4 and R5a paragraphs, and the G8 results.

Tests

• Host (crates/drm-core/src/objects.rs):
  • reimport_and_second_handles_keep_the_attachment
  • failed_creation_undoes_only_its_own_open
  • concurrent_hooks_follow_the_handles: 8 threads and 4 files. The threads import shared buffers (twice in a row, as DRI3 does), open them by name, create and close handles, with hooks that yield and assert that open and close alternate. A checker holds every hook lock and verifies that a file has an object open exactly while it holds a handle to it.
  • Mutation runs: with the old open-then-undo behaviour all three tests fail; without the hook lock the concurrent test fails.
  • cargo test -p drm-core -p drm-uapi -p drm-facade: 127 + 2 + 1 pass. virtio_gpu --no-default-features: 6 pass. clippy -D warnings, fmt and rustdoc -D warnings are clean.
• Kernel (tools/test/drm_g8_test.c, run-g8-test.sh kernel|reproducer|cleanup). On cdrmnull, rdrmnull and virtio_gpu:
  • A file imports its own dumb buffer twice: same handle, and handles, attached, objects, umem_bytes, dma_segments and opens stay the same; the mapping keeps its contents.
  • A second file imports it twice: +1 handle, +1 attachment, +1 open.
  • GEM_OPEN and MODE_GETFB give a second handle with no attachment or open; closing it leaves the attachment.
  • Every other driver imports it twice: one handle and one import object.
  • The DRI3 case on virtio_gpu with virgl, on a 1024x768 3D texture: the server file and a client file each import it twice. Then, through the server's first handle:
    • a virgl clear reads back with TRANSFER_FROM_HOST;
    • a pattern written to the mapping and sent with TRANSFER_TO_HOST reads back;
    • ADDFB2 + SETCRTC + DIRTYFB (RESOURCE_FLUSH) shows it, and a screendump confirms it.
  • The client then draws into the texture through its own context and closes its handle; the server's context keeps the texture.
• G3's two-process reproducer is added as tools/test/dmabuf_import_test.c, unchanged apart from a provenance header and DRM_RENDER.

Evidence (hephaistos, guest g8-run1, port 2440, fresh overlay of golden-20260930.qcow2, QEMU 6.2 virtio-gpu-gl-pci + egl-headless, virglrenderer 0.9.1, 2 vCPUs, 3 GiB; logs in the workspace under logs/hephaistos/illumos-drm/20261003-g8-run1-*)

Run	Kernel	Result
drm_g8_test, archives of base 2306e85	7e802e88a3	90 passed, 53 failed. After the double import the server's clear and transfers read back the old contents (786432 of 786432 pixels). The display stayed red (screendump). QEMU: Illegal resource 2, Illegal command buffer. Missing attached/opens kstats account for the other failures. (-baseline2-2306e85.log)
drm_g8_test at c2ea15b	7e802e88a3	143 passed, 0 failed. Azure display (screendump), no QEMU log lines, /etc and /devices unchanged after unload (-fix-c2ea15b.log; first round -fix-f614b7a.log was also 143/0)
run-r5a-test.sh at c2ea15b	7e802e88a3	PASS, 167 checks, 0 FAIL. Includes R0, R1 and R2 on both null drivers (R2 200/200 each, 32-bit probes), R3 on rdrmnull, R4 63/63 (four runs), R5a 81/81 and three reattach orders, G6 3×27 + getsundev 73, run-r1/r3/r4/g6, and four cdrmnull unregistration races. virtio_gpu detach: 19522 requests, 0 failed, 2011 resources, 0 left (-r5a-suite-c2ea15b.log)
drm_g8_test at c2ea15b after pkg update to osnet 23298	2cafbda106	143 passed, 0 failed (-23298-fix-kernel.log)
dmabuf_import_test, G3 Mesa 25.3.6 (p5p sha256 51e7de0e…6a), base archives	2cafbda106	twice FAIL, once PASS, draw-first FAIL: the importer's red is lost and QEMU logs Illegal resource (-23298-old-reproducer.log)
dmabuf_import_test at c2ea15b	2cafbda106	twice PASS (12 checks), once PASS (10), draw-first PASS (13), no QEMU errors (-23298-fix-reproducer.log)

Screendumps are in the workspace under artifacts/g8-reimport/. G4's dri3_probe against a glamor Xorg could not be run: OI's xorg-server 1.19.7 glamor requires EGL_MESA_drm_image, which Mesa removed in 25.2. The two-process reproducer, and the kernel test's DRI3 sequence, are the end-to-end evidence. No panic. The guest was shut down and its overlay deleted.

Review

One review round under the Anima rules (round 1: approve, non-blocking findings), all addressed in c2ea15b:

• references are dropped after the hook lock is released;
• the table is not scanned for objects without other handles;
• the opens counter, so the null-driver checks catch an open-then-close re-import;
• README and cdrmnull comments;
• SET_MASTER reporting, and a reproducer guard in the script.

Anima work item: G8 `01a102ed-3645-7ce3-a093-c9d22212d5d1` Stacked on #9 (`r5a-facade` @ `2306e85`). Importing or opening an object that a file already holds no longer detaches it from the GPU. ## Root cause `GemCore::handle_find_or_create` (the `PRIME_FD_TO_HANDLE` path) called the driver's `handle_opened` hook before looking for the file's handle. When it found one, it undid the hook with `handle_closed` (`gem.rs` at `2306e85`, lines 1838–1863). On virtio_gpu these hooks are `CTX_ATTACH_RESOURCE` and `CTX_DETACH_RESOURCE`, and the host does not count attachments. So re-importing a buffer the file already held detached the resource from that file's virgl context while its existing handle was still using it. That is G3's DTrace finding (attach then detach for the same context) and G4's black screen: after Xorg's DRI3 PixmapFromBuffer of its own front buffer, glamor's context lost the buffer ("Illegal resource"). Linux's per-handle callbacks would have the same effect in one more case: a second handle from `GEM_OPEN` or `MODE_GETFB`, closed again (Linux's virtio-gpu detaches on every close). ## Fix - **`drm_core::objects::FileTable` / `Locked`.** A file's handle table now calls the driver's open hook when the file gets its first handle to an object, and the close hook when its last handle goes. A per-file hook lock is held across the decision, the hook and the table change; it plays the role of Linux's per-file PRIME lock. Lock order: hook lock, then `names`, then the file's handle table. - A handle creation that fails undoes only an open hook that this same call ran. - The table is scanned only when the object has handles somewhere else, so creating and closing a file's own buffers costs no scan. - **`GemCore::import_handle`** replaces `handle_find_or_create`. It looks for the file's handle under the hook lock first and runs no hook if it finds one. This covers same-device buffers and another driver's buffers (R5a). For another driver's buffer, the import (`gem_prime_import`) now also happens under the lock, so two threads of one file importing the same buffer get one import (before, this was a documented race). - **`GEM_OPEN`, `MODE_GETFB` and `drm_gem_handle_create()` still create a second handle,** but run no hook for it, so closing that handle leaves the attachment alone. - Deviation from the item text, deliberate: the item says Linux's `GEM_OPEN` returns the existing handle. It does not. `drm_gem_open_ioctl` goes through `drm_gem_handle_create_tail`, which always allocates a new handle. - R1's test checks for the second handle (`drm_r1_test.c:687`). - Userland that opens a name, reads the size and closes the handle would otherwise close its own buffer. - **Facade contract.** `funcs->open` and `funcs->close` (and drm-facade's `GemObject::open`/`close`) now run once per file and object; this is documented in `include/drm/drm_gem.h` and in the README as an R5a deviation. Drivers that count their calls, as Linux's amdgpu, radeon, nouveau and cdrmnull do, see the same balance. - **Undo-path audit.** Every close hook now goes through `FileHooks::closed`, called by `Locked::add`'s undo (only after its own open) or `Locked::released` (only once the file holds no handle to the object). The GETFB2 error path closes only handles that the same call created. `handle_find_or_create` is gone. - **kstat.** The `gem` kstat gains `attached` (file and object pairs currently open) and `opens` (open-hook calls so far). - **Docs.** README: the decision *Driver hooks per file and object (G8)*, the R2, R4 and R5a paragraphs, and the G8 results. ## Tests - **Host** (`crates/drm-core/src/objects.rs`): - `reimport_and_second_handles_keep_the_attachment` - `failed_creation_undoes_only_its_own_open` - `concurrent_hooks_follow_the_handles`: 8 threads and 4 files. The threads import shared buffers (twice in a row, as DRI3 does), open them by name, create and close handles, with hooks that yield and assert that open and close alternate. A checker holds every hook lock and verifies that a file has an object open exactly while it holds a handle to it. - Mutation runs: with the old open-then-undo behaviour all three tests fail; without the hook lock the concurrent test fails. - `cargo test -p drm-core -p drm-uapi -p drm-facade`: 127 + 2 + 1 pass. `virtio_gpu --no-default-features`: 6 pass. clippy `-D warnings`, fmt and rustdoc `-D warnings` are clean. - **Kernel** (`tools/test/drm_g8_test.c`, `run-g8-test.sh kernel|reproducer|cleanup`). On cdrmnull, rdrmnull and virtio_gpu: - A file imports its own dumb buffer twice: same handle, and handles, attached, objects, umem_bytes, dma_segments and opens stay the same; the mapping keeps its contents. - A second file imports it twice: +1 handle, +1 attachment, +1 open. - `GEM_OPEN` and `MODE_GETFB` give a second handle with no attachment or open; closing it leaves the attachment. - Every other driver imports it twice: one handle and one import object. - The DRI3 case on virtio_gpu with virgl, on a 1024x768 3D texture: the server file and a client file each import it twice. Then, through the server's first handle: - a virgl clear reads back with `TRANSFER_FROM_HOST`; - a pattern written to the mapping and sent with `TRANSFER_TO_HOST` reads back; - `ADDFB2` + `SETCRTC` + `DIRTYFB` (`RESOURCE_FLUSH`) shows it, and a screendump confirms it. - The client then draws into the texture through its own context and closes its handle; the server's context keeps the texture. - **G3's two-process reproducer** is added as `tools/test/dmabuf_import_test.c`, unchanged apart from a provenance header and `DRM_RENDER`. ## Evidence (hephaistos, guest `g8-run1`, port 2440, fresh overlay of `golden-20260930.qcow2`, QEMU 6.2 `virtio-gpu-gl-pci` + egl-headless, virglrenderer 0.9.1, 2 vCPUs, 3 GiB; logs in the workspace under `logs/hephaistos/illumos-drm/20261003-g8-run1-*`) | Run | Kernel | Result | | --- | --- | --- | | `drm_g8_test`, archives of base `2306e85` | 7e802e88a3 | 90 passed, **53 failed**. After the double import the server's clear and transfers read back the old contents (786432 of 786432 pixels). The display stayed red (screendump). QEMU: `Illegal resource 2`, `Illegal command buffer`. Missing `attached`/`opens` kstats account for the other failures. (`-baseline2-2306e85.log`) | | `drm_g8_test` at `c2ea15b` | 7e802e88a3 | **143 passed, 0 failed**. Azure display (screendump), no QEMU log lines, `/etc` and `/devices` unchanged after unload (`-fix-c2ea15b.log`; first round `-fix-f614b7a.log` was also 143/0) | | `run-r5a-test.sh` at `c2ea15b` | 7e802e88a3 | **PASS, 167 checks, 0 FAIL**. Includes R0, R1 and R2 on both null drivers (R2 200/200 each, 32-bit probes), R3 on rdrmnull, R4 63/63 (four runs), R5a 81/81 and three reattach orders, G6 3×27 + getsundev 73, run-r1/r3/r4/g6, and four cdrmnull unregistration races. virtio_gpu detach: 19522 requests, 0 failed, 2011 resources, 0 left (`-r5a-suite-c2ea15b.log`) | | `drm_g8_test` at `c2ea15b` after `pkg update` to osnet 23298 | 2cafbda106 | 143 passed, 0 failed (`-23298-fix-kernel.log`) | | `dmabuf_import_test`, G3 Mesa 25.3.6 (p5p sha256 `51e7de0e…6a`), base archives | 2cafbda106 | twice **FAIL**, once PASS, draw-first **FAIL**: the importer's red is lost and QEMU logs `Illegal resource` (`-23298-old-reproducer.log`) | | `dmabuf_import_test` at `c2ea15b` | 2cafbda106 | **twice PASS (12 checks), once PASS (10), draw-first PASS (13)**, no QEMU errors (`-23298-fix-reproducer.log`) | Screendumps are in the workspace under `artifacts/g8-reimport/`. G4's `dri3_probe` against a glamor Xorg could not be run: OI's xorg-server 1.19.7 glamor requires `EGL_MESA_drm_image`, which Mesa removed in 25.2. The two-process reproducer, and the kernel test's DRI3 sequence, are the end-to-end evidence. No panic. The guest was shut down and its overlay deleted. ## Review One review round under the Anima rules (round 1: approve, non-blocking findings), all addressed in `c2ea15b`: - references are dropped after the hook lock is released; - the table is not scanned for objects without other handles; - the `opens` counter, so the null-driver checks catch an open-then-close re-import; - README and cdrmnull comments; - `SET_MASTER` reporting, and a reproducer guard in the script.

toasterson added 3 commits 2026-10-03 18:58:26 +00:00

drm-core: a file's GEM hooks run once per object, re-imports run none ... f614b7aa2f

Importing an object a file holds already (PRIME_FD_TO_HANDLE) ran the
driver's open hook before looking for the file's handle and, finding
one, undid the hook with the close hook. virtio_gpu's hooks attach the
resource to the file's virgl context and detach it, and the host does
not count attachments, so the undo detached the resource that the
existing handle still used: after Xorg's DRI3 PixmapFromBuffer of its
own front buffer, glamor's context lost the buffer ("Illegal resource")
and the screen stayed black (G3, G4).

The handle table of a file (objects::FileTable) now calls the driver's
open hook when the file gets its first handle to an object and its
close hook when the last one goes, with a per-file hook lock held
across the decision, the hook and the table change (Linux's per-file
PRIME lock). PRIME_FD_TO_HANDLE looks for the file's handle under that
lock first and runs no hook when it finds one, for same-device buffers
and for another driver's (whose import is now also made under the
lock, so two threads of one file get one import). GEM_OPEN and
MODE_GETFB still make a second handle, as in Linux, but run no hook for
it, and closing it leaves the attachment alone. A failed handle
creation undoes only the open hook it ran itself. The gem kstat counts
the file and object pairs whose open hook ran (attached).

Host tests: re-import and second handles keep the attachment, a failed
creation undoes only its own open, and eight threads importing (twice,
as DRI3 does), opening by name and closing in four files with yielding
hooks keep "attached exactly while held" (both fail on the old
behaviour and without the hook lock). Guest tests: tools/test/
drm_g8_test.c and run-g8-test.sh; G3's two-process reproducer as
tools/test/dmabuf_import_test.c.

Review round 1: references dropped after the hook lock, opens kstat ... c2ea15bc95

The core drops the references that come back from a failed handle
creation, and those of file_close, after the file's hook lock is
released (statement-scoped guards); the lock rule in gem.rs says that
only driver hooks and gem_prime_import may drop references under the
hook lock. Adding or removing a handle scans the file's table only when
the object has handles elsewhere, so a file's own buffers stay O(1).
The gem kstat counts the driver's open hook calls (opens), with which
drm_g8_test catches an open-then-close re-import on the null drivers
too. README: the per-file-and-object hooks as a decision and a facade
deviation; the R2 and R4 paragraphs and cdrmnull's counters follow.
drm_g8_test reports a failed SET_MASTER; run-g8-test.sh says what to
run first when the reproducer has no nodes.

README: G8 results at c2ea15b 8b90787d0f

toasterson referenced this pull request 2026-10-03 21:51:49 +00:00

R5b: public-layout base objects (facade 1.0): Linux fence semantics, GEM refcount, drmm, drm_printer, handle walk #13

toasterson changed target branch from r5a-facade to main 2026-10-05 16:47:00 +00:00

toasterson merged commit d705f095d7 into main 2026-10-05 16:47:04 +00:00

toasterson referenced this pull request from a commit 2026-10-05 16:47:05 +00:00

Merge pull request 'G8: a re-import or second handle no longer detaches a buffer (GEM hooks once per file and object)' (#11) from g8-reimport into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

illumos/illumos-drm!11

No description provided.