[AMD Official Use Only - AMD Internal Distribution Only] Hi Christian,
> -----Original Message----- > From: Christian König <[email protected]> > Sent: Tuesday, December 2, 2025 8:43 PM > To: [email protected]; SHANMUGAM, SRINIVASAN > <[email protected]>; Liu, Leo <[email protected]>; Dong, > Ruijing <[email protected]>; [email protected] > Subject: [RFC PATCH 1/3] drm/amdgpu: rework MMIO_REMAP BO creation > > First of all avoid using AMDGPU_GEM_DOMAIN_MMIO_REMAP in the TTM code. > > Then while at it remove some confusing comments, cleanup the comments who > make sense and rename the functions to be a bit more clear what they do. Mentioning Fixes-tag: would be helpful for references, I feel here, to get the continuity from where this was derived for all these series of patches. > > Signed-off-by: Christian König <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 66 ++++++++++++++----------- > 1 file changed, 38 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index e553cf411191..3166469d437a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1892,42 +1892,41 @@ static void amdgpu_ttm_pools_fini(struct > amdgpu_device *adev) } > > /** > - * amdgpu_ttm_mmio_remap_bo_init - Allocate the singleton 4K MMIO_REMAP > BO > + * amdgpu_ttm_mmio_remap_bo_init - Allocate the singleton MMIO_REMAP BO > * @adev: amdgpu device > * > - * Allocates a one-page (4K) GEM BO in > AMDGPU_GEM_DOMAIN_MMIO_REMAP when the > + * Allocates a global BO with backing AMDGPU_PL_MMIO_REMAP when the > * hardware exposes a remap base (adev->rmmio_remap.bus_addr) and the host > * PAGE_SIZE is <= AMDGPU_GPU_PAGE_SIZE (4K). The BO is created as a > regular > * GEM object (amdgpu_bo_create). > * > - * The BO is created as a normal GEM object via amdgpu_bo_create(), then > - * reserved and pinned at the TTM level (ttm_bo_pin()) so it can never be > - * migrated or evicted. No CPU mapping is established here. > - * > * Return: > * * 0 on success or intentional skip (feature not present/unsupported) > * * negative errno on allocation failure > */ > -static int amdgpu_ttm_mmio_remap_bo_init(struct amdgpu_device *adev) > +static int amdgpu_ttm_alloc_mmio_remap_bo(struct amdgpu_device *adev) > { > + struct ttm_operation_ctx ctx = { false, false }; > + struct ttm_placement placement; > + struct ttm_buffer_object *tbo; > + struct ttm_place placements; > struct amdgpu_bo_param bp; > + struct ttm_resource *tmp; > int r; > > /* Skip if HW doesn't expose remap, or if PAGE_SIZE > > AMDGPU_GPU_PAGE_SIZE (4K). */ > if (!adev->rmmio_remap.bus_addr || PAGE_SIZE > > AMDGPU_GPU_PAGE_SIZE) > return 0; > > + /* Allocate an empty BO without backing store */ the “empty BO without backing store” wording is a bit confusing here since amdgpu_bo_create() still assigns an initial TTM resource which we then replace with a AMDGPU_PL_MMIO_REMAP resource via amdgpu_ttm_alloc_mmio_remap_bo (). Maybe rephrase the comment to something like “allocate a BO and then move it to AMDGPU_PL_MMIO_REMAP” so the comment better matches the actual flow. > memset(&bp, 0, sizeof(bp)); > - > - /* Create exactly one GEM BO in the MMIO_REMAP domain. */ > - bp.type = ttm_bo_type_device; /* userspace-mappable GEM > */ > - bp.size = AMDGPU_GPU_PAGE_SIZE; /* 4K */ > + bp.type = ttm_bo_type_device; > + bp.size = AMDGPU_GPU_PAGE_SIZE; > bp.byte_align = AMDGPU_GPU_PAGE_SIZE; > - bp.domain = AMDGPU_GEM_DOMAIN_MMIO_REMAP; > + bp.domain = 0; > bp.flags = 0; > bp.resv = NULL; > bp.bo_ptr_size = sizeof(struct amdgpu_bo); > - > r = amdgpu_bo_create(adev, &bp, &adev->rmmio_remap.bo); > if (r) > return r; > @@ -1936,37 +1935,48 @@ static int amdgpu_ttm_mmio_remap_bo_init(struct > amdgpu_device *adev) > if (r) > goto err_unref; > > + tbo = &adev->rmmio_remap.bo->tbo; > + > /* > * MMIO_REMAP is a fixed I/O placement (AMDGPU_PL_MMIO_REMAP). > - * Use TTM-level pin so the BO cannot be evicted/migrated, > - * independent of GEM domains. This > - * enforces the “fixed I/O window” > */ > - ttm_bo_pin(&adev->rmmio_remap.bo->tbo); > + placement.num_placement = 1; > + placement.placement = &placements; > + placements.fpfn = 0; > + placements.lpfn = 0; > + placements.mem_type = AMDGPU_PL_MMIO_REMAP; > + placements.flags = 0; > + r = ttm_bo_mem_space(tbo, &placement, &tmp, &ctx); > + if (unlikely(r)) > + goto err_unlock; > + > + ttm_resource_free(tbo, &tbo->resource); > + ttm_bo_assign_mem(tbo, tmp); > + ttm_bo_pin(tbo); > > amdgpu_bo_unreserve(adev->rmmio_remap.bo); > return 0; > > +err_unlock: > + amdgpu_bo_unreserve(adev->rmmio_remap.bo); > + > err_unref: > - if (adev->rmmio_remap.bo) > - amdgpu_bo_unref(&adev->rmmio_remap.bo); > + amdgpu_bo_unref(&adev->rmmio_remap.bo); > adev->rmmio_remap.bo = NULL; > return r; > } > > /** > - * amdgpu_ttm_mmio_remap_bo_fini - Free the singleton MMIO_REMAP BO > + * amdgpu_ttm_free_mmio_remap_bo - Free the singleton MMIO_REMAP BO > * @adev: amdgpu device > * > * Frees the kernel-owned MMIO_REMAP BO if it was allocated by > * amdgpu_ttm_mmio_remap_bo_init(). > */ > -static void amdgpu_ttm_mmio_remap_bo_fini(struct amdgpu_device *adev) > +static void amdgpu_ttm_free_mmio_remap_bo(struct amdgpu_device *adev) > { > - struct amdgpu_bo *bo = adev->rmmio_remap.bo; > - > - if (!bo) > - return; /* <-- safest early exit */ > + if (!adev->rmmio_remap.bo) > + return; > > if (!amdgpu_bo_reserve(adev->rmmio_remap.bo, true)) { > ttm_bo_unpin(&adev->rmmio_remap.bo->tbo); > @@ -2152,8 +2162,8 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) > return r; > } > > - /* Allocate the singleton MMIO_REMAP BO (4K) if supported */ > - r = amdgpu_ttm_mmio_remap_bo_init(adev); > + /* Allocate the singleton MMIO_REMAP BO if supported */ > + r = amdgpu_ttm_alloc_mmio_remap_bo(adev); > if (r) > return r; > > @@ -2220,7 +2230,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev) > amdgpu_bo_free_kernel(&adev->mman.sdma_access_bo, NULL, > &adev->mman.sdma_access_ptr); > > - amdgpu_ttm_mmio_remap_bo_fini(adev); > + amdgpu_ttm_free_mmio_remap_bo(adev); Might be worth a short comment around amdgpu_ttm_free_mmio_remap_bo() or its call in amdgpu_ttm_fini() that we rely on the usual DRM teardown ordering (no more user ioctls once we start TTM fini), so adev->rmmio_remap.bo can’t be accessed via OPEN_GLOBAL any more at this point. That makes the lifetime assumptions of the global BO explicit. > amdgpu_ttm_fw_reserve_vram_fini(adev); > amdgpu_ttm_drv_reserve_vram_fini(adev); > > -- > 2.43.0
