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. 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; > }
