From cde8dcbfc7ce6bda3a9d010457c009a757c65e91 Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 27 Oct 2026 16:25:57 -0300 Subject: [PATCH 1/2] drm/asahi: lock only the VM reservation for read-only VM queries Submit validation (covers_ranges_batch and friends), translate_offset, vm_context_mask, new_execution_context and capacity only read VmInner, but took the full GPUVM exec lock: the VM reservation plus every external BO's reservation. Each submit therefore cost O(external BOs). Under virtio-gpu every guest BO is external (libkrun maps blobs through dma-buf export), so a game with ~15k BOs spent 12-20 ms per submit in drm_gpuvm_prepare_objects and was capped at single-digit fps. Every page-table and mapping change already holds the VM reservation, so readers need only that lock; use GpuVm::lock_inner() for them. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/mmu.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/asahi/mmu.rs b/drivers/gpu/drm/asahi/mmu.rs index 3522010949e35..8599b424745a8 100644 --- a/drivers/gpu/drm/asahi/mmu.rs +++ b/drivers/gpu/drm/asahi/mmu.rs @@ -1305,9 +1305,7 @@ impl KernelMapping { return Err(EINVAL); } let address = self.iova().checked_add(offset as u64).ok_or(EOVERFLOW)?; - lock_vm(&self.0.owner, None, false)? - .page_table - .translate_iova(address) + self.0.owner.lock_inner().page_table.translate_iova(address) } /// Writes a userspace timestamp through the retained GEM mapping. Aligned results @@ -2250,9 +2248,7 @@ impl Vm { } pub(crate) fn covers_range(&self, address: u64, size: u64, read: bool, write: bool) -> bool { - let Ok(mut inner) = lock_vm(&self.gpuvm, None, false) else { - return false; - }; + let mut inner = self.gpuvm.lock_inner(); Self::covers_range_locked(&mut inner, address, size, read, write) } @@ -2267,9 +2263,7 @@ impl Vm { if stride <= block_size { return false; } - let Ok(mut inner) = lock_vm(&self.gpuvm, None, false) else { - return false; - }; + let mut inner = self.gpuvm.lock_inner(); addresses.iter().all(|&address| { let Some(guard) = address.checked_add(block_size) else { return false; @@ -2279,12 +2273,12 @@ impl Vm { }) } - /// Checks all exact request tuples under one GPUVM exec lock. Access bit0 + /// Checks all exact request tuples under one hold of the VM reservation. Access bit0 /// requests reading and bit1 writing. The epoch names this same snapshot. pub(crate) fn covers_ranges_batch(&self, ranges: &[(u64, u64, u8)]) -> (bool, Option) { - let Ok(mut inner) = lock_vm(&self.gpuvm, None, false) else { - return (false, None); - }; + // Every mapping change holds the VM reservation, so readers need only that one. The full + // exec lock would also lock every external BO, making each submit O(external BOs). + let mut inner = self.gpuvm.lock_inner(); let epoch = inner.epoch; let covered = ranges.iter().all(|&(address, size, access)| { Self::covers_range_locked(&mut inner, address, size, access & 1 != 0, access & 2 != 0) @@ -3136,7 +3130,7 @@ impl Uat { return Err(EINVAL); } let root = { - let owner = lock_vm(&vm.gpuvm, None, false)?; + let owner = vm.gpuvm.lock_inner(); if owner.is_kernel || !Arc::ptr_eq(&owner.uat_inner, &self.inner) { return Err(EINVAL); } @@ -3281,7 +3275,7 @@ impl Uat { return Err(EINVAL); } { - let owner = lock_vm(&vm.gpuvm, None, false)?; + let owner = vm.gpuvm.lock_inner(); if owner.is_kernel || !core::ptr::eq(Arc::as_ptr(&owner.uat_inner), Arc::as_ptr(&self.inner)) { @@ -3663,7 +3657,7 @@ impl Vm { /// Diagnostic reservation occupancy (including guards), not a placement promise. /// Removal can progress between queries; allocations are excluded by the VM lock. pub(crate) fn capacity(&self, range: Range) -> Result<(u64, u64)> { - let inner = lock_vm(&self.gpuvm, None, false)?; + let inner = self.gpuvm.lock_inner(); let mut next = range.start; let mut used = 0; let mut largest = 0; From f3cc4a691438be2e067a4035241ee028370ad1fd Mon Sep 17 00:00:00 2001 From: Jonathan Goyvaerts Date: Tue, 27 Oct 2026 16:25:57 -0300 Subject: [PATCH 2/2] drm/asahi: lock only the VM reservation for VA updates Every map and unmap (user VM_BIND, kernel mappings such as each G17 submission's completion record, object teardown) took GpuVm::exec_lock(), i.e. drm_gpuvm_exec_lock(): the VM reservation plus the reservation of every external object. Under virtio-gpu every guest BO is external, so with ~15k BOs each bind and each submit spent around a millisecond in drm_gpuvm_prepare_objects(). This VM is IMMEDIATE_MODE and RESV_PROTECTED: VA updates need the VM reservation and each object's gpuva lock, which the link and unlink paths take themselves. Backing is pinned and its segments copied before any reservation is taken, no caller adds fences, and the driver already unmaps under the VM reservation alone on its low-memory path. Add GpuVm::exec_lock_vm(), which runs the drm_exec loop over the VM reservation (and an external obj, if given) without drm_gpuvm_prepare_objects(), and use it for all VA updates. Signed-off-by: Jonathan Goyvaerts Assisted-by: LLM --- drivers/gpu/drm/asahi/mmu.rs | 12 ++--- drivers/gpu/drm/asahi/mmu/bind.rs | 4 +- rust/helpers/drm_gpuvm.c | 6 +++ rust/kernel/drm/gpuvm/mod.rs | 81 +++++++++++++++++++++++++++++-- 4 files changed, 91 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/asahi/mmu.rs b/drivers/gpu/drm/asahi/mmu.rs index 8599b424745a8..c4afbc0127d70 100644 --- a/drivers/gpu/drm/asahi/mmu.rs +++ b/drivers/gpu/drm/asahi/mmu.rs @@ -2381,7 +2381,7 @@ impl Vm { let size = object_range.range(); let mut vm_exec = unsafe { - self.gpuvm.exec_lock(/*Some(gem)*/ None, false)? + self.gpuvm.exec_lock_vm(/*Some(gem)*/ None, false)? }; let inner = vm_exec.unique_gpuvm.data(); @@ -2461,7 +2461,7 @@ impl Vm { // SAFETY: no other UniqRefGpuVm exists so `GpuVm::exec_lock()` is safe let mut vm_exec = unsafe { self.gpuvm - .exec_lock(/*Some(&gem)*/ None, false) + .exec_lock_vm(/*Some(&gem)*/ None, false) .inspect_err(|_| { dev_err!(self.dev, "map_at(): Failed to exec_lock gpuvm\n"); })? @@ -2549,7 +2549,7 @@ impl Vm { let mut ctx = StepContext::new(Some(device.as_ref().as_ref()), prot, &mut resources)?; let mut vm_exec = unsafe { - self.gpuvm.exec_lock(/*Some(&gem)*/ None, false)? + self.gpuvm.exec_lock_vm(/*Some(&gem)*/ None, false)? }; let inner = vm_exec.unique_gpuvm.data(); @@ -2620,7 +2620,7 @@ impl Vm { size: usize, prot: Prot, ) -> Result { - let mut vm_exec = unsafe { self.gpuvm.exec_lock(None, false)? }; + let mut vm_exec = unsafe { self.gpuvm.exec_lock_vm(None, false)? }; if (iova as usize | phys | size) & UAT_PGMSK != 0 { dev_err!( @@ -2724,7 +2724,7 @@ impl Vm { let mut resources = VmOpResources::for_unmap()?; let mut ctx = StepContext::new(dev, Prot::default(), &mut resources)?; - let mut vm_exec = unsafe { self.gpuvm.exec_lock(None, false)? }; + let mut vm_exec = unsafe { self.gpuvm.exec_lock_vm(None, false)? }; mod_dev_dbg!(self.dev, "MMU: sm_unmap: {:#x}:{:#x}\n", iova, size); let _commit = if user { @@ -2755,7 +2755,7 @@ impl Vm { let mut ctx = StepContext::new(None, Prot::default(), &mut resources)?; // SAFETY: See `lock_vm()`. - let mut vm_exec = match unsafe { self.gpuvm.exec_lock(None, false) } { + let mut vm_exec = match unsafe { self.gpuvm.exec_lock_vm(None, false) } { Err(error) if error == ENOMEM => return self.drop_mappings_low_memory(gem), result => result?, }; diff --git a/drivers/gpu/drm/asahi/mmu/bind.rs b/drivers/gpu/drm/asahi/mmu/bind.rs index 0b3d85077aacb..0fa3f304af1d4 100644 --- a/drivers/gpu/drm/asahi/mmu/bind.rs +++ b/drivers/gpu/drm/asahi/mmu/bind.rs @@ -132,7 +132,7 @@ impl Vm { PreparedUserBindOp::Map(map) => { // SAFETY: See `lock_vm()`. As in `Vm::bind_object()`, the object itself is // not locked: the map step may still populate its pages. - let mut inner = unsafe { self.gpuvm.exec_lock(None, true)? }; + let mut inner = unsafe { self.gpuvm.exec_lock_vm(None, true)? }; match self.mapping_commit() { Ok(commit) => { let (flags, repeat) = if map.single_page { @@ -177,7 +177,7 @@ impl Vm { } PreparedUserBindOp::Unmap(unmap) => { // SAFETY: See `lock_vm()`. - let mut inner = unsafe { self.gpuvm.exec_lock(None, true)? }; + let mut inner = unsafe { self.gpuvm.exec_lock_vm(None, true)? }; match self.mapping_commit() { Ok(commit) => { let mut ctx = diff --git a/rust/helpers/drm_gpuvm.c b/rust/helpers/drm_gpuvm.c index 30fb1777fbf2b..d42d3d3491006 100644 --- a/rust/helpers/drm_gpuvm.c +++ b/rust/helpers/drm_gpuvm.c @@ -23,6 +23,12 @@ bool rust_helper_drm_gpuvm_is_extobj(struct drm_gpuvm *gpuvm, return drm_gpuvm_is_extobj(gpuvm, obj); } +__rust_helper +bool rust_helper_drm_exec_is_contended(struct drm_exec *exec) +{ + return drm_exec_is_contended(exec); +} + __rust_helper void rust_helper_drm_gpuvm_exec_unlock(struct drm_gpuvm_exec *vm_exec) { diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs index 146f3c9ed8362..d873eb4ef6384 100644 --- a/rust/kernel/drm/gpuvm/mod.rs +++ b/rust/kernel/drm/gpuvm/mod.rs @@ -60,6 +60,43 @@ pub use self::vm_bo::*; mod va; pub use self::va::*; +/// `drm_gpuvm_exec_lock()` without `drm_gpuvm_prepare_objects()`: the +/// `drm_exec_until_all_locked()` loop over the VM reservation and `vm_exec.extra`. +/// +/// # Safety +/// +/// `vm_exec` must be initialized as for `drm_gpuvm_exec_lock()`. +unsafe fn exec_lock_vm_raw(vm_exec: &mut bindings::drm_gpuvm_exec) -> core::ffi::c_int { + let exec = &raw mut vm_exec.exec; + // SAFETY: Per the safety requirements, `exec` belongs to a valid `drm_gpuvm_exec`. + unsafe { bindings::drm_exec_init(exec, vm_exec.flags, 0) }; + let mut ret = 0; + // SAFETY (whole loop): `exec` was initialized above and `vm_exec.vm` is a valid GPUVM. + // `drm_exec_cleanup()` drops all locks and restarts after contention, as the C macros do. + while unsafe { bindings::drm_exec_cleanup(exec) } { + ret = unsafe { bindings::drm_gpuvm_prepare_vm(vm_exec.vm, exec, vm_exec.num_fences) }; + if unsafe { bindings::drm_exec_is_contended(exec) } { + continue; + } + if ret == 0 { + if let Some(extra) = vm_exec.extra.fn_ { + ret = unsafe { extra(vm_exec) }; + if unsafe { bindings::drm_exec_is_contended(exec) } { + continue; + } + } + } + if ret != 0 { + break; + } + } + if ret != 0 { + // SAFETY: Finalize the context initialized above, as `drm_gpuvm_exec_lock()` does on error. + unsafe { bindings::drm_exec_fini(exec) }; + } + ret +} + pub(super) unsafe extern "C" fn exec_lock_gem_object( vm_exec: *mut bindings::drm_gpuvm_exec, ) -> core::ffi::c_int { @@ -260,6 +297,36 @@ impl GpuVm { &'a self, obj: Option<&T::Object>, interruptible: bool, + ) -> Result> { + // SAFETY: Same contract as this function. + unsafe { self.exec_lock_impl(obj, interruptible, true) } + } + + /// HACK: added for the asahi downstream driver. Like [`GpuVm::exec_lock`], but locks only + /// the VM's shared reservation (and `obj` if it is external), not every external object. + /// + /// Enough for VA updates of this immediate-mode, `DRM_GPUVM_RESV_PROTECTED` VM: they need + /// the VM reservation and each object's gpuva lock, which the link and unlink paths take. + /// Callers must not need external objects' reservations (fences, validation, eviction). + /// Locking all of them made each update cost O(external objects). + /// + /// # Safety + /// + /// Same as [`GpuVm::exec_lock`]. + pub unsafe fn exec_lock_vm<'a>( + &'a self, + obj: Option<&T::Object>, + interruptible: bool, + ) -> Result> { + // SAFETY: Same contract as this function. + unsafe { self.exec_lock_impl(obj, interruptible, false) } + } + + unsafe fn exec_lock_impl<'a>( + &'a self, + obj: Option<&T::Object>, + interruptible: bool, + all_objects: bool, ) -> Result> { // Do not try to lock the object if it is internal (since it is already locked). let is_ext = obj.map(|a| self.is_extobj(a)).unwrap_or(false); @@ -286,10 +353,16 @@ impl GpuVm { GFP_KERNEL, )?; - // SAFETY: The object is valid and was initialized above. On failure, - // `drm_gpuvm_exec_lock()` has already finalized the exec context, so the box only needs to - // be freed, which dropping it does. - to_result(unsafe { bindings::drm_gpuvm_exec_lock(&mut *vm_exec) })?; + // SAFETY: The object is valid and was initialized above. On failure, both lock functions + // have already finalized the exec context, so the box only needs to be freed, which + // dropping it does. + to_result(unsafe { + if all_objects { + bindings::drm_gpuvm_exec_lock(&mut *vm_exec) + } else { + exec_lock_vm_raw(&mut *vm_exec) + } + })?; // Build the guard, and take its VM reference, only once the locks are held: its `Drop` // unlocks them.