From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f74.google.com (mail-wm1-f74.google.com [209.85.128.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 296E9366540 for ; Thu, 22 Jan 2026 08:20:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769070058; cv=none; b=eocVvJbtGrC3hRywr6AUZvOYTQyl4KQqLeqls2bDpDos/R9spw4WCErh09QtdbVEXyuClBcoNyL16IepzspzbiLsXSa0iaw12Gtsl4Xw9dRrNXIJNGObOHa/MbJo+CwT9gDFd6Dolk1vjj7Hu3UYhkmWICRe07tAvrXvOxB7TBE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769070058; c=relaxed/simple; bh=9w6QFLbX0NeeTXSjGz7HKZUl36ItxnsMp+HVMGdrtt8=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=IdDDwxoGb25Y+l6ODl+6v9PB0VcxsadEvr2XW63TYesIQtHdzL2YFIDA/fUZ+CUuDab1T9Vwu7IIoPnHG6eTkq7PEM9jZeIjOih7HjE6TeGorWy2j41WIqCroes/6cAzvvlA1/aQ2JJ4lzM2Lax0R17ALsONYh0yPapC2ZkPiLo= 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=Ntr5xs5p; arc=none smtp.client-ip=209.85.128.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="Ntr5xs5p" Received: by mail-wm1-f74.google.com with SMTP id 5b1f17b1804b1-4803b4e3b9eso5318475e9.3 for ; Thu, 22 Jan 2026 00:20:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1769070054; x=1769674854; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=PlwMpAn9Al0QHgVBZe5vJEVXfz0qnjNPXGGsZraEwjM=; b=Ntr5xs5p8H1lr6QAii8GgNFo5OGB+F8+p1PfR4ih/DDjCp1Iqv985amb2dl8JNdYmL JopT2/Yni2omeQ73NMoZWecr3bDcNIPrGXx70AbjjRPH17e10zF3lOpw+OiRFLcQbXrz FuuWsGs4988BPAt4dN/59ZLVkAObRNr9otyRU7A7wQLrDXIzUWi5Ok2kDicjh0rag6i5 gbIQx8tbSiAfvdHFWOpjfIwtnZgRKT7uQZrpB6UduApvsc1SGZEF+jxGN8nl0VQRdFUi KrHvPcxxmnW+TAwj5yhoeBKTiW8lFUPBbNnPITul3fcvBiCH7aL2aJV2V5T8hPHwEM5y 12PQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769070054; x=1769674854; h=content-transfer-encoding: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; bh=PlwMpAn9Al0QHgVBZe5vJEVXfz0qnjNPXGGsZraEwjM=; b=k/MYwVp+S6U/zQ8jwVoturk3q2hf2xNEkKqmbXUFsPaUahOUY4oxueIcKlnnp214yH Gahb5L3C6TE/jl1LP8tfM99PUht3HY5MSK3boY2gRxkUTBK2yl3q+HSHO0uqRMxMShBZ v/38w9d7a7dad2FpeFFLRwfTTljuufM7kY6NTngwkT4hO4eKNPd/tTUxmSocU/mEIHpo Ek0P00COxGPOwkJoqOQGB6qfghnTuDCpM+sZfwYVlR9KJ7AwzXjb0oANdVtNgk6cCMg6 jcUUF6G7xkWf1/vh8EVPEXSuNVgsNoed087l/ILKpmRXmoLGPc+4+9vFBfZp0RctIVYs +vrg== X-Forwarded-Encrypted: i=1; AJvYcCX+o6kVL+5WYK9hoUlQ4rpZlHw5q/vi5kb4aWpTcolByeagW6JNnMFHMWtHjNoMpwKujDnNpnzOz7nBnk8=@vger.kernel.org X-Gm-Message-State: AOJu0Yzt6JXc9UBfVxM/O9RnsBAdo/JJzc479wo68tgPqkNHGGSKIs/L 2y43GBd3a80cb0AWQIoc/Pmyj8XA84MDd1H60N/DFADREU3Ow4Pz29ersc7HtD6gdzCKmOfw4kP PC5tQ1cA4uvLG6d+hPw== X-Received: from wmow17.prod.google.com ([2002:a05:600c:4751:b0:47e:e4a5:c5f2]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:600d:6413:10b0:480:3a72:5c10 with SMTP id 5b1f17b1804b1-4803a725caemr150095185e9.16.1769070054393; Thu, 22 Jan 2026 00:20:54 -0800 (PST) Date: Thu, 22 Jan 2026 08:20:53 +0000 In-Reply-To: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260121-gpuvm-rust-v3-0-dd95c04aec35@google.com> <20260121-gpuvm-rust-v3-3-dd95c04aec35@google.com> Message-ID: Subject: Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain() From: Alice Ryhl To: Daniel Almeida Cc: Danilo Krummrich , Boris Brezillon , Janne Grunau , Matthew Brost , "Thomas =?utf-8?Q?Hellstr=C3=B6m?=" , Lyude Paul , Asahi Lina , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Wed, Jan 21, 2026 at 02:31:26PM -0300, Daniel Almeida wrote: > Hi Alice, >=20 > This looks good. See a few nits below: >=20 > > On 21 Jan 2026, at 08:31, Alice Ryhl wrote: > >=20 > > This provides a mechanism to create (or look up) VMBO instances, which > > represent the mapping between GPUVM and GEM objects. > >=20 > > The GpuVmBoResident type can be considered like ARef>, > > except that no way to increment the refcount is provided. >=20 > So this is like GpuVmCore, right? Since that is an ARef whose refcount ca= nnot > be incremented. Sort of, except that GpuVmBoResident is not truly unique since you can call obtain() twice. > > The GpuVmBoAlloc type is more akin to a pre-allocated GpuVm, so > > it's not really a GpuVm yet. Its destructor could call >=20 > Maybe you mean a pre-allocated GpuVmBo? Yes that's what I meant. > > drm_gpuvm_bo_destroy_not_in_lists(), but as the type is currently > > private and never called anywhere, this perf optimization does not need > > to happen now. > >=20 > > Pre-allocating and obtaining the gpuvm_bo object is exposed as a single > > step. This could theoretically be a problem if one wanted to call > > drm_gpuvm_bo_obtain_prealloc() during the fence signalling critical > > path, but that's not a possibility because: > >=20 > > 1. Adding the BO to the extobj list requires the resv lock, so it canno= t > > happen during the fence signalling critical path. > > 2. obtain() requires that the BO is not in the extobj list, so obtain() > > must be called before adding the BO to the extobj list. > >=20 > > Thus, drm_gpuvm_bo_obtain_prealloc() cannot be called during the fence > > signalling critical path. (For extobjs at least.) >=20 > What about internal objects? This is merely a sanity check from my side. There are only two lists: extobj and evicted. The extobj list is used to find the dma_resv locks of external objects. This isn't required for internal ones, as they all share the resv lock of the GPUVM itself. > > + #[inline] > > + fn raw_resv_lock(&self) -> *mut bindings::dma_resv { > > + // SAFETY: `r_obj` is immutable and valid for duration of GPUV= M. > > + unsafe { (*(*self.as_raw()).r_obj).resv } > > + } >=20 > Shouldn=E2=80=99t we call this raw_resv? Adding =E2=80=9Clock=E2=80=9D to= a name may incorrectly > hint that the lock is actually taken. Good call. > > + /// Custom function for allocating a `drm_gpuvm_bo`. > > + /// > > + /// # Safety > > + /// > > + /// Always safe to call. > > + // Unsafe to match function pointer type in C struct. >=20 > Is this missing an extra =E2=80=9C/=E2=80=9C token? Also, I think this is= a bit hard to parse initially. Well, I didn't meant to include it in the docs. But I can reformat this to be less confusing. > > + /// Look up whether there is an existing [`GpuVmBo`] for this gem = object. > > + #[inline] > > + pub(super) fn obtain(self) -> GpuVmBoResident { > > + let me =3D ManuallyDrop::new(self); > > + // SAFETY: Valid `drm_gpuvm_bo` not already in the lists. > > + let ptr =3D unsafe { bindings::drm_gpuvm_bo_obtain_prealloc(me= .as_raw()) }; > > + > > + // If the vm_bo does not already exist, ensure that it's in th= e extobj list. >=20 > Wording is a bit off. Obviously only external objects should be in the ex= tobj > list. This is correctly captured by the check below, but the wording abov= e > makes it sound unconditional. I'll update the comment. The "does not already exist" refers to the `ptr::eq()` part of the condition, which checks whether the pre-allocated vm_bo was created, or whether the GPUVM already has a vm_bo for this GEM object. > > + if ptr::eq(ptr, me.as_raw()) && me.gpuvm().is_extobj(me.obj())= { > > + let resv_lock =3D me.gpuvm().raw_resv_lock(); > > + // SAFETY: The GPUVM is still alive, so its resv lock is t= oo. > > + unsafe { bindings::dma_resv_lock(resv_lock, ptr::null_mut(= )) }; > > + // SAFETY: We hold the GPUVMs resv lock. > > + unsafe { bindings::drm_gpuvm_bo_extobj_add(ptr) }; > > + // SAFETY: We took the lock, so we can unlock it. > > + unsafe { bindings::dma_resv_unlock(resv_lock) }; > > + } > > + > > + // INVARIANTS: Valid `drm_gpuvm_bo` in the GEM list. > > + // SAFETY: `drm_gpuvm_bo_obtain_prealloc` always returns a non= -null ptr > > + GpuVmBoResident(unsafe { NonNull::new_unchecked(ptr.cast()) }) > > + } > > +} > Reviewed-by: Daniel Almeida Thanks for the reivew! Alice