[PATCH] drm/amdgpu: use a 64bit interval tree for VM management

Felix Kuehling felix.kuehling at amd.com
Fri Mar 31 19:55:40 UTC 2017


On 17-03-31 03:15 AM, Christian König wrote:
> Am 30.03.2017 um 16:55 schrieb Felix Kuehling:
>> This only makes a difference for 32-bit systems. The idea is to have a
>> fixed virtual address space size with 4-level page tables and to
>> minimize differences between 32 and 64-bit systems.
> Thanks going to use that as commit message.
>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
>
> BTW: What do you think about changing the code now to use full
> addresses instead of pfn and exclusive instead of inclusive ranges?

I don't know. It would require fairly wide-spread changes. I'd be
worried about regressions because of missing something. Those pesky
off-by-one errors are easy to miss and can linger in the code undetected
for a long time. Are there potential benefits that outweigh the
potential pain?

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>>
>> On 17-03-30 08:23 AM, Christian König wrote:
>>> From: Christian König <christian.koenig at amd.com>
>>>
>>> Starting with Vega10 we have a 48bit VA, so the pfn won't
>>> fit into 32bits any more.
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  7 ++-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c    | 12 ++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c    |  1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 12 ++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c   |  4 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c   |  4 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    | 93
>>> ++++++++++++++++---------------
>>>   7 files changed, 71 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 3115cdc..86fba1a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -32,7 +32,7 @@
>>>   #include <linux/wait.h>
>>>   #include <linux/list.h>
>>>   #include <linux/kref.h>
>>> -#include <linux/interval_tree.h>
>>> +#include <linux/rbtree.h>
>>>   #include <linux/hashtable.h>
>>>   #include <linux/fence.h>
>>>   @@ -381,7 +381,10 @@ struct amdgpu_bo_list_entry {
>>>     struct amdgpu_bo_va_mapping {
>>>       struct list_head        list;
>>> -    struct interval_tree_node    it;
>>> +    struct rb_node            rb;
>>> +    uint64_t            start;
>>> +    uint64_t            last;
>>> +    uint64_t            __subtree_last;
>>>       uint64_t            offset;
>>>       uint64_t            flags;
>>>   };
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index dde1ecf..3389f1b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -949,7 +949,7 @@ static int amdgpu_cs_ib_fill(struct
>>> amdgpu_device *adev,
>>>               }
>>>                 if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> +                (m->last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>                   DRM_ERROR("IB va_start+ib_bytes is invalid\n");
>>>                   return -EINVAL;
>>>               }
>>> @@ -960,7 +960,7 @@ static int amdgpu_cs_ib_fill(struct
>>> amdgpu_device *adev,
>>>                   return r;
>>>               }
>>>   -            offset = ((uint64_t)m->it.start) * AMDGPU_GPU_PAGE_SIZE;
>>> +            offset = m->start * AMDGPU_GPU_PAGE_SIZE;
>>>               kptr += chunk_ib->va_start - offset;
>>>                 r =  amdgpu_ib_get(adev, vm, chunk_ib->ib_bytes, ib);
>>> @@ -1388,8 +1388,8 @@ amdgpu_cs_find_mapping(struct amdgpu_cs_parser
>>> *parser,
>>>               continue;
>>>             list_for_each_entry(mapping, &lobj->bo_va->valids, list) {
>>> -            if (mapping->it.start > addr ||
>>> -                addr > mapping->it.last)
>>> +            if (mapping->start > addr ||
>>> +                addr > mapping->last)
>>>                   continue;
>>>                 *bo = lobj->bo_va->bo;
>>> @@ -1397,8 +1397,8 @@ amdgpu_cs_find_mapping(struct amdgpu_cs_parser
>>> *parser,
>>>           }
>>>             list_for_each_entry(mapping, &lobj->bo_va->invalids,
>>> list) {
>>> -            if (mapping->it.start > addr ||
>>> -                addr > mapping->it.last)
>>> +            if (mapping->start > addr ||
>>> +                addr > mapping->last)
>>>                   continue;
>>>                 *bo = lobj->bo_va->bo;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> index 7ea3cac..38f739f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> @@ -31,6 +31,7 @@
>>>   #include <linux/firmware.h>
>>>   #include <linux/module.h>
>>>   #include <linux/mmu_notifier.h>
>>> +#include <linux/interval_tree.h>
>>>   #include <drm/drmP.h>
>>>   #include <drm/drm.h>
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> index f38e5e2..381b770 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> @@ -226,8 +226,8 @@ TRACE_EVENT(amdgpu_vm_bo_map,
>>>             TP_fast_assign(
>>>                  __entry->bo = bo_va ? bo_va->bo : NULL;
>>> -               __entry->start = mapping->it.start;
>>> -               __entry->last = mapping->it.last;
>>> +               __entry->start = mapping->start;
>>> +               __entry->last = mapping->last;
>>>                  __entry->offset = mapping->offset;
>>>                  __entry->flags = mapping->flags;
>>>                  ),
>>> @@ -250,8 +250,8 @@ TRACE_EVENT(amdgpu_vm_bo_unmap,
>>>             TP_fast_assign(
>>>                  __entry->bo = bo_va->bo;
>>> -               __entry->start = mapping->it.start;
>>> -               __entry->last = mapping->it.last;
>>> +               __entry->start = mapping->start;
>>> +               __entry->last = mapping->last;
>>>                  __entry->offset = mapping->offset;
>>>                  __entry->flags = mapping->flags;
>>>                  ),
>>> @@ -270,8 +270,8 @@ DECLARE_EVENT_CLASS(amdgpu_vm_mapping,
>>>                    ),
>>>             TP_fast_assign(
>>> -               __entry->soffset = mapping->it.start;
>>> -               __entry->eoffset = mapping->it.last + 1;
>>> +               __entry->soffset = mapping->start;
>>> +               __entry->eoffset = mapping->last + 1;
>>>                  __entry->flags = mapping->flags;
>>>                  ),
>>>           TP_printk("soffs=%010llx, eoffs=%010llx, flags=%08x",
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> index e1a838e..ff8ae50 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>>> @@ -741,10 +741,10 @@ static int amdgpu_uvd_cs_pass2(struct
>>> amdgpu_uvd_cs_ctx *ctx)
>>>         start = amdgpu_bo_gpu_offset(bo);
>>>   -    end = (mapping->it.last + 1 - mapping->it.start);
>>> +    end = (mapping->last + 1 - mapping->start);
>>>       end = end * AMDGPU_GPU_PAGE_SIZE + start;
>>>   -    addr -= ((uint64_t)mapping->it.start) * AMDGPU_GPU_PAGE_SIZE;
>>> +    addr -= mapping->start * AMDGPU_GPU_PAGE_SIZE;
>>>       start += addr;
>>>         amdgpu_set_ib_value(ctx->parser, ctx->ib_idx, ctx->data0,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> index eccd70a..e43c83f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> @@ -595,13 +595,13 @@ static int amdgpu_vce_cs_reloc(struct
>>> amdgpu_cs_parser *p, uint32_t ib_idx,
>>>       }
>>>         if ((addr + (uint64_t)size) >
>>> -        ((uint64_t)mapping->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> +        (mapping->last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>           DRM_ERROR("BO to small for addr 0x%010Lx %d %d\n",
>>>                 addr, lo, hi);
>>>           return -EINVAL;
>>>       }
>>>   -    addr -= ((uint64_t)mapping->it.start) * AMDGPU_GPU_PAGE_SIZE;
>>> +    addr -= mapping->start * AMDGPU_GPU_PAGE_SIZE;
>>>       addr += amdgpu_bo_gpu_offset(bo);
>>>       addr -= ((uint64_t)size) * ((uint64_t)index);
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index c11d15e..43adc4b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -26,6 +26,7 @@
>>>    *          Jerome Glisse
>>>    */
>>>   #include <linux/fence-array.h>
>>> +#include <linux/interval_tree_generic.h>
>>>   #include <drm/drmP.h>
>>>   #include <drm/amdgpu_drm.h>
>>>   #include "amdgpu.h"
>>> @@ -51,6 +52,15 @@
>>>    * SI supports 16.
>>>    */
>>>   +#define START(node) ((node)->start)
>>> +#define LAST(node) ((node)->last)
>>> +
>>> +INTERVAL_TREE_DEFINE(struct amdgpu_bo_va_mapping, rb, uint64_t,
>>> __subtree_last,
>>> +             START, LAST, static, amdgpu_vm_it)
>>> +
>>> +#undef START
>>> +#undef LAST
>>> +
>>>   /* Local structure. Encapsulate some VM table update parameters to
>>> reduce
>>>    * the number of function parameters
>>>    */
>>> @@ -1301,7 +1311,7 @@ static int amdgpu_vm_bo_split_mapping(struct
>>> amdgpu_device *adev,
>>>                         struct drm_mm_node *nodes,
>>>                         struct fence **fence)
>>>   {
>>> -    uint64_t pfn, src = 0, start = mapping->it.start;
>>> +    uint64_t pfn, src = 0, start = mapping->start;
>>>       int r;
>>>         /* normally,bo_va->flags only contians READABLE and
>>> WIRTEABLE bit go here
>>> @@ -1353,7 +1363,7 @@ static int amdgpu_vm_bo_split_mapping(struct
>>> amdgpu_device *adev,
>>>           }
>>>           addr += pfn << PAGE_SHIFT;
>>>   -        last = min((uint64_t)mapping->it.last, start +
>>> max_entries - 1);
>>> +        last = min((uint64_t)mapping->last, start + max_entries - 1);
>>>           r = amdgpu_vm_bo_update_mapping(adev, exclusive,
>>>                           src, pages_addr, vm,
>>>                           start, last, flags, addr,
>>> @@ -1368,7 +1378,7 @@ static int amdgpu_vm_bo_split_mapping(struct
>>> amdgpu_device *adev,
>>>           }
>>>           start = last + 1;
>>>   -    } while (unlikely(start != mapping->it.last + 1));
>>> +    } while (unlikely(start != mapping->last + 1));
>>>         return 0;
>>>   }
>>> @@ -1724,9 +1734,8 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
>>>                uint64_t saddr, uint64_t offset,
>>>                uint64_t size, uint64_t flags)
>>>   {
>>> -    struct amdgpu_bo_va_mapping *mapping;
>>> +    struct amdgpu_bo_va_mapping *mapping, *tmp;
>>>       struct amdgpu_vm *vm = bo_va->vm;
>>> -    struct interval_tree_node *it;
>>>       uint64_t eaddr;
>>>         /* validate the parameters */
>>> @@ -1743,14 +1752,12 @@ int amdgpu_vm_bo_map(struct amdgpu_device
>>> *adev,
>>>       saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>       eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>   -    it = interval_tree_iter_first(&vm->va, saddr, eaddr);
>>> -    if (it) {
>>> -        struct amdgpu_bo_va_mapping *tmp;
>>> -        tmp = container_of(it, struct amdgpu_bo_va_mapping, it);
>>> +    tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
>>> +    if (tmp) {
>>>           /* bo and tmp overlap, invalid addr */
>>>           dev_err(adev->dev, "bo %p va 0x%010Lx-0x%010Lx conflict
>>> with "
>>> -            "0x%010lx-0x%010lx\n", bo_va->bo, saddr, eaddr,
>>> -            tmp->it.start, tmp->it.last + 1);
>>> +            "0x%010Lx-0x%010Lx\n", bo_va->bo, saddr, eaddr,
>>> +            tmp->start, tmp->last + 1);
>>>           return -EINVAL;
>>>       }
>>>   @@ -1759,13 +1766,13 @@ int amdgpu_vm_bo_map(struct amdgpu_device
>>> *adev,
>>>           return -ENOMEM;
>>>         INIT_LIST_HEAD(&mapping->list);
>>> -    mapping->it.start = saddr;
>>> -    mapping->it.last = eaddr;
>>> +    mapping->start = saddr;
>>> +    mapping->last = eaddr;
>>>       mapping->offset = offset;
>>>       mapping->flags = flags;
>>>         list_add(&mapping->list, &bo_va->invalids);
>>> -    interval_tree_insert(&mapping->it, &vm->va);
>>> +    amdgpu_vm_it_insert(mapping, &vm->va);
>>>         if (flags & AMDGPU_PTE_PRT)
>>>           amdgpu_vm_prt_get(adev);
>>> @@ -1823,13 +1830,13 @@ int amdgpu_vm_bo_replace_map(struct
>>> amdgpu_device *adev,
>>>       saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>       eaddr /= AMDGPU_GPU_PAGE_SIZE;
>>>   -    mapping->it.start = saddr;
>>> -    mapping->it.last = eaddr;
>>> +    mapping->start = saddr;
>>> +    mapping->last = eaddr;
>>>       mapping->offset = offset;
>>>       mapping->flags = flags;
>>>         list_add(&mapping->list, &bo_va->invalids);
>>> -    interval_tree_insert(&mapping->it, &vm->va);
>>> +    amdgpu_vm_it_insert(mapping, &vm->va);
>>>         if (flags & AMDGPU_PTE_PRT)
>>>           amdgpu_vm_prt_get(adev);
>>> @@ -1860,7 +1867,7 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device
>>> *adev,
>>>       saddr /= AMDGPU_GPU_PAGE_SIZE;
>>>         list_for_each_entry(mapping, &bo_va->valids, list) {
>>> -        if (mapping->it.start == saddr)
>>> +        if (mapping->start == saddr)
>>>               break;
>>>       }
>>>   @@ -1868,7 +1875,7 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device
>>> *adev,
>>>           valid = false;
>>>             list_for_each_entry(mapping, &bo_va->invalids, list) {
>>> -            if (mapping->it.start == saddr)
>>> +            if (mapping->start == saddr)
>>>                   break;
>>>           }
>>>   @@ -1877,7 +1884,7 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device
>>> *adev,
>>>       }
>>>         list_del(&mapping->list);
>>> -    interval_tree_remove(&mapping->it, &vm->va);
>>> +    amdgpu_vm_it_remove(mapping, &vm->va);
>>>       trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>>         if (valid)
>>> @@ -1905,7 +1912,6 @@ int amdgpu_vm_bo_clear_mappings(struct
>>> amdgpu_device *adev,
>>>                   uint64_t saddr, uint64_t size)
>>>   {
>>>       struct amdgpu_bo_va_mapping *before, *after, *tmp, *next;
>>> -    struct interval_tree_node *it;
>>>       LIST_HEAD(removed);
>>>       uint64_t eaddr;
>>>   @@ -1927,43 +1933,42 @@ int amdgpu_vm_bo_clear_mappings(struct
>>> amdgpu_device *adev,
>>>       INIT_LIST_HEAD(&after->list);
>>>         /* Now gather all removed mappings */
>>> -    it = interval_tree_iter_first(&vm->va, saddr, eaddr);
>>> -    while (it) {
>>> -        tmp = container_of(it, struct amdgpu_bo_va_mapping, it);
>>> -        it = interval_tree_iter_next(it, saddr, eaddr);
>>> -
>>> +    tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
>>> +    while (tmp) {
>>>           /* Remember mapping split at the start */
>>> -        if (tmp->it.start < saddr) {
>>> -            before->it.start = tmp->it.start;
>>> -            before->it.last = saddr - 1;
>>> +        if (tmp->start < saddr) {
>>> +            before->start = tmp->start;
>>> +            before->last = saddr - 1;
>>>               before->offset = tmp->offset;
>>>               before->flags = tmp->flags;
>>>               list_add(&before->list, &tmp->list);
>>>           }
>>>             /* Remember mapping split at the end */
>>> -        if (tmp->it.last > eaddr) {
>>> -            after->it.start = eaddr + 1;
>>> -            after->it.last = tmp->it.last;
>>> +        if (tmp->last > eaddr) {
>>> +            after->start = eaddr + 1;
>>> +            after->last = tmp->last;
>>>               after->offset = tmp->offset;
>>> -            after->offset += after->it.start - tmp->it.start;
>>> +            after->offset += after->start - tmp->start;
>>>               after->flags = tmp->flags;
>>>               list_add(&after->list, &tmp->list);
>>>           }
>>>             list_del(&tmp->list);
>>>           list_add(&tmp->list, &removed);
>>> +
>>> +        tmp = amdgpu_vm_it_iter_next(tmp, saddr, eaddr);
>>>       }
>>>         /* And free them up */
>>>       list_for_each_entry_safe(tmp, next, &removed, list) {
>>> -        interval_tree_remove(&tmp->it, &vm->va);
>>> +        amdgpu_vm_it_remove(tmp, &vm->va);
>>>           list_del(&tmp->list);
>>>   -        if (tmp->it.start < saddr)
>>> -            tmp->it.start = saddr;
>>> -        if (tmp->it.last > eaddr)
>>> -            tmp->it.last = eaddr;
>>> +        if (tmp->start < saddr)
>>> +            tmp->start = saddr;
>>> +        if (tmp->last > eaddr)
>>> +            tmp->last = eaddr;
>>>             list_add(&tmp->list, &vm->freed);
>>>           trace_amdgpu_vm_bo_unmap(NULL, tmp);
>>> @@ -1971,7 +1976,7 @@ int amdgpu_vm_bo_clear_mappings(struct
>>> amdgpu_device *adev,
>>>         /* Insert partial mapping before the range */
>>>       if (!list_empty(&before->list)) {
>>> -        interval_tree_insert(&before->it, &vm->va);
>>> +        amdgpu_vm_it_insert(before, &vm->va);
>>>           if (before->flags & AMDGPU_PTE_PRT)
>>>               amdgpu_vm_prt_get(adev);
>>>       } else {
>>> @@ -1980,7 +1985,7 @@ int amdgpu_vm_bo_clear_mappings(struct
>>> amdgpu_device *adev,
>>>         /* Insert partial mapping after the range */
>>>       if (!list_empty(&after->list)) {
>>> -        interval_tree_insert(&after->it, &vm->va);
>>> +        amdgpu_vm_it_insert(after, &vm->va);
>>>           if (after->flags & AMDGPU_PTE_PRT)
>>>               amdgpu_vm_prt_get(adev);
>>>       } else {
>>> @@ -2014,13 +2019,13 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device
>>> *adev,
>>>         list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
>>>           list_del(&mapping->list);
>>> -        interval_tree_remove(&mapping->it, &vm->va);
>>> +        amdgpu_vm_it_remove(mapping, &vm->va);
>>>           trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>>           list_add(&mapping->list, &vm->freed);
>>>       }
>>>       list_for_each_entry_safe(mapping, next, &bo_va->invalids, list) {
>>>           list_del(&mapping->list);
>>> -        interval_tree_remove(&mapping->it, &vm->va);
>>> +        amdgpu_vm_it_remove(mapping, &vm->va);
>>>           amdgpu_vm_free_mapping(adev, vm, mapping,
>>>                          bo_va->last_pt_update);
>>>       }
>>> @@ -2162,9 +2167,9 @@ void amdgpu_vm_fini(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm)
>>>       if (!RB_EMPTY_ROOT(&vm->va)) {
>>>           dev_err(adev->dev, "still active bo inside vm\n");
>>>       }
>>> -    rbtree_postorder_for_each_entry_safe(mapping, tmp, &vm->va,
>>> it.rb) {
>>> +    rbtree_postorder_for_each_entry_safe(mapping, tmp, &vm->va, rb) {
>>>           list_del(&mapping->list);
>>> -        interval_tree_remove(&mapping->it, &vm->va);
>>> +        amdgpu_vm_it_remove(mapping, &vm->va);
>>>           kfree(mapping);
>>>       }
>>>       list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>



More information about the amd-gfx mailing list