9846f288b4
The Mesa Redox winsys CS submit path at src/gallium/winsys/redox/drm/redox_drm_cs.c:156 faked the seqno with 'result.seqno = rws->cs->last_seqno + 1 : 1;' instead of reading the kernel-assigned seqno from the ioctl response. This is a correctness bug under multi-process GPU use (the normal case for any compositor + GPU client setup). The kernel's seqno is global per device, but each Mesa process had its own local counter. When process A submits batch #1 (kernel seqno 100) and process B submits batch #2 (kernel seqno 101), process A's local counter diverges from the kernel's actual seqno. Fence waits keyed on the local counter would never complete when waiting for seqnos in the kernel's namespace. Fix (three coordinated changes): ### 1. redox-drm kernel side (local/recipes/gpu/redox-drm/) Following the standard DRM bidirectional-ioctl pattern that DrmAmdgpuCsWire already uses: a) driver.rs: - Added Default derive to RedoxPrivateCsSubmit and RedoxPrivateCsWait structs (needed for ..Default::default() at construction sites). - Added response field 'seqno: u64' to RedoxPrivateCsSubmit (bidirectional: input fields src..byte_count, output seqno). - Added response fields to RedoxPrivateCsWait: completed(u8), _pad([u8;7]), completed_seqno(u64). - Updated size tests: Submit 32->40 bytes, Wait 16->32 bytes. - Added doc comments noting the bidirectional pattern and the kernel-Writes-Response contract. b) scheme.rs: - CS_SUBMIT handler writes resp.seqno back into req.seqno and serializes req instead of serializing the separate resp (bytes_of(&resp) -> bytes_of(&req)). This is the kernel returning the response in the same struct. - CS_WAIT handler similarly copies result fields into req. - req made mutable for in-place mutation before serialization. - All other places that construct these structs use ..Default::default() for the new response fields. c) drivers/amd/mod.rs, intel/mod.rs, virtio/mod.rs: - Each cs_submit and cs_wait construction site now uses ..Default::default() for the new response fields. No logic changes (the drivers return RedoxPrivateCsSubmitResult / RedoxPrivateCsWaitResult from the trait method; scheme.rs copies the response into the bidirectional struct). ### 2. Mesa winsys source (local/recipes/libs/mesa/source/) Merge the separate input/result wire structs into bidirectional structs so the kernel's response is read back from the same struct the caller passed to drmIoctl: a) redox_drm_cs.c: - Merged RedoxCsSubmitWire and RedoxCsSubmitResultWire into one struct (RedoxCsSubmitWire now has the seqno output field). - Merged RedoxCsWaitWire and RedoxCsWaitResultWire into one struct (RedoxCsWaitWire now has completed + completed_seqno fields). - Removed 'result.seqno = rws->cs->last_seqno + 1 : 1;' fake. Instead, reads 'submit.seqno' and 'wait.completed_seqno' from the same struct after drmIoctl returns. - Updated file-header comment to document the bidirectional pattern, kernel ABI, and the multi-process correctness consequence. b) Patches the durability: - Added 'mesa/26-cs-submit-bidirectional-seqno.patch' to the patches list in local/recipes/libs/mesa/recipe.toml. - The patch persists the C-side merge across clean re-extracts of the upstream Mesa 26.1.4 tarball. Note (operator runtime gate): - The kernel ABI change requires that the ioctl bytes ARE read back into the same user buffer on Redox schemes. This is the standard pattern for all other DRM ioctls in redox-drm's scheme.rs (DrmGemCreateWire, DrmAmdgpuCsWire, DrmCreateDumbWire etc.). Verification of the runtime fix requires multi-process GPU testing on real hardware — operator-side gate. - Per AGENTS.md NO-FALLBACK policy: this fixes a real correctness bug. The pre-fix 'fake seqno' code was admitted in the original file via a '// TODO' comment with the requirement to integrate with the actual scheme:drm protocol - now done. Files changed: - local/recipes/gpu/redox-drm/source/src/driver.rs - local/recipes/gpu/redox-drm/source/src/scheme.rs - local/recipes/gpu/redox-drm/source/src/drivers/amd/mod.rs - local/recipes/gpu/redox-drm/source/src/drivers/intel/mod.rs - local/recipes/gpu/redox-drm/source/src/drivers/virtio/mod.rs - local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_cs.c - local/recipes/libs/mesa/recipe.toml - local/patches/mesa/26-cs-submit-bidirectional-seqno.patch
136 lines
4.8 KiB
Diff
136 lines
4.8 KiB
Diff
--- a/src/gallium/winsys/redox/drm/redox_drm_cs.c
|
|
+++ b/src/gallium/winsys/redox/drm/redox_drm_cs.c
|
|
@@ -5,14 +5,17 @@
|
|
* local/recipes/gpu/redox-drm/source/src/scheme.rs
|
|
* local/recipes/gpu/redox-drm/source/src/driver.rs
|
|
*
|
|
- * Two ioctls are used:
|
|
+ * Two ioctls are used, both following the standard DRM bidirectional-ioctl
|
|
+ * pattern: the caller fills the input fields, passes the struct to drmIoctl,
|
|
+ * and the kernel writes the response fields back into the SAME struct.
|
|
+ *
|
|
* - REDOX_PRIVATE_CS_SUBMIT (DRM_IOCTL_BASE + 31)
|
|
- * in: src_handle (gem), dst_handle (gem, ignored for now),
|
|
- * src_offset (offset into src buffer), byte_count (DWORDs to submit)
|
|
+ * in: src_handle (gem), dst_handle (gem, ignored for now),
|
|
+ * src_offset, dst_offset, byte_count
|
|
* out: seqno (u64) — the kernel-assigned submission id
|
|
* - REDOX_PRIVATE_CS_WAIT (DRM_IOCTL_BASE + 32)
|
|
- * in: seqno, timeout_ns
|
|
- * out: completed (bool), completed_seqno (u64)
|
|
+ * in: seqno, timeout_ns
|
|
+ * out: completed (u8), completed_seqno (u64)
|
|
*
|
|
* The kernel reads `byte_count` u32 dwords from the mapped GEM at
|
|
* `src_handle + src_offset` and writes them into the GPU render ring.
|
|
@@ -20,6 +23,12 @@
|
|
* We invoke these ioctls via `drmIoctl` on the winsys fd. libdrm's
|
|
* redox patch redirects the ioctls to /scheme:drm/card0.
|
|
*
|
|
+ * The seqno returned by SUBMIT is the kernel's global per-device counter.
|
|
+ * It must NOT be faked with a local counter: under multi-process GPU use
|
|
+ * (compositor + multiple clients sharing /scheme/drm/card0) a local counter
|
|
+ * diverges from the kernel's namespace and fence waits keyed on it never
|
|
+ * complete.
|
|
+ *
|
|
* SPDX-License-Identifier: MIT
|
|
*/
|
|
|
|
@@ -51,7 +60,12 @@
|
|
#include "util/u_debug.h"
|
|
|
|
/* Wire format of the kernel ABI structs. MUST match the kernel definitions
|
|
- * (kernel's GemHandle = u32; the structs are #[repr(C)] in Rust).
|
|
+ * in local/recipes/gpu/redox-drm/source/src/driver.rs (#[repr(C)]).
|
|
+ *
|
|
+ * Both structs are bidirectional: the caller fills the input fields, and the
|
|
+ * kernel writes the response fields back into the same struct on return.
|
|
+ * There is no separate result struct on the wire — the kernel serializes the
|
|
+ * same struct (with response fields populated) back to userspace.
|
|
*/
|
|
struct RedoxCsSubmitWire {
|
|
uint32_t src_handle;
|
|
@@ -59,20 +73,15 @@
|
|
uint64_t src_offset;
|
|
uint64_t dst_offset;
|
|
uint64_t byte_count;
|
|
-};
|
|
-
|
|
-struct RedoxCsSubmitResultWire {
|
|
- uint64_t seqno;
|
|
+ uint64_t seqno; /* response: kernel-assigned submission id */
|
|
};
|
|
|
|
struct RedoxCsWaitWire {
|
|
uint64_t seqno;
|
|
uint64_t timeout_ns;
|
|
-};
|
|
-
|
|
-struct RedoxCsWaitResultWire {
|
|
- uint8_t completed;
|
|
- uint64_t completed_seqno;
|
|
+ uint8_t completed; /* response: non-zero if seqno completed */
|
|
+ uint8_t _pad[7]; /* align completed_seqno to 8 bytes */
|
|
+ uint64_t completed_seqno; /* response: highest completed seqno */
|
|
};
|
|
|
|
uint64_t
|
|
@@ -81,7 +90,6 @@
|
|
uint32_t batch_dword_count)
|
|
{
|
|
struct RedoxCsSubmitWire submit = {0};
|
|
- struct RedoxCsSubmitResultWire result = {0};
|
|
size_t byte_count;
|
|
void *mapped;
|
|
int err;
|
|
@@ -143,30 +151,23 @@
|
|
return 0;
|
|
}
|
|
|
|
- /* The ioctl is the kernel-side drmIoctl wrapper; the actual submit
|
|
- * returns the seqno through a separate read (DRM protocol on Redox
|
|
- * uses a write-then-read pattern). For now, this uses a placeholder
|
|
- * ioctl that returns the result synchronously.
|
|
- *
|
|
- * TODO: integrate with the actual scheme:drm request/response
|
|
- * protocol by calling drmCommandWriteRead or by using a dedicated
|
|
- * 'ioctl with response' helper once the recipe-side IOCTL
|
|
- * interface supports that mode.
|
|
+ /* The kernel writes the assigned seqno back into submit.seqno
|
|
+ * (bidirectional ioctl — no separate response struct on the wire).
|
|
+ * This is the kernel's global per-device counter, correct under
|
|
+ * multi-process GPU use where a local counter would diverge.
|
|
*/
|
|
- result.seqno = rws->cs ? rws->cs->last_seqno + 1 : 1;
|
|
|
|
/* Drop the batch BO; the kernel has copied from it into the ring. */
|
|
redox_drm_bo_unmap(rws, batch_pr);
|
|
redox_drm_bo_destroy(rws, batch_pr);
|
|
|
|
- return result.seqno;
|
|
+ return submit.seqno;
|
|
}
|
|
|
|
uint64_t
|
|
redox_drm_cs_query_last_seqno(struct redox_drm_winsys *rws)
|
|
{
|
|
struct RedoxCsWaitWire wait = {0};
|
|
- struct RedoxCsWaitResultWire result = {0};
|
|
int err;
|
|
|
|
if (!rws)
|
|
@@ -184,10 +185,10 @@
|
|
return rws->cs ? rws->cs->last_seqno : 0;
|
|
}
|
|
|
|
- /* Same write-then-read caveat as submit; for now the placeholder
|
|
- * ioctl fills in result synchronously.
|
|
+ /* The kernel writes completed_seqno back into the wait struct
|
|
+ * (bidirectional ioctl — same struct for request and response).
|
|
*/
|
|
- return result.completed_seqno;
|
|
+ return wait.completed_seqno;
|
|
}
|
|
|
|
struct redox_drm_cs *
|