[PATCH] drm/amdkfd: Set pte_flags for actual BO location
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Aug 30 06:00:38 UTC 2022
Am 29.08.22 um 21:30 schrieb Felix Kuehling:
>
> Am 2022-08-29 um 14:59 schrieb Christian König:
>> Am 29.08.22 um 18:07 schrieb Felix Kuehling:
>>> Am 2022-08-29 um 11:38 schrieb Christian König:
>>>> Am 27.08.22 um 01:16 schrieb Felix Kuehling:
>>>>> BOs can be in a different location than was intended at allocation
>>>>> time,
>>>>> for example when restoring fails after an eviction or BOs get
>>>>> pinned in
>>>>> system memory. On some GPUs the MTYPE for coherent mappings
>>>>> depends on
>>>>> the actual memory location.
>>>>>
>>>>> Use the actual location to determine the pte_flags every time the
>>>>> page
>>>>> tables are updated.
>>>>
>>>> For a workaround ok, but looks a bit awkward. Basically we need
>>>> different MTYPE based on the location, right?
>>>
>>> Yes. On Aldebaran and Arcturus we need different MTYPEs for
>>> fine-grained coherence depending on the location.
>>
>> It would be much cleaner to have a better description how all this
>> came to be in the mapping.
>>
>> E.g. that we generate the flags in the VM code based on the
>> requirements described in the mapping.
>>
>> Do we have time to clean this up thoughtfully?
>
> Currently we have two places in the KFD code that generates the
> mapping flags. I was planning to eliminate the duplication. I think
> you're suggesting moving it into the amdgpu_vm code instead.
> Unfortunately it's somewhat GPU-specific. So you probably won't like
> this code in the general amdgpu_vm code. Maybe the HW-specific part
> should be in gmc_v*.c.
We have the gmc_v*_get_vm_pte() and gmc_v*_get_vm_pde() functions
exactly for that.
>
> The requirements would include:
>
> * Memory type and mapping (local, system, PCIe P2P, XGMI P2P)
> * Memory model (coarse-grained or fine-grained coherence)
Question is if any of this is a per BO or per mapping information?
For the gfx side we unfortunately have put the MTYPE into the mapping
(which turned out to be a bad idea).
So as far as I understand it the MTYPE should rather be obtained from
per buffer flags and the current placement, correct?
Regards,
Christian.
>
> Regards,
> Felix
>
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> Felix
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>>> ---
>>>>> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 9 ++++++++-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 19
>>>>> +++++++++++++++++++
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>>>>> 3 files changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> index cbd593f7d553..5dd89f5a032f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> @@ -405,6 +405,7 @@ static int vm_update_pds(struct amdgpu_vm *vm,
>>>>> struct amdgpu_sync *sync)
>>>>> static uint64_t get_pte_flags(struct amdgpu_device *adev, struct
>>>>> kgd_mem *mem)
>>>>> {
>>>>> struct amdgpu_device *bo_adev =
>>>>> amdgpu_ttm_adev(mem->bo->tbo.bdev);
>>>>> + bool is_vram = mem->bo->tbo.resource->mem_type == TTM_PL_VRAM;
>>>>> bool coherent = mem->alloc_flags &
>>>>> KFD_IOC_ALLOC_MEM_FLAGS_COHERENT;
>>>>> bool uncached = mem->alloc_flags &
>>>>> KFD_IOC_ALLOC_MEM_FLAGS_UNCACHED;
>>>>> uint32_t mapping_flags;
>>>>> @@ -420,7 +421,7 @@ static uint64_t get_pte_flags(struct
>>>>> amdgpu_device *adev, struct kgd_mem *mem)
>>>>> switch (adev->asic_type) {
>>>>> case CHIP_ARCTURUS:
>>>>> case CHIP_ALDEBARAN:
>>>>> - if (mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) {
>>>>> + if (is_vram) {
>>>>> if (bo_adev == adev) {
>>>>> if (uncached)
>>>>> mapping_flags |= AMDGPU_VM_MTYPE_UC;
>>>>> @@ -1236,12 +1237,18 @@ static int update_gpuvm_pte(struct kgd_mem
>>>>> *mem,
>>>>> {
>>>>> struct amdgpu_bo_va *bo_va = entry->bo_va;
>>>>> struct amdgpu_device *adev = entry->adev;
>>>>> + uint64_t pte_flags = get_pte_flags(adev, mem);
>>>>> int ret;
>>>>> ret = kfd_mem_dmamap_attachment(mem, entry);
>>>>> if (ret)
>>>>> return ret;
>>>>> + if (unlikely(entry->pte_flags != pte_flags)) {
>>>>> + amdgpu_vm_bo_update_flags(bo_va, pte_flags);
>>>>> + entry->pte_flags = pte_flags;
>>>>> + }
>>>>> +
>>>>> /* Update the page tables */
>>>>> ret = amdgpu_vm_bo_update(adev, bo_va, false);
>>>>> if (ret) {
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 59cac347baa3..954a40d5d828 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1862,6 +1862,25 @@ void amdgpu_vm_bo_invalidate(struct
>>>>> amdgpu_device *adev,
>>>>> }
>>>>> }
>>>>> +/**
>>>>> + * amdgpu_vm_bo_update_flags - Update mapping flags of invalid
>>>>> mappings
>>>>> + *
>>>>> + * @bo_va: identifies the BO and VM
>>>>> + * @flags: new mapping flags
>>>>> + *
>>>>> + * The update is only applied to invalid mappings. This allows
>>>>> updating the
>>>>> + * mapping flags after a migration to maintain the desired
>>>>> coherence. The next
>>>>> + * call to amdgpu_vm_bo_update() will apply the new @flags to the
>>>>> page table.
>>>>> + */
>>>>> +void amdgpu_vm_bo_update_flags(struct amdgpu_bo_va *bo_va,
>>>>> + uint64_t flags)
>>>>> +{
>>>>> + struct amdgpu_bo_va_mapping *mapping;
>>>>> +
>>>>> + list_for_each_entry(mapping, &bo_va->invalids, list)
>>>>> + mapping->flags = flags;
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * amdgpu_vm_get_block_size - calculate VM page table size as
>>>>> power of two
>>>>> *
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 9ecb7f663e19..11793716cd8b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -413,6 +413,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>>> *adev,
>>>>> bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
>>>>> void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>>>>> struct amdgpu_bo *bo, bool evicted);
>>>>> +void amdgpu_vm_bo_update_flags(struct amdgpu_bo_va *bo_va,
>>>>> uint64_t flags);
>>>>> uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr,
>>>>> uint64_t addr);
>>>>> struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>>>>> struct amdgpu_bo *bo);
>>>>
>>
More information about the amd-gfx
mailing list