v5.0: Mesa CS submit seqno multi-process correctness fix

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
This commit is contained in:
kellito
2026-07-26 17:04:40 +09:00
parent 6d8ef13dc1
commit 9846f288b4
9 changed files with 225 additions and 45 deletions
@@ -0,0 +1,135 @@
--- 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 *
@@ -11,27 +11,47 @@ pub enum DriverEvent {
Hotplug { connector_id: u32 },
}
/// Bidirectional CS submit wire struct, mirroring the standard DRM
/// bidirectional-ioctl pattern (see `DrmAmdgpuCsWire` in scheme.rs).
/// The caller fills the input fields (`src_handle` … `byte_count`);
/// the kernel writes the assigned sequence number back into `seqno`
/// in the same struct — there is no separate response struct on the wire.
#[repr(C)]
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, Default)]
pub struct RedoxPrivateCsSubmit {
pub src_handle: GemHandle,
pub dst_handle: GemHandle,
pub src_offset: u64,
pub dst_offset: u64,
pub byte_count: u64,
/// Response — kernel-assigned submission sequence number.
/// Written by the kernel; callers read it after the ioctl returns.
pub seqno: u64,
}
/// Result type returned by the `GpuDriver::redox_private_cs_submit`
/// trait method. Retained as the trait return type; the wire response
/// is carried in the bidirectional `RedoxPrivateCsSubmit.seqno` field.
#[repr(C)]
#[derive(Clone, Copy, Debug)]
pub struct RedoxPrivateCsSubmitResult {
pub seqno: u64,
}
/// Bidirectional CS wait wire struct. The caller fills `seqno` and
/// `timeout_ns`; the kernel writes the completion status back into
/// `completed` and `completed_seqno` in the same struct.
#[repr(C)]
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, Default)]
pub struct RedoxPrivateCsWait {
pub seqno: u64,
pub timeout_ns: u64,
/// Response — non-zero if the waited-for seqno has completed.
pub completed: u8,
/// Padding so `completed_seqno` is naturally aligned to 8 bytes.
pub _pad: [u8; 7],
/// Response — highest completed seqno at the time of the call.
pub completed_seqno: u64,
}
#[repr(C)]
@@ -624,9 +644,9 @@ mod tests {
#[test]
fn redox_private_cs_submit_size() {
// src_handle(u32) + dst_handle(u32) + src_offset(u64) + dst_offset(u64) + byte_count(u64)
// = 4 + 4 + 8 + 8 + 8 = 32 bytes
assert_eq!(size_of::<RedoxPrivateCsSubmit>(), 32);
// src_handle(u32) + dst_handle(u32) + src_offset(u64) + dst_offset(u64)
// + byte_count(u64) + seqno(u64) = 4 + 4 + 8 + 8 + 8 + 8 = 40 bytes
assert_eq!(size_of::<RedoxPrivateCsSubmit>(), 40);
}
#[test]
@@ -637,8 +657,9 @@ mod tests {
#[test]
fn redox_private_cs_wait_size() {
// seqno(u64) + timeout_ns(u64) = 16 bytes
assert_eq!(size_of::<RedoxPrivateCsWait>(), 16);
// seqno(u64) + timeout_ns(u64) + completed(u8) + _pad([u8;7]) + completed_seqno(u64)
// = 8 + 8 + 8 + 8 = 32 bytes
assert_eq!(size_of::<RedoxPrivateCsWait>(), 32);
}
#[test]
@@ -694,12 +715,15 @@ mod tests {
assert_eq!(offset_of!(RedoxPrivateCsSubmit, src_offset), 8);
assert_eq!(offset_of!(RedoxPrivateCsSubmit, dst_offset), 16);
assert_eq!(offset_of!(RedoxPrivateCsSubmit, byte_count), 24);
assert_eq!(offset_of!(RedoxPrivateCsSubmit, seqno), 32);
}
#[test]
fn redox_private_cs_wait_is_repr_c() {
assert_eq!(offset_of!(RedoxPrivateCsWait, seqno), 0);
assert_eq!(offset_of!(RedoxPrivateCsWait, timeout_ns), 8);
assert_eq!(offset_of!(RedoxPrivateCsWait, completed), 16);
assert_eq!(offset_of!(RedoxPrivateCsWait, completed_seqno), 24);
}
#[test]
@@ -570,6 +570,7 @@ impl GpuDriver for AmdDriver {
src_offset: 0,
dst_offset: 0,
byte_count: 0,
..Default::default()
};
let result = self.redox_private_cs_submit(&submit)?;
Ok(result.seqno)
@@ -805,6 +805,7 @@ impl GpuDriver for IntelDriver {
src_offset: batch_start_offset,
dst_offset: 0,
byte_count: batch_len as u64,
..Default::default()
};
let result = self.redox_private_cs_submit(&submit)?;
Ok(result.seqno)
@@ -925,6 +925,7 @@ mod tests {
.redox_private_cs_wait(&RedoxPrivateCsWait {
seqno: 41,
timeout_ns: 0,
..Default::default()
})
.unwrap();
assert!(result.completed);
@@ -1152,6 +1152,7 @@ impl DrmScheme {
let req = crate::driver::RedoxPrivateCsWait {
seqno: u64::MAX,
timeout_ns: 0,
..Default::default()
};
match self.driver.redox_private_cs_wait(&req) {
Ok(r) => r.completed_seqno,
@@ -2155,6 +2156,7 @@ impl DrmScheme {
src_offset: req.src_offset,
dst_offset: req.dst_offset,
byte_count: req.size,
..Default::default()
};
self.validate_private_cs_ranges(&submit, "AMD SDMA submit")?;
req.seqno = self
@@ -2356,6 +2358,7 @@ impl DrmScheme {
.redox_private_cs_wait(&RedoxPrivateCsWait {
seqno: req.seqno,
timeout_ns: req.timeout_ns,
..Default::default()
})
.map_err(driver_to_syscall)?;
req.completed = u32::from(result.completed);
@@ -2433,7 +2436,7 @@ impl DrmScheme {
}
DRM_IOCTL_REDOX_PRIVATE_CS_SUBMIT => {
let req = decode_wire::<RedoxPrivateCsSubmit>(payload)?;
let mut req = decode_wire::<RedoxPrivateCsSubmit>(payload)?;
self.validate_private_cs_handles(
id,
req.src_handle,
@@ -2445,19 +2448,22 @@ impl DrmScheme {
.driver
.redox_private_cs_submit(&req)
.map_err(driver_to_syscall)?;
req.seqno = resp.seqno;
if let Some(handle) = self.handles.get_mut(&id) {
handle.last_submitted_seqno = resp.seqno;
}
bytes_of(&resp)
bytes_of(&req)
}
DRM_IOCTL_REDOX_PRIVATE_CS_WAIT => {
let req = decode_wire::<RedoxPrivateCsWait>(payload)?;
let mut req = decode_wire::<RedoxPrivateCsWait>(payload)?;
let resp: RedoxPrivateCsWaitResult = self
.driver
.redox_private_cs_wait(&req)
.map_err(driver_to_syscall)?;
bytes_of(&resp)
req.completed = u8::from(resp.completed);
req.completed_seqno = resp.completed_seqno;
bytes_of(&req)
}
// REDOX_SCANOUT_FLIP — flip the framebuffer on a CRTC.
@@ -2909,6 +2915,7 @@ impl SchemeSync for DrmScheme {
.redox_private_cs_wait(&RedoxPrivateCsWait {
seqno,
timeout_ns: FSYNC_TIMEOUT_NS,
..Default::default()
})
.map_err(driver_to_syscall)?;
@@ -3393,6 +3400,7 @@ mod tests {
src_offset: 0,
dst_offset: 0,
byte_count: 64,
..Default::default()
};
let err = write_ioctl(
&mut scheme,
@@ -3448,6 +3456,7 @@ mod tests {
let wait = RedoxPrivateCsWait {
seqno: 1,
timeout_ns: 0,
..Default::default()
};
let err =
@@ -3480,9 +3489,10 @@ mod tests {
src_offset: 0,
dst_offset: 0,
byte_count: 128,
..Default::default()
};
write_ioctl(&mut scheme, card, DRM_IOCTL_REDOX_PRIVATE_CS_SUBMIT, &submit).unwrap();
let _ = read_response::<RedoxPrivateCsSubmitResult>(&mut scheme, card);
let _ = read_response::<RedoxPrivateCsSubmit>(&mut scheme, card);
scheme.fsync(card).unwrap();
}
@@ -3512,6 +3522,7 @@ mod tests {
src_offset: 0,
dst_offset: 0,
byte_count: 128,
..Default::default()
};
write_ioctl(
@@ -3521,7 +3532,7 @@ mod tests {
&submit,
)
.unwrap();
let response = read_response::<RedoxPrivateCsSubmitResult>(&mut scheme, card);
let response = read_response::<RedoxPrivateCsSubmit>(&mut scheme, card);
assert_eq!(response.seqno, 7);
assert_eq!(driver.submit_calls(), 1);
@@ -3549,6 +3560,7 @@ mod tests {
src_offset: 4090,
dst_offset: 0,
byte_count: 64,
..Default::default()
};
let err = write_ioctl(
+5
View File
@@ -19,6 +19,11 @@ patches = [
# (enables with_dri → with_egl/gbm) and to the _GNU_SOURCE platform list.
# Was an untracked in-place edit lost on every clean re-extract.
"mesa/08-meson-redox-kms-drm.patch",
# Fix the CS submit seqno bug: merge the separate input/result wire structs
# into bidirectional structs so the kernel's global per-device seqno is
# read back instead of faked with a local counter. Required for correct
# multi-process GPU fence tracking (compositor + GPU clients).
"mesa/26-cs-submit-bidirectional-seqno.patch",
]
[build]
template = "custom"
@@ -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 @@ struct RedoxCsSubmitWire {
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 @@ redox_drm_cs_submit(struct redox_drm_winsys *rws,
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 @@ redox_drm_cs_submit(struct redox_drm_winsys *rws,
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 @@ redox_drm_cs_query_last_seqno(struct redox_drm_winsys *rws)
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 *