Three fixes to the safe Rust KMS layer, shaken out while bringing up the
out-of-tree evdi virtual-display driver (they are equally correct for any
KmsDriver, including vino):
* kms: store MODE_CONFIG_OPS as a 'static reference. probe_kms() installed
mode_config.funcs / .helper_private from a by-value ModeConfigOps local,
leaving those pointers dangling once probe_kms() returned. The DRM core
later read a garbage mode_valid slot out of the reused stack and jumped to
it (RIP=0x9). Promoting the vtable to a const-promoted &'static keeps it
alive for the device's lifetime.
* framebuffer: bracket the vmap in drm_gem_fb_begin/end_cpu_access. Reads
through the CPU mapping could otherwise return stale cached pixels (an
earlier compositor frame) while new frames were rendered elsewhere -- the
scanout appeared frozen. end_cpu_access is balanced in the vmap guard's
Drop.
* driver: call drm_atomic_helper_shutdown() from Registration::drop() before
drm_dev_unplug(), replacing the commented-out TODO. Without it the last
committed atomic state kept framebuffers -- and the connectors they
reference -- pinned, surfacing as drm_mode_config_cleanup() "leaked"
warnings at teardown.
Signed-off-by: Mike Lothian <[email protected]>
Assisted-by: Claude:claude-opus-4-8 [Claude-Code]
---
rust/kernel/drm/driver.rs | 24 ++++++++++++------------
rust/kernel/drm/kms.rs | 12 +++++++++---
rust/kernel/drm/kms/framebuffer.rs | 24 ++++++++++++++++++++++++
3 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index b7e80df6289e..1d9309bc41c2 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -287,6 +287,18 @@ unsafe impl<T: Driver> Send for Registration<T> where
impl<T: Driver> Drop for Registration<T> {
fn drop(&mut self) {
+ // Disable the display pipeline before unregistering. KMS drivers must
shut the active
+ // configuration down at teardown; otherwise the committed atomic
state keeps the
+ // framebuffers -- and the mode objects it references, such as the
still-attached
+ // connectors -- alive, which surfaces later as
`drm_mode_config_cleanup()` "leaked"
+ // warnings once the last userspace fd closes. This must run while the
device is still
+ // registered (i.e. before `drm_dev_unplug()`).
+ if drm::Device::<T>::has_kms() {
+ // SAFETY: KMS is set up for this device (checked above) and it is
still registered, so
+ // committing the "everything off" state is valid.
+ unsafe { bindings::drm_atomic_helper_shutdown(self.drm.as_raw()) };
+ }
+
// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure
that existing
// `drm_dev_enter()` critical sections complete before unregistration
proceeds. This
// is required for the safety of `UnbindGuard`, which relies on the
SRCU barrier in
@@ -299,17 +311,5 @@ fn drop(&mut self) {
// After drm_dev_unplug(), the SRCU barrier guarantees that all
UnbindGuard critical
// sections have completed, so no one holds a reference to reg_data
anymore.
// reg_data is dropped here automatically.
-
- /* TODO: We need to figure out what to do about this, and whether or
not we should just call
- * this before dev unplug
- */
- /*
- if drm::device::Device::<T>::has_kms() {
- // SAFETY: We just checked above that KMS was setup for this
device, so this is safe to
- // call
- unsafe { bindings::drm_atomic_helper_shutdown(self.0.as_raw()) }
- }
- */
- //build_error!("You forgot to update the cleanup methods")
}
}
diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
index 874a2ebc7887..f1472e913952 100644
--- a/rust/kernel/drm/kms.rs
+++ b/rust/kernel/drm/kms.rs
@@ -59,7 +59,13 @@ pub trait KmsImpl {
type Driver: Driver;
/// The optional KMS callback operations for this driver.
- const MODE_CONFIG_OPS: Option<ModeConfigOps>;
+ ///
+ /// This is a `'static` reference so that the `drm_mode_config_funcs`
and
+ /// `drm_mode_config_helper_funcs` pointers installed into
`drm_device.mode_config` in
+ /// [`probe_kms`] outlive `probe_kms`'s stack frame. Storing
`&ops.kms_vtable` from a
+ /// by-value local left `mode_config.funcs` dangling once `probe_kms`
returned; the DRM core
+ /// then read a garbage `mode_valid` slot out of the reused stack and
called it (RIP=0x9).
+ const MODE_CONFIG_OPS: Option<&'static ModeConfigOps>;
/// The callback for using a Driver's callbacks to probe the KMS
capabilities and object
/// topology.
@@ -189,7 +195,7 @@ fn atomic_commit_tail<'a>(
impl<T: KmsDriver> private::KmsImpl for T {
type Driver = Self;
- const MODE_CONFIG_OPS: Option<ModeConfigOps> = Some(ModeConfigOps {
+ const MODE_CONFIG_OPS: Option<&'static ModeConfigOps> =
Some(&ModeConfigOps {
kms_vtable: bindings::drm_mode_config_funcs {
atomic_check: Some(bindings::drm_atomic_helper_check),
fb_create: Some(bindings::drm_gem_fb_create),
@@ -269,7 +275,7 @@ impl<T: KmsDriver> KmsImpl for T {}
impl<T: Driver> private::KmsImpl for PhantomData<T> {
type Driver = T;
- const MODE_CONFIG_OPS: Option<ModeConfigOps> = None;
+ const MODE_CONFIG_OPS: Option<&'static ModeConfigOps> = None;
}
impl<T: Driver> KmsImpl for PhantomData<T> {}
diff --git a/rust/kernel/drm/kms/framebuffer.rs
b/rust/kernel/drm/kms/framebuffer.rs
index e12fa7a24e15..00696b733c7a 100644
--- a/rust/kernel/drm/kms/framebuffer.rs
+++ b/rust/kernel/drm/kms/framebuffer.rs
@@ -171,6 +171,21 @@ pub fn vmap(&self) -> Result<FramebufferVmap<'_, T>> {
unsafe { bindings::drm_gem_fb_vunmap(self.as_raw(),
map.as_mut_ptr()) };
return Err(EINVAL);
}
+ // Synchronise the CPU's view with any GPU (or dma-buf importer)
writes before the caller
+ // reads the pixels. Without this, reads through the vmap can return
stale *cached* data --
+ // e.g. a compositor's earlier frame -- so the scanout appears frozen
while the GPU keeps
+ // rendering new frames elsewhere. `end_cpu_access` in `Drop` balances
it.
+ // SAFETY: `self.as_raw()` is the valid GEM-backed framebuffer just
vmapped above.
+ if let Err(e) = to_result(unsafe {
+ bindings::drm_gem_fb_begin_cpu_access(
+ self.as_raw(),
+ bindings::dma_data_direction_DMA_FROM_DEVICE,
+ )
+ }) {
+ // SAFETY: balances the vmap just done, with the same `map`.
+ unsafe { bindings::drm_gem_fb_vunmap(self.as_raw(),
map.as_mut_ptr()) };
+ return Err(e);
+ }
Ok(FramebufferVmap { fb: self, map, _p: PhantomData })
}
}
@@ -209,6 +224,15 @@ pub fn as_bytes(&self) -> &[u8] {
impl<'a, T: KmsDriver> Drop for FramebufferVmap<'a, T> {
fn drop(&mut self) {
+ // Balance the `begin_cpu_access` from `vmap` before unmapping.
+ // SAFETY: `self.fb.as_raw()` is the framebuffer passed to
`drm_gem_fb_begin_cpu_access`
+ // in `vmap`, with the same direction.
+ unsafe {
+ bindings::drm_gem_fb_end_cpu_access(
+ self.fb.as_raw(),
+ bindings::dma_data_direction_DMA_FROM_DEVICE,
+ )
+ };
// SAFETY: `self.fb.as_raw()`/`self.map` are exactly the pair passed to
// `drm_gem_fb_vmap` in `Framebuffer::vmap`, and the mapping has not
been released
// since.
--
2.55.0
