AMD General Gentle ping
-----Original Message----- From: Zhu, Lingshan Sent: Tuesday, June 23, 2026 12:34 PM To: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]> Cc: [email protected]; Huang, Ray <[email protected]> Subject: RE: [PATCH] drm/amdgpu: split amdgpu_bo_create_reserved into create and map helpers ping -----Original Message----- From: Zhu, Lingshan <[email protected]> Sent: Wednesday, June 17, 2026 2:55 PM To: Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]> Cc: [email protected]; Huang, Ray <[email protected]>; Zhu, Lingshan <[email protected]> Subject: [PATCH] drm/amdgpu: split amdgpu_bo_create_reserved into create and map helpers 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. 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. - 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; } -- 2.54.0
