Re: [PATCH 3/6] drm/amdgpu: allocate VM PDs/PTs on demand

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I find that it's related to CPU page table updates. If I force page 
table updates with SDMA, I don't get the VM fault.

Regards,
   Felix

On 2019-03-11 12:55 p.m., Christian König wrote:
> Hi guys,
>
> well it's most likely some missing handling in the KFD, so I'm rather 
> reluctant to revert the change immediately.
>
> Problem is that I don't have time right now to look into it 
> immediately. So Kent can you continue to take a look?
>
> Sounds like its crashing immediately, so it should be something obvious.
>
> Christian.
>
> Am 11.03.19 um 10:49 schrieb Russell, Kent:
>>  From what I've been able to dig through, the VM Fault seems to occur 
>> right after a doorbell mmap, but that's as far as I got. I can try to 
>> revert it in today's merge and see how things go.
>>
>>   Kent
>>
>>> -----Original Message-----
>>> From: Kuehling, Felix
>>> Sent: Friday, March 08, 2019 11:16 PM
>>> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Russell, Kent
>>> <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> Subject: RE: [PATCH 3/6] drm/amdgpu: allocate VM PDs/PTs on demand
>>>
>>> My concerns were related to eviction fence handing. It would 
>>> manifest by
>>> unnecessary eviction callbacks into KFD that aren't cause by real 
>>> evictions. I
>>> addressed that with a previous patch series that removed the need to
>>> remove eviction fences and add them back around page table updates in
>>> amdgpu_amdkfd_gpuvm.c.
>>>
>>> I don't know what's going on here. I can probably take a look on 
>>> Monday. I
>>> haven't considered what changed with respect to PD updates.
>>>
>>> Kent, can we temporarily revert the offending change in amd-kfd-staging
>>> just to unblock the merge?
>>>
>>> Christian, I think KFD is currently broken on amd-staging-drm-next. 
>>> If we're
>>> serious about supporting KFD upstream, you may also want to consider
>>> reverting your change there for now. Also consider building the 
>>> Thunk and
>>> kfdtest so you can do quick smoke tests locally whenever you make
>>> amdgpu_vm changes that can affect KFD.
>>> https://github.com/RadeonOpenCompute/ROCT-Thunk-Interface
>>>
>>> Regards,
>>>    Felix
>>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
>>> Christian König
>>> Sent: Friday, March 08, 2019 9:14 AM
>>> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> Subject: Re: [PATCH 3/6] drm/amdgpu: allocate VM PDs/PTs on demand
>>>
>>> My best guess is that we forget somewhere to update the PDs. What
>>> hardware is that on?
>>>
>>> Felix already mentioned that this could be problematic for the KFD.
>>>
>>> Maybe he has an idea,
>>> Christian.
>>>
>>> Am 08.03.19 um 15:04 schrieb Russell, Kent:
>>>> Hi Christian,
>>>>
>>>> This patch ended up causing a VM Fault in KFDTest. Reverting just this
>>> patch addressed the issue:
>>>> [   82.703503] amdgpu 0000:0c:00.0: GPU fault detected: 146 
>>>> 0x0000480c for
>>> process  pid 0 thread  pid 0
>>>> [   82.703512] amdgpu 0000:0c:00.0:
>>> VM_CONTEXT1_PROTECTION_FAULT_ADDR   0x00001000
>>>> [   82.703516] amdgpu 0000:0c:00.0:
>>> VM_CONTEXT1_PROTECTION_FAULT_STATUS 0x1004800C
>>>> [   82.703522] amdgpu 0000:0c:00.0: VM fault (0x0c, vmid 8, pasid 
>>>> 32769) at
>>> page 4096, read from 'TC0' (0x54433000) (72)
>>>> [   82.703585] Evicting PASID 32769 queues
>>>>
>>>> I am looking into it, but if you have any insight that would be 
>>>> great in
>>> helping to resolve it quickly.
>>>>    Kent
>>>>> -----Original Message-----
>>>>> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
>>>>> Christian König
>>>>> Sent: Tuesday, February 26, 2019 7:47 AM
>>>>> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>>> Subject: [PATCH 3/6] drm/amdgpu: allocate VM PDs/PTs on demand
>>>>>
>>>>> Let's start to allocate VM PDs/PTs on demand instead of
>>>>> pre-allocating them during mapping.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
>>>>> ---
>>>>>    .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  10 +-
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c       |   9 --
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       |  10 --
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 136 
>>>>> +++++------------
>>> -
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 -
>>>>>    5 files changed, 39 insertions(+), 129 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> index 31e3953dcb6e..088e9b6b765b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> @@ -410,15 +410,7 @@ static int add_bo_to_vm(struct amdgpu_device
>>>>> *adev, struct kgd_mem *mem,
>>>>>        if (p_bo_va_entry)
>>>>>            *p_bo_va_entry = bo_va_entry;
>>>>>
>>>>> -    /* Allocate new page tables if needed and validate
>>>>> -     * them.
>>>>> -     */
>>>>> -    ret = amdgpu_vm_alloc_pts(adev, vm, va, amdgpu_bo_size(bo));
>>>>> -    if (ret) {
>>>>> -        pr_err("Failed to allocate pts, err=%d\n", ret);
>>>>> -        goto err_alloc_pts;
>>>>> -    }
>>>>> -
>>>>> +    /* Allocate validate page tables if needed */
>>>>>        ret = vm_validate_pt_pd_bos(vm);
>>>>>        if (ret) {
>>>>>            pr_err("validate_pt_pd_bos() failed\n"); diff --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> index 7e22be7ca68a..54dd02a898b9 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
>>>>> @@ -92,15 +92,6 @@ int amdgpu_map_static_csa(struct amdgpu_device
>>>>> *adev, struct amdgpu_vm *vm,
>>>>>            return -ENOMEM;
>>>>>        }
>>>>>
>>>>> -    r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, csa_addr,
>>>>> -                size);
>>>>> -    if (r) {
>>>>> -        DRM_ERROR("failed to allocate pts for static CSA, err=%d\n",
>>>>> r);
>>>>> -        amdgpu_vm_bo_rmv(adev, *bo_va);
>>>>> -        ttm_eu_backoff_reservation(&ticket, &list);
>>>>> -        return r;
>>>>> -    }
>>>>> -
>>>>>        r = amdgpu_vm_bo_map(adev, *bo_va, csa_addr, 0, size,
>>>>>                     AMDGPU_PTE_READABLE |
>>>>> AMDGPU_PTE_WRITEABLE |
>>>>>                     AMDGPU_PTE_EXECUTABLE);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 555285e329ed..fcaaac30e84b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -625,11 +625,6 @@ int amdgpu_gem_va_ioctl(struct drm_device
>>> *dev,
>>>>> void *data,
>>>>>
>>>>>        switch (args->operation) {
>>>>>        case AMDGPU_VA_OP_MAP:
>>>>> -        r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args-
>>>>>> va_address,
>>>>> -                    args->map_size);
>>>>> -        if (r)
>>>>> -            goto error_backoff;
>>>>> -
>>>>>            va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>            r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
>>>>>                         args->offset_in_bo, args->map_size, @@ -
>>>>> 645,11 +640,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>> void
>>>>> *data,
>>>>>                            args->map_size);
>>>>>            break;
>>>>>        case AMDGPU_VA_OP_REPLACE:
>>>>> -        r = amdgpu_vm_alloc_pts(adev, bo_va->base.vm, args-
>>>>>> va_address,
>>>>> -                    args->map_size);
>>>>> -        if (r)
>>>>> -            goto error_backoff;
>>>>> -
>>>>>            va_flags = amdgpu_gmc_get_pte_flags(adev, args->flags);
>>>>>            r = amdgpu_vm_bo_replace_map(adev, bo_va, args-
>>>>>> va_address,
>>>>>                             args->offset_in_bo, args-
>>>>>> map_size, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 362436f4e856..dfad543fc000 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -504,47 +504,6 @@ static void amdgpu_vm_pt_next(struct
>>>>> amdgpu_device *adev,
>>>>>        }
>>>>>    }
>>>>>
>>>>> -/**
>>>>> - * amdgpu_vm_pt_first_leaf - get first leaf PD/PT
>>>>> - *
>>>>> - * @adev: amdgpu_device pointer
>>>>> - * @vm: amdgpu_vm structure
>>>>> - * @start: start addr of the walk
>>>>> - * @cursor: state to initialize
>>>>> - *
>>>>> - * Start a walk and go directly to the leaf node.
>>>>> - */
>>>>> -static void amdgpu_vm_pt_first_leaf(struct amdgpu_device *adev,
>>>>> -                    struct amdgpu_vm *vm, uint64_t start,
>>>>> -                    struct amdgpu_vm_pt_cursor *cursor)
>>>>> -{
>>>>> -    amdgpu_vm_pt_start(adev, vm, start, cursor);
>>>>> -    while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>> -}
>>>>> -
>>>>> -/**
>>>>> - * amdgpu_vm_pt_next_leaf - get next leaf PD/PT
>>>>> - *
>>>>> - * @adev: amdgpu_device pointer
>>>>> - * @cursor: current state
>>>>> - *
>>>>> - * Walk the PD/PT tree to the next leaf node.
>>>>> - */
>>>>> -static void amdgpu_vm_pt_next_leaf(struct amdgpu_device *adev,
>>>>> -                   struct amdgpu_vm_pt_cursor *cursor)
>>>>> -{
>>>>> -    amdgpu_vm_pt_next(adev, cursor);
>>>>> -    if (cursor->pfn != ~0ll)
>>>>> -        while (amdgpu_vm_pt_descendant(adev, cursor));
>>>>> -}
>>>>> -
>>>>> -/**
>>>>> - * for_each_amdgpu_vm_pt_leaf - walk over all leaf PDs/PTs in the
>>>>> hierarchy
>>>>> - */
>>>>> -#define for_each_amdgpu_vm_pt_leaf(adev, vm, start, end, cursor)
>>>>>     \
>>>>> -    for (amdgpu_vm_pt_first_leaf((adev), (vm), (start), &(cursor));
>>>>>         \
>>>>> -         (cursor).pfn <= end; amdgpu_vm_pt_next_leaf((adev),
>>>>> &(cursor)))
>>>>> -
>>>>>    /**
>>>>>     * amdgpu_vm_pt_first_dfs - start a deep first search
>>>>>     *
>>>>> @@ -915,74 +874,51 @@ static void amdgpu_vm_bo_param(struct
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>     * Returns:
>>>>>     * 0 on success, errno otherwise.
>>>>>     */
>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> -            struct amdgpu_vm *vm,
>>>>> -            uint64_t saddr, uint64_t size)
>>>>> +static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> +                   struct amdgpu_vm *vm,
>>>>> +                   struct amdgpu_vm_pt_cursor *cursor)
>>>>>    {
>>>>> -    struct amdgpu_vm_pt_cursor cursor;
>>>>> +    struct amdgpu_vm_pt *entry = cursor->entry;
>>>>> +    struct amdgpu_bo_param bp;
>>>>>        struct amdgpu_bo *pt;
>>>>> -    uint64_t eaddr;
>>>>>        int r;
>>>>>
>>>>> -    /* validate the parameters */
>>>>> -    if (saddr & AMDGPU_GPU_PAGE_MASK || size &
>>>>> AMDGPU_GPU_PAGE_MASK)
>>>>> -        return -EINVAL;
>>>>> +    if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>>>> +        unsigned num_entries;
>>>>>
>>>>> -    eaddr = saddr + size - 1;
>>>>> -
>>>>> -    saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>> -    eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>>> -
>>>>> -    if (eaddr >= adev->vm_manager.max_pfn) {
>>>>> -        dev_err(adev->dev, "va above limit (0x%08llX >=
>>>>> 0x%08llX)\n",
>>>>> -            eaddr, adev->vm_manager.max_pfn);
>>>>> -        return -EINVAL;
>>>>> +        num_entries = amdgpu_vm_num_entries(adev, cursor-
>>>>>> level);
>>>>> +        entry->entries = kvmalloc_array(num_entries,
>>>>> +                        sizeof(*entry->entries),
>>>>> +                        GFP_KERNEL | __GFP_ZERO);
>>>>> +        if (!entry->entries)
>>>>> +            return -ENOMEM;
>>>>>        }
>>>>>
>>>>> -    for_each_amdgpu_vm_pt_leaf(adev, vm, saddr, eaddr, cursor) {
>>>>> -        struct amdgpu_vm_pt *entry = cursor.entry;
>>>>> -        struct amdgpu_bo_param bp;
>>>>> -
>>>>> -        if (cursor.level < AMDGPU_VM_PTB) {
>>>>> -            unsigned num_entries;
>>>>> -
>>>>> -            num_entries = amdgpu_vm_num_entries(adev,
>>>>> cursor.level);
>>>>> -            entry->entries = kvmalloc_array(num_entries,
>>>>> -                            sizeof(*entry-
>>>>>> entries),
>>>>> -                            GFP_KERNEL |
>>>>> -                            __GFP_ZERO);
>>>>> -            if (!entry->entries)
>>>>> -                return -ENOMEM;
>>>>> -        }
>>>>> -
>>>>> -
>>>>> -        if (entry->base.bo)
>>>>> -            continue;
>>>>> -
>>>>> -        amdgpu_vm_bo_param(adev, vm, cursor.level, &bp);
>>>>> -
>>>>> -        r = amdgpu_bo_create(adev, &bp, &pt);
>>>>> -        if (r)
>>>>> -            return r;
>>>>> -
>>>>> -        if (vm->use_cpu_for_update) {
>>>>> -            r = amdgpu_bo_kmap(pt, NULL);
>>>>> -            if (r)
>>>>> -                goto error_free_pt;
>>>>> -        }
>>>>> +    if (entry->base.bo)
>>>>> +        return 0;
>>>>>
>>>>> -        /* Keep a reference to the root directory to avoid
>>>>> -        * freeing them up in the wrong order.
>>>>> -        */
>>>>> -        pt->parent = amdgpu_bo_ref(cursor.parent->base.bo);
>>>>> +    amdgpu_vm_bo_param(adev, vm, cursor->level, &bp);
>>>>>
>>>>> -        amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>> +    r = amdgpu_bo_create(adev, &bp, &pt);
>>>>> +    if (r)
>>>>> +        return r;
>>>>>
>>>>> -        r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>> +    if (vm->use_cpu_for_update) {
>>>>> +        r = amdgpu_bo_kmap(pt, NULL);
>>>>>            if (r)
>>>>>                goto error_free_pt;
>>>>>        }
>>>>>
>>>>> +    /* Keep a reference to the root directory to avoid
>>>>> +     * freeing them up in the wrong order.
>>>>> +     */
>>>>> +    pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>>> +    amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>>>>> +
>>>>> +    r = amdgpu_vm_clear_bo(adev, vm, pt);
>>>>> +    if (r)
>>>>> +        goto error_free_pt;
>>>>> +
>>>>>        return 0;
>>>>>
>>>>>    error_free_pt:
>>>>> @@ -1627,6 +1563,7 @@ static int amdgpu_vm_update_ptes(struct
>>>>> amdgpu_pte_update_params *params,
>>>>>        struct amdgpu_vm_pt_cursor cursor;
>>>>>        uint64_t frag_start = start, frag_end;
>>>>>        unsigned int frag;
>>>>> +    int r;
>>>>>
>>>>>        /* figure out the initial fragment */
>>>>>        amdgpu_vm_fragment(params, frag_start, end, flags, &frag,
>>>>> &frag_end); @@ -1634,12 +1571,15 @@ static int
>>>>> amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>>>>        /* walk over the address space and update the PTs */
>>>>>        amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>>>>        while (cursor.pfn < end) {
>>>>> -        struct amdgpu_bo *pt = cursor.entry->base.bo;
>>>>>            unsigned shift, parent_shift, mask;
>>>>>            uint64_t incr, entry_end, pe_start;
>>>>> +        struct amdgpu_bo *pt;
>>>>>
>>>>> -        if (!pt)
>>>>> -            return -ENOENT;
>>>>> +        r = amdgpu_vm_alloc_pts(params->adev, params->vm,
>>>>> &cursor);
>>>>> +        if (r)
>>>>> +            return r;
>>>>> +
>>>>> +        pt = cursor.entry->base.bo;
>>>>>
>>>>>            /* The root level can't be a huge page */
>>>>>            if (cursor.level == adev->vm_manager.root_level) { diff 
>>>>> --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 81ff8177f092..116605c038d2 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -303,9 +303,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>> int
>>>>> amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct
>>>>> amdgpu_vm *vm,
>>>>>                      int (*callback)(void *p, struct amdgpu_bo *bo),
>>>>>                      void *param);
>>>>> -int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>>>>> -            struct amdgpu_vm *vm,
>>>>> -            uint64_t saddr, uint64_t size);
>>>>>    int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job
>>>>> *job, bool need_pipe_sync);  int amdgpu_vm_update_directories(struct
>>>>> amdgpu_device *adev,
>>>>>                     struct amdgpu_vm *vm);
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux