On 7/1/2026 5:13 PM, Christian König wrote: > On 6/17/26 08:54, Zhu Lingshan wrote: >> amdgpu_bo_create_reserved() only allocates a new BO when >> *bo_ptr (struct amdgpu_bo **bo_ptr as input parameter) is >> NULL, it simply skips creation when *bo_ptr is non-NULL. >> But it unconditionally reserves, pins, gart allocates >> and maps the BO afterwards. >> >> When the same non-NULL BO pointer is passed in again, >> for example firmware buffers that live in adev and are >> re-loaded on every resume / cp_resume / start >> under AMDGPU_FW_LOAD_DIRECT, amdgpu_bo_pin() just increases >> pin_count unconditionally, however the matching teardown only unpins >> once, so pin_count never drops to zero, so TTM is not able >> to move, swap or evict a BO, causing BO leaks. > That's a good catch. > >> Ideally the BO should only be pinned once at creation. So this commit >> splits current amdgpu_bo_create_reserved() into two helpers: >> >> - amdgpu_bo_create_pinned(): create + reserve + pin + alloc_gart and >> return the BO unreserved. It requires *bo_ptr == NULL on entry, >> so only pins the BO once. > That sounds like a bad idea to me, just move the pinning under the if as well. > > This just creates a wrapper function for a wrapper function.
Yeah, that is a good idea, will send V2 soon. Thanks Lingshan > > Regards, > Christian. > >> - amdgpu_bo_get_access(): reserve + map gpu/cpu addr + unreserve. >> This function is idempotent and is safe to call multiple times >> on an existing pinned BO, for example, on every resume path. >> >> amdgpu_bo_create_reserved() now calls amdgpu_bo_create_pinned() >> only when *bo_ptr is NULL, then calls amdgpu_bo_get_access() for >> CPU/GPU address mapping, and reserves the BO before returning. >> Repeated calls no longer take additional pin references. >> >> Signed-off-by: Zhu Lingshan <[email protected]> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 175 +++++++++++++++++---- >> 1 file changed, 143 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> index 4dd7c712b8c3..dd0a59137028 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> @@ -217,57 +217,53 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo >> *abo, u32 domain) >> } >> >> /** >> - * amdgpu_bo_create_reserved - create reserved BO for kernel use >> + * amdgpu_bo_create_pinned - create and pin a BO for kernel use >> * >> * @adev: amdgpu device object >> * @size: size for the new BO >> * @align: alignment for the new BO >> * @domain: where to place it >> - * @bo_ptr: used to initialize BOs in structures >> - * @gpu_addr: GPU addr of the pinned BO >> - * @cpu_addr: optional CPU address mapping >> + * @bo_ptr: used to return the newly created BO, must point to NULL on entry >> + * @cpu_access: true if the BO needs to be CPU accessible >> * >> - * Allocates and pins a BO for kernel internal use, and returns it still >> - * reserved. >> + * Allocates and pins a BO for kernel internal use, and returns it >> unreserved >> * >> - * Note: For bo_ptr new BO is only created if bo_ptr points to NULL. >> + * Note: *bo_ptr must be NULL on entry, this helper always creates >> + * a new BO and never reuses an existing one, >> + * so it never pins the same BO twice. >> * >> * Returns: >> * 0 on success, negative error code otherwise. >> */ >> -int amdgpu_bo_create_reserved(struct amdgpu_device *adev, >> - unsigned long size, int align, >> - u32 domain, struct amdgpu_bo **bo_ptr, >> - u64 *gpu_addr, void **cpu_addr) >> +static int amdgpu_bo_create_pinned(struct amdgpu_device *adev, >> + unsigned long size, int align, >> + u32 domain, struct amdgpu_bo **bo_ptr, >> + bool cpu_access) >> { >> struct amdgpu_bo_param bp; >> - bool free = false; >> int r; >> >> - if (!size) { >> - amdgpu_bo_unref(bo_ptr); >> - return 0; >> - } >> + if (WARN_ON(!bo_ptr || *bo_ptr)) >> + return -EINVAL; >> + >> + if (WARN_ON(!size)) >> + return -EINVAL; >> >> memset(&bp, 0, sizeof(bp)); >> bp.size = size; >> bp.byte_align = align; >> bp.domain = domain; >> - bp.flags = cpu_addr ? AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED >> + bp.flags = cpu_access ? AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED >> : AMDGPU_GEM_CREATE_NO_CPU_ACCESS; >> bp.flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS; >> bp.type = ttm_bo_type_kernel; >> bp.resv = NULL; >> bp.bo_ptr_size = sizeof(struct amdgpu_bo); >> >> - if (!*bo_ptr) { >> - r = amdgpu_bo_create(adev, &bp, bo_ptr); >> - if (r) { >> - dev_err(adev->dev, "(%d) failed to allocate kernel >> bo\n", >> - r); >> - return r; >> - } >> - free = true; >> + r = amdgpu_bo_create(adev, &bp, bo_ptr); >> + if (r) { >> + dev_err(adev->dev, "(%d) failed to allocate kernel bo\n", r); >> + return r; >> } >> >> r = amdgpu_bo_reserve(*bo_ptr, false); >> @@ -288,27 +284,142 @@ int amdgpu_bo_create_reserved(struct amdgpu_device >> *adev, >> goto error_unpin; >> } >> >> + amdgpu_bo_unreserve(*bo_ptr); >> + return 0; >> + >> +error_unpin: >> + amdgpu_bo_unpin(*bo_ptr); >> +error_unreserve: >> + amdgpu_bo_unreserve(*bo_ptr); >> +error_free: >> + amdgpu_bo_unref(bo_ptr); >> + return r; >> +} >> + >> +/** >> + * amdgpu_bo_get_access - get CPU/GPU access to a BO >> + * >> + * @bo: the input BO, must be pinned >> + * @gpu_addr: optional, returns the GPU address of the BO >> + * @cpu_addr: optional, returns the CPU address of the BO >> + * >> + * Note: *bo must be already pinned! >> + * >> + * Returns: >> + * 0 on success, negative error code otherwise. >> + */ >> +static int amdgpu_bo_get_access(struct amdgpu_bo *bo, u64 *gpu_addr, >> + void **cpu_addr) >> +{ >> + struct amdgpu_device *adev; >> + int r; >> + >> + if (WARN_ON(!bo)) >> + return -EINVAL; >> + >> + adev = amdgpu_ttm_adev(bo->tbo.bdev); >> + >> + r = amdgpu_bo_reserve(bo, false); >> + if (r) { >> + dev_err(adev->dev, "(%d) failed to reserve kernel bo\n", r); >> + return r; >> + } >> + >> + /* >> + * The BO must already be pinned. A GPU or CPU address of an >> + * unpinned BO would become meaningless because TTM >> + * may then move or evict it at any time. >> + */ >> + if (WARN_ON_ONCE(!bo->tbo.pin_count)) { >> + r = -EINVAL; >> + goto error_unreserve; >> + } >> + >> if (gpu_addr) >> - *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr); >> + *gpu_addr = amdgpu_bo_gpu_offset(bo); >> >> if (cpu_addr) { >> - r = amdgpu_bo_kmap(*bo_ptr, cpu_addr); >> + r = amdgpu_bo_kmap(bo, cpu_addr); >> if (r) { >> dev_err(adev->dev, "(%d) kernel bo map failed\n", r); >> - goto error_unpin; >> + goto error_unreserve; >> } >> } >> >> + amdgpu_bo_unreserve(bo); >> return 0; >> >> -error_unpin: >> - amdgpu_bo_unpin(*bo_ptr); >> error_unreserve: >> - amdgpu_bo_unreserve(*bo_ptr); >> + amdgpu_bo_unreserve(bo); >> + return r; >> +} >> + >> +/** >> + * amdgpu_bo_create_reserved - create reserved BO for kernel use >> + * >> + * @adev: amdgpu device object >> + * @size: size for the new BO >> + * @align: alignment for the new BO >> + * @domain: where to place it >> + * @bo_ptr: used to initialize BOs in structures >> + * @gpu_addr: GPU addr of the pinned BO >> + * @cpu_addr: optional CPU address mapping >> + * >> + * Allocates and pins a BO for kernel internal use, and returns it still >> + * reserved. >> + * >> + * Note: For bo_ptr new BO is only created if bo_ptr points to NULL. An >> + * existing BO is only re-accessed (get CPU/GPU mapping) and not pinned >> again, >> + * so repeated calls with the same BO do not leak pin references. >> + * >> + * Returns: >> + * 0 on success, negative error code otherwise. >> + */ >> +int amdgpu_bo_create_reserved(struct amdgpu_device *adev, >> + unsigned long size, int align, >> + u32 domain, struct amdgpu_bo **bo_ptr, >> + u64 *gpu_addr, void **cpu_addr) >> +{ >> + bool created = false; >> + int r; >> + >> + if (WARN_ON(!bo_ptr)) >> + return -EINVAL; >> + >> + if (!size) { >> + amdgpu_bo_unref(bo_ptr); >> + return 0; >> + } >> + >> + if (!*bo_ptr) { >> + r = amdgpu_bo_create_pinned(adev, size, align, domain, bo_ptr, >> + !!cpu_addr); >> + if (r) >> + return r; >> + created = true; >> + } >> + >> + r = amdgpu_bo_get_access(*bo_ptr, gpu_addr, cpu_addr); >> + if (r) >> + goto error_free; >> + >> + r = amdgpu_bo_reserve(*bo_ptr, false); >> + if (r) { >> + dev_err(adev->dev, "(%d) failed to reserve kernel bo\n", r); >> + goto error_free; >> + } >> + >> + return 0; >> >> error_free: >> - if (free) >> + if (created) { >> + if (amdgpu_bo_reserve(*bo_ptr, true) == 0) { >> + amdgpu_bo_kunmap(*bo_ptr); >> + amdgpu_bo_unpin(*bo_ptr); >> + amdgpu_bo_unreserve(*bo_ptr); >> + } >> amdgpu_bo_unref(bo_ptr); >> + } >> >> return r; >> }
