From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f74.google.com (mail-wr1-f74.google.com [209.85.221.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DD42D33C53F for ; Fri, 10 Jul 2026 14:15:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783692913; cv=none; b=Q0N7MysAXV54WFxGX0DYACfYhJZS7ulxbzvYdvDi41Gm9Rog1BV5TAiXqhp3wz0km+LBqAblFVW4XkOXCRFX6f9SI/z13HntPXrSo+ubUgUPxT+fj8osiyp77K7XQCmKmorj0THv3v9SaqOfln04S8uzUpBsjQ+cjSNbX2fkC+o= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783692913; c=relaxed/simple; bh=lOt+ARBIoeRQpbIeowh2vYdIaEIo+NWTnmhRuFyJtmc=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=BeAP8ttiViKiNLGhRnPfewM1OkBes1jMpojLLdR2OxgYzv+mkSj3gPj2Ru0oWnS5bBDAxXBF2Bqy5dcWNkU7VW9MXu/xh4Eb5Dq2/4s6bmyzupJfvlP6BsgXjkjie2xnag2jduGRGK2sD7LHRJRnNJnuIFPuIslPnxswwfGItII= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=C4sv87sg; arc=none smtp.client-ip=209.85.221.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="C4sv87sg" Received: by mail-wr1-f74.google.com with SMTP id ffacd0b85a97d-4767036f8faso605630f8f.3 for ; Fri, 10 Jul 2026 07:15:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1783692910; x=1784297710; darn=vger.kernel.org; h=content-type:cc:to:from:subject:message-id:references:mime-version :in-reply-to:date:from:to:cc:subject:date:message-id:reply-to :content-type; bh=cZ/Rax3JtQ73njUkK4P8UwJJ63zTUvy1XJcf1ykx4E4=; b=C4sv87sgtvq7+UxfDX/4S+mQzPdkZ8T8IQ1qGV60rFLI9suJXozpNCNUt9SOy/1QTW w0bTQ1vo0iLoYpAqkeX3I7hqs3uGWpvQ88nbvdFW/EyXMsBBwyO+GArW5h2dkz+UskJp apv6RK35T0muP9ENYMYTSuRtiPvbX3u0mBG2oK/wRnNeTqMKT2mFiOig7AJVsT+B8iDg ZN1wxFok1BHjIULPwQ2SNEh+ivkSWOJKyuB+z12OTvBErrX3RLPVObWK6NglXSJmGpkd 3E8eAUzMZZ+K8UTg5KBG4asbpsyDqQw98tP5cmglKpHE189S/lFfWgqT/87/MFUHuPkb Vetw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783692910; x=1784297710; h=content-type:cc:to:from:subject:message-id:references:mime-version :in-reply-to:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=cZ/Rax3JtQ73njUkK4P8UwJJ63zTUvy1XJcf1ykx4E4=; b=lBzCoNVVzLisMHrAwzNettePH+QTuJNuAmfBHyibsIshwh+FtyZFzQWAvOWKqy8i2O N8ki5rKWThtM1WDGNEjY8MXewzYjZfL4qlMslqkJHrdkq7s0OxYt5/Peakjsr7f0ymon GekGndV/K4KvYsQmBiuWvTnP24zQcEFBeN+D+5nnZtDp++/sJwp7Uxu1oNhnCtjZKABE yJ0PxP2ueC3TUo9UDE3PlBAf6JbFdR0uZAQeObmR6mhlM5n5aC/1Tctg0zPa/m1fe4Lq OifV0Tjgn7rJGfE63DhEKupfZPja7QR/C+IB6i62HGmJVHUqISCXEG+69nMmSwRQvYLY h7EA== X-Forwarded-Encrypted: i=1; AHgh+RrLLhCBdsC23d4u9ZDNH2vsxDr09vgA0ED3bCjGOpcx0SKlEoF51s/pUaBhy6p+iGGzYo4FE979eL7bnps=@vger.kernel.org X-Gm-Message-State: AOJu0YzXaWUp2btjRQGNpzsmZ4S9G8GCyZtBrmMU1l1mX51FXPok27LT ppKQ5xOBpdn3L8dgvI3bVf4QakmMIreRZtPD87P7QsDAtImXy17z6CbSxyEGFYo+f9ccPHZ0bsA XDFbVS5NUrUF2AfUlaQ== X-Received: from wrqt12.prod.google.com ([2002:a5d:460c:0:b0:463:dd19:ae30]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6000:24c2:b0:475:f100:360c with SMTP id ffacd0b85a97d-47df0822e2dmr12652945f8f.59.1783692909823; Fri, 10 Jul 2026 07:15:09 -0700 (PDT) Date: Fri, 10 Jul 2026 14:15:09 +0000 In-Reply-To: <20260709-fw-boot-b4-v6-4-ca391e1a4108@collabora.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260709-fw-boot-b4-v6-0-ca391e1a4108@collabora.com> <20260709-fw-boot-b4-v6-4-ca391e1a4108@collabora.com> Message-ID: Subject: Re: [PATCH v6 4/7] drm/tyr: add GPU virtual memory (VM) support From: Alice Ryhl To: Deborah Brouwer Cc: Daniel Almeida , Danilo Krummrich , David Airlie , Simona Vetter , Benno Lossin , Gary Guo , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, boris.brezillon@collabora.com, samitolvanen@google.com, acourbot@nvidia.com, alvin.sun@linux.dev, laura.nao@collabora.com, work@onurozkan.dev, beata.michalska@arm.com, steven.price@arm.com, lyude@redhat.com Content-Type: text/plain; charset="utf-8" On Thu, Jul 09, 2026 at 02:36:44PM -0700, Deborah Brouwer wrote: > From: Boris Brezillon > > Add GPU virtual address space management using the DRM GPUVM framework. > Each virtual memory (VM) space is backed by ARM64 LPAE Stage 1 page tables > and can be mapped into hardware address space (AS) slots for GPU execution. > > The implementation provides memory isolation and virtual address > allocation. VMs support mapping GEM buffer objects with configurable > protection flags (readonly, noexec, uncached) and handle both 4KB and 2MB > page sizes. A new_dummy_object() helper is provided to create a dummy GEM > object for use as a GPUVM root. > > The vm module integrates with the MMU for address space activation and > provides map/unmap/remap operations with page table synchronization. > > Signed-off-by: Boris Brezillon > Co-developed-by: Daniel Almeida > Signed-off-by: Daniel Almeida > Co-developed-by: Deborah Brouwer > Signed-off-by: Deborah Brouwer > +impl core::fmt::Display for VmMapFlags { Use kernel::fmt instead of core::fmt. > +impl TryFrom for VmMapFlags { > + type Error = Error; > + > + fn try_from(value: u32) -> core::result::Result { Simplifies to Result without full path. > + let valid = (kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_READONLY > + | kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC > + | kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED) > + as u32; Simplifies to let valid = Self::Readonly as u32 | Self::Noexec as u32 | Self::Uncached as u32; > +impl Drop for PtUpdateContext<'_, '_> { > + fn drop(&mut self) { > + if let Err(e) = self.mmu.end_vm_update(self.as_data) { > + pr_err!("Failed to end VM update {:?}\n", e); > + } > + > + if let Err(e) = self.mmu.flush_vm(self.as_data) { > + pr_err!("Failed to flush VM {:?}\n", e); > + } I assume it's not possible for this to run during a VM eviction, right? > + let gpuvm_unique = kernel::drm::gpuvm::GpuVm::new::( > + c_str!("Tyr::GpuVm"), This can just be c"Tyr::GpuVm". The macro is only needed in macro contexts where this rewrite is not possible e.g. because the macro's caller provides a non-c str literal. > + let mut gpuvm_unique = self.gpuvm_unique.lock(); > + > + self.exec_op(gpuvm_unique.as_mut().get_mut(), req, &mut resources)?; > + > + // We flush the defer cleanup list now. Things will be different in > + // the asynchronous VM_BIND path, where we want the cleanup to > + // happen outside the DMA signalling path. > + self.gpuvm.deferred_cleanup(); You should probably release the gpuvm_unique mutex before invoking deferred_cleanup(). (Multiple occurances in this patch.) > + if gem_offset > 0 { > + // Skip the entire SGT entry if the gem_offset exceeds its length > + let skip = sgt_entry_length.min(gem_offset); Nit: this may just be my idiosyncrasy, but I think this is more readable as: usize::min(sgt_entry_length, gem_offset) (Multiple occurances in this file.) > + let segment_mapped = match pt_map(context.pt, iova, paddr, len, prot) { > + Ok(segment_mapped) => segment_mapped, > + Err(e) => { > + // clean up any successful mappings from previous SGT entries. > + let total_mapped = iova - start_iova; > + if total_mapped > 0 { > + pt_unmap(context.pt, start_iova..(start_iova + total_mapped)).ok(); Nit: the idiomatic way to ignore errors is to write: let _ = pt_unmap(context.pt, start_iova..(start_iova + total_mapped)); instead of converting the Result into an Option with .ok() and abusing the fact that Option is not #[must_use]. > +/// This function selects the largest supported block size (currently 4KB or 2MB) > +/// that can be used for a mapping at the given address and size, respecting alignment constraints. > +/// > +/// We can map multiple pages at once but we can't exceed the size of the > +// table entry itself. So, if mapping 4KB pages, figure out how many pages > +// can be mapped before we hit the 2MB boundary. Or, if mapping 2MB pages, > +// figure out how many pages can be mapped before hitting the 1GB boundary > +// Returns the page size (4KB or 2MB) and the number of pages that can be mapped at that size. > +fn get_pgsize(addr: u64, size: u64) -> (u64, u64) { Some of these comments are missing a / character. > + // SAFETY: Exclusive access to the page table is ensured because > + // the pt reference comes from PtUpdateContext, which is created > + // during a VM update operation, ensuring the driver does not concurrently > + // modify the page table. > + let (mapped, result) = unsafe { > + pt.map_pages( > + curr_iova as usize, > + (curr_paddr as usize).try_into().unwrap(), > + pgsize as usize, > + pgcount as usize, > + prot, > + GFP_KERNEL, > + ) > + }; It would be ideal if this SAFETY comment explains why this safety requirement is satisfied: This page table must not contain any mapping that overlaps with the mapping created by this call. (It's because GPUVM tells you to unmap pages before it tells you to map them.) > +/// Unmaps a virtual address range from the page table. > +/// > +/// This function removes all page table entries in the specified range, > +/// automatically handling different page sizes that may be present. > +fn pt_unmap(pt: &IoPageTable<'_, ARM64LPAES1>, range: Range) -> Result { > + let mut iova = range.start; > + let mut bytes_left_to_unmap = range.end - range.start; > + > + while bytes_left_to_unmap > 0 { > + let (pgsize, pgcount) = get_pgsize(iova, bytes_left_to_unmap); > + > + // SAFETY: Exclusive access to the page table is ensured because > + // the pt reference comes from PtUpdateContext, which was > + // created while holding &mut Vm, preventing any other access to the > + // page table for the duration of this operation. > + let unmapped = unsafe { pt.unmap_pages(iova as usize, pgsize as usize, pgcount as usize) }; It would be ideal if this SAFETY comment explains why this safety requirement is satisfied: This page table must contain one or more consecutive mappings starting at `iova` whose total size is `pgcount * pgsize`. I.e. why can we be sure that this call isn't trying to unmap a 4KB sub-page in something that exists as a 2MB mapping in the page table? (It's because GPUVM tells you to unmap exactly the range you were previously told to map.) Alice