On 02.09.25 15:31, Alex Deucher wrote:
> On Tue, Sep 2, 2025 at 9:27 AM Christian König <[email protected]>
> wrote:
>>
>> On 02.09.25 15:25, Alex Deucher wrote:
>>> On Tue, Sep 2, 2025 at 3:38 AM Christian König <[email protected]>
>>> wrote:
>>>>
>>>> On 02.09.25 05:29, Srinivasan Shanmugam wrote:
>>>>> Add mmio_remap bookkeeping to amdgpu_device and introduce
>>>>> amdgpu_ttm_mmio_remap_bo_init()/fini() to manage a kernel-owned,
>>>>> one-page (4K) BO in AMDGPU_GEM_DOMAIN_MMIO_REMAP.
>>>>>
>>>>> Bookkeeping:
>>>>> - adev->rmmio_remap.bo : kernel-owned singleton BO
>>>>>
>>>>> The BO is allocated during TTM init when a remap bus address is available
>>>>> (adev->rmmio_remap.bus_addr) and PAGE_SIZE <= AMDGPU_GPU_PAGE_SIZE (4K),
>>>>> and freed during TTM fini.
>>>>>
>>>>> v2:
>>>>> - Check mmio_remap bus address (adev->rmmio_remap.bus_addr) instead of
>>>>> rmmio_base. (Alex)
>>>>> - Skip quietly if PAGE_SIZE > AMDGPU_GPU_PAGE_SIZE or no bus address
>>>>> (no warn). (Alex)
>>>>> - Use `amdgpu_bo_create()` (not *_kernel) - Only with this The object
>>>>> is stored in adev->mmio_remap.bo and will later be exposed to
>>>>> userspace via a GEM handle. (Christian)
>>>>>
>>>>> v3:
>>>>> - Remove obvious comment before amdgpu_ttm_mmio_remap_bo_fini() call.
>>>>> (Alex)
>>>>>
>>>>> v4:
>>>>> - Squash bookkeeping into this patch
>>>>> - Place longer declaration first; clear bp via memset
>>>>> - Reserve + pin + kmap(+kunmap) the BO at init; unpin in fini
>>>>> (Christian)
>>>>>
>>>>> Suggested-by: Christian König <[email protected]>
>>>>> Suggested-by: Alex Deucher <[email protected]>
>>>>> Signed-off-by: Srinivasan Shanmugam <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 87 +++++++++++++++++++++++++
>>>>> 2 files changed, 88 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> index ddd472e56f69..24501d3fbefe 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> @@ -752,6 +752,7 @@ typedef void (*amdgpu_block_wreg_t)(struct
>>>>> amdgpu_device*, uint32_t, uint32_t, u
>>>>> struct amdgpu_mmio_remap {
>>>>> u32 reg_offset;
>>>>> resource_size_t bus_addr;
>>>>> + struct amdgpu_bo *bo;
>>>>> };
>>>>>
>>>>> /* Define the HW IP blocks will be used in driver , add more if
>>>>> necessary */
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> index 1a68ba17a62d..0d03e3a6f92d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> @@ -1854,6 +1854,87 @@ static void amdgpu_ttm_pools_fini(struct
>>>>> amdgpu_device *adev)
>>>>> adev->mman.ttm_pools = NULL;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * amdgpu_ttm_mmio_remap_bo_init - Allocate the singleton 4K MMIO_REMAP
>>>>> BO
>>>>> + * @adev: amdgpu device
>>>>> + *
>>>>> + * Allocates a one-page (4K) GEM BO in AMDGPU_GEM_DOMAIN_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).
>>>>> + *
>>>>> + * 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)
>>>>> +{
>>>>> + struct amdgpu_bo_param bp;
>>>>> + int r;
>>>>
>>>>> + void *kptr;
>>>>
>>>> kptr should potentially be saved in amdgpu_mmio_remap.
>>>>
>>>>> +
>>>>> + /* 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;
>>>>> +
>>>>> + 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.byte_align = AMDGPU_GPU_PAGE_SIZE;
>>>>> + bp.domain = AMDGPU_GEM_DOMAIN_MMIO_REMAP;
>>>>> + 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;
>>>>> +
>>>>> + r = amdgpu_bo_reserve(adev->rmmio_remap.bo, false);
>>>>
>>>> The last parameter should probably be true here.
>>>>
>>>>> + if (r)
>>>>> + goto err_unref;
>>>>> +
>>>>> + r = amdgpu_bo_pin(adev->rmmio_remap.bo,
>>>>> AMDGPU_GEM_DOMAIN_MMIO_REMAP);
>>>>> + if (r)
>>>>> + goto err_unres;
>>>>> +
>>>>> + r = amdgpu_bo_kmap(adev->rmmio_remap.bo, &kptr);
>>>
>>> Can't we just skip this? We don't need the CPU address in the kernel.
>>
>> I thought you suggested to use the remapped HDP registers for the HDP flush
>> in the kernel as well?
>>
>> If we don't want to do this we can just skip this.
>
> In the kernel we just use the existing mmio memory map via the WREG()
> macros. Using this other buffer would just complicate things.
Ok in this case I misunderstood you. @Srini please remove the kmap again.
Thanks,
Christian.
>
> Alex
>
>>
>> Christian.
>>
>>>
>>> Alex
>>>
>>>>> + if (r)
>>>>> + goto err_unpin;
>>>>> +
>>>>> + amdgpu_bo_kunmap(adev->rmmio_remap.bo);
>>>>> + amdgpu_bo_unreserve(adev->rmmio_remap.bo);
>>>>> + return 0;
>>>>> +
>>>>> +err_unpin:
>>>>> + amdgpu_bo_unpin(adev->rmmio_remap.bo);
>>>>> +err_unres:
>>>>> + amdgpu_bo_unreserve(adev->rmmio_remap.bo);
>>>>> +err_unref:
>>>>> + 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
>>>>> + * @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)
>>>>> +{
>>>>> + if (!amdgpu_bo_reserve(adev->rmmio_remap.bo, false)) {
>>>>
>>>> Same here.
>>>>
>>>> Apart from that looks good to me, feel free to add my rb.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> + amdgpu_bo_unpin(adev->rmmio_remap.bo);
>>>>> + amdgpu_bo_unreserve(adev->rmmio_remap.bo);
>>>>> + }
>>>>> + amdgpu_bo_unref(&adev->rmmio_remap.bo);
>>>>> + adev->rmmio_remap.bo = NULL;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * amdgpu_ttm_init - Init the memory management (ttm) as well as various
>>>>> * gtt/vram related fields.
>>>>> @@ -2028,6 +2109,11 @@ 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);
>>>>> + if (r)
>>>>> + return r;
>>>>> +
>>>>> /* Initialize preemptible memory pool */
>>>>> r = amdgpu_preempt_mgr_init(adev);
>>>>> if (r) {
>>>>> @@ -2091,6 +2177,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_fw_reserve_vram_fini(adev);
>>>>> amdgpu_ttm_drv_reserve_vram_fini(adev);
>>>>>
>>>>
>>
