On Wed, Jun 15, 2016 at 7:44 AM, Christian König deathsimple@vodafone.de wrote:

From: Christian König christian.koenig@amd.com

Free up the memory immediately, remember the last eviction for each domain and make new allocations depend on the last eviction to be completed.

Signed-off-by: Christian König christian.koenig@amd.com

Minor typo in the patch title: s/infrastructur/infrastructure/

Alex

---

drivers/gpu/drm/ttm/ttm_bo.c | 49 ++++++++++++++++++--- drivers/gpu/drm/ttm/ttm_bo_util.c | 92 +++++++++++++++++++++++++++++++++++++++ include/drm/ttm/ttm_bo_driver.h | 24 ++++++++++ 3 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 28cd535..5d93169 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -788,6 +788,34 @@ void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem) EXPORT_SYMBOL(ttm_bo_mem_put);

/**

• • Add the last move fence to the BO and reserve a new shared slot.
• */

+static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,

•                            struct ttm_mem_type_manager *man,
  

•                            struct ttm_mem_reg *mem)
  

+{

•   struct fence *fence;
  

•   int ret;
  

•   spin_lock(&man->move_lock);
  

•   fence = fence_get(man->move);
  

•   spin_unlock(&man->move_lock);
  

•   if (fence) {
  

•           reservation_object_add_shared_fence(bo->resv, fence);
  

•           ret = reservation_object_reserve_shared(bo->resv);
  

•           if (unlikely(ret))
  

•                   return ret;
  

•           fence_put(bo->moving);
  

•           bo->moving = fence;
  

•   }
  

•   return 0;
  

+}

+/**

• Repeatedly evict memory from the LRU for @mem_type until we create enough
• space, or we've evicted everything and there isn't enough space.

*/ @@ -813,10 +841,8 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, if (unlikely(ret != 0)) return ret; } while (1);

•   if (mem->mm_node == NULL)
  

•           return -ENOMEM;
    mem->mem_type = mem_type;
  

•   return 0;
  

•   return ttm_bo_add_move_fence(bo, man, mem);
  

}

static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man, @@ -886,6 +912,10 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, bool has_erestartsys = false; int i, ret;

•   ret = reservation_object_reserve_shared(bo->resv);
  

•   if (unlikely(ret))
  

•           return ret;
  

•   mem->mm_node = NULL;
    for (i = 0; i < placement->num_placement; ++i) {
            const struct ttm_place *place = &placement->placement[i];
  

@@ -919,9 +949,15 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, ret = (*man->func->get_node)(man, bo, place, mem); if (unlikely(ret)) return ret;

•           if (mem->mm_node)
  

•           if (mem->mm_node) {
  

•                   ret = ttm_bo_add_move_fence(bo, man, mem);
  

•                   if (unlikely(ret)) {
  

•                           (*man->func->put_node)(man, mem);
  

•                           return ret;
  

•                   }
                    break;
  

•           }
    }
  
    if ((type_ok && (mem_type == TTM_PL_SYSTEM)) || mem->mm_node) {
  

@@ -1290,6 +1326,7 @@ int ttm_bo_clean_mm(struct ttm_bo_device *bdev, unsigned mem_type) mem_type); return ret; }

•   fence_put(man->move);
  
    man->use_type = false;
    man->has_type = false;
  

@@ -1335,6 +1372,7 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type, man->io_reserve_fastpath = true; man->use_io_reserve_lru = false; mutex_init(&man->io_reserve_mutex);

•   spin_lock_init(&man->move_lock);
    INIT_LIST_HEAD(&man->io_reserve_lru);
  
    ret = bdev->driver->init_mem_type(bdev, type, man);
  

@@ -1353,6 +1391,7 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type, man->size = p_size;

    INIT_LIST_HEAD(&man->lru);

•   man->move = NULL;
  
    return 0;
  

} diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 9ea8d02..0c389a5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -696,3 +696,95 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, return 0; } EXPORT_SYMBOL(ttm_bo_move_accel_cleanup);

+int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,

•                    struct fence *fence, bool evict,
  

•                    struct ttm_mem_reg *new_mem)
  

+{

•   struct ttm_bo_device *bdev = bo->bdev;
  

•   struct ttm_mem_reg *old_mem = &bo->mem;
  

•   struct ttm_mem_type_manager *from = &bdev->man[old_mem->mem_type];
  

•   struct ttm_mem_type_manager *to = &bdev->man[new_mem->mem_type];
  

•   int ret;
  

•   reservation_object_add_excl_fence(bo->resv, fence);
  

•   if (!evict) {
  

•           struct ttm_buffer_object *ghost_obj;
  

•           /**
  

•            * This should help pipeline ordinary buffer moves.
  

•            *
  

•            * Hang old buffer memory on a new buffer object,
  

•            * and leave it to be released when the GPU
  

•            * operation has completed.
  

•            */
  

•           fence_put(bo->moving);
  

•           bo->moving = fence_get(fence);
  

•           ret = ttm_buffer_object_transfer(bo, &ghost_obj);
  

•           if (ret)
  

•                   return ret;
  

•           reservation_object_add_excl_fence(ghost_obj->resv, fence);
  

•           /**
  

•            * If we're not moving to fixed memory, the TTM object
  

•            * needs to stay alive. Otherwhise hang it on the ghost
  

•            * bo to be unbound and destroyed.
  

•            */
  

•           if (!(to->flags & TTM_MEMTYPE_FLAG_FIXED))
  

•                   ghost_obj->ttm = NULL;
  

•           else
  

•                   bo->ttm = NULL;
  

•           ttm_bo_unreserve(ghost_obj);
  

•           ttm_bo_unref(&ghost_obj);
  

•   } else if (from->flags & TTM_MEMTYPE_FLAG_FIXED) {
  

•           /**
  

•            * BO doesn't have a TTM we need to bind/unbind. Just remember
  

•            * this eviction and free up the allocation
  

•            */
  

•           spin_lock(&from->move_lock);
  

•           if (!from->move || fence_is_later(from->move, fence)) {
  

•                   fence_put(from->move);
  

•                   from->move = fence_get(fence);
  

•           }
  

•           spin_unlock(&from->move_lock);
  

•           ttm_bo_free_old_node(bo);
  

•           fence_put(bo->moving);
  

•           bo->moving = fence_get(fence);
  

•   } else {
  

•           /**
  

•            * Last resort, wait for the move to be completed.
  

•            *
  

•            * Should never happen in pratice.
  

•            */
  

•           ret = ttm_bo_wait(bo, false, false);
  

•           if (ret)
  

•                   return ret;
  

•           if (to->flags & TTM_MEMTYPE_FLAG_FIXED) {
  

•                   ttm_tt_destroy(bo->ttm);
  

•                   bo->ttm = NULL;
  

•           }
  

•           ttm_bo_free_old_node(bo);
  

•   }
  

•   *old_mem = *new_mem;
  

•   new_mem->mm_node = NULL;
  

•   return 0;
  

+} +EXPORT_SYMBOL(ttm_bo_pipeline_move); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 44dea22..e2ebe66 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -258,8 +258,10 @@ struct ttm_mem_type_manager_func {

• reserved by the TTM vm system.
• @io_reserve_lru: Optional lru list for unreserving io mem regions.
• @io_reserve_fastpath: Only use bdev::driver::io_mem_reserve to obtain

• • @move_lock: lock for move fence
  • static information. bdev::driver::io_mem_free is never used.
  • @lru: The lru list for this memory type.
• • @move: The fence of the last pipelined move operation.
  • This structure is used to identify and manage memory types for a device.
  • It's set up by the ttm_bo_driver::init_mem_type method.

@@ -286,6 +288,7 @@ struct ttm_mem_type_manager { struct mutex io_reserve_mutex; bool use_io_reserve_lru; bool io_reserve_fastpath;

•   spinlock_t move_lock;
  
    /*
     * Protected by @io_reserve_mutex:
  

@@ -298,6 +301,11 @@ struct ttm_mem_type_manager { */

    struct list_head lru;

•   /*
  

•    * Protected by @move_lock.
  

•    */
  

•   struct fence *move;
  

};

/** @@ -1014,6 +1022,22 @@ extern void ttm_bo_free_old_node(struct ttm_buffer_object *bo); extern int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, struct fence *fence, bool evict, struct ttm_mem_reg *new_mem);

+/**

• • ttm_bo_pipeline_move.
• • @bo: A pointer to a struct ttm_buffer_object.
• • @fence: A fence object that signals when moving is complete.
• • @evict: This is an evict move. Don't return until the buffer is idle.
• • @new_mem: struct ttm_mem_reg indicating where to move.
• • Function for pipelining accelerated moves. Either free the memory
• • immediately or hang it on a temporary buffer object.
• */

+int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,

•                    struct fence *fence, bool evict,
  

•                    struct ttm_mem_reg *new_mem);
  

/**

• ttm_io_prot

-- 2.5.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 02.07.2016 um 10:39 schrieb Thomas Hellstrom:

Christian,

Thanks for doing this! A question below:

On 06/15/2016 01:44 PM, Christian König wrote:

...

From: Christian König christian.koenig@amd.com

Free up the memory immediately, remember the last eviction for each domain and make new allocations depend on the last eviction to be completed.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_bo.c | 49 ++++++++++++++++++--- drivers/gpu/drm/ttm/ttm_bo_util.c | 92 +++++++++++++++++++++++++++++++++++++++ include/drm/ttm/ttm_bo_driver.h | 24 ++++++++++ 3 files changed, 160 insertions(+), 5 deletions(-)

/* * Protected by @io_reserve_mutex: @@ -298,6 +301,11 @@ struct ttm_mem_type_manager { */

struct list_head lru;

• /*

• * Protected by @move_lock.
  

• */
  

• struct fence *move; };

Did you look at protecting the move fence with RCU? I figure where there actually is a fence it doesn't matter much but in the fastpath where move is NULL we'd be able to get rid of a number of locking cycles.

Yeah, thought about that as well.

I guess though there might be both licensing implications and requirements to using kfree_rcu() to free the fence.

The problem wasn't licensing (but it is good that you point that out I wasn't aware of this problem), but that that the existing fence RCU function wouldn't worked here and I didn't had time to take a closer look.

Should be relative easy to switch the read path over, because we already free the fences RCU protected.

Regards, Christian.

/Thomas

On 2016年06月15日 19:44, Christian König wrote:

From: Christian König christian.koenig@amd.com

When we pipeline evictions the page directory could already be moving somewhere else when grab_id is called.

Isn't PD bo protected by job fence? I think before job fence is signalled, the PD bo is safe, there shouldn't be a chance to evict PD bo.

Regards, David Zhou

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index a3d7d13..850c4dd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -661,6 +661,8 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev, } }

• p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->page_directory);
• r = amdgpu_bo_vm_update_pte(p, vm); if (!r) amdgpu_cs_sync_rings(p);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index d3e0576..82efb40 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -177,7 +177,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, struct amdgpu_sync *sync, struct fence *fence, unsigned *vm_id, uint64_t *vm_pd_addr) {

• uint64_t pd_addr = amdgpu_bo_gpu_offset(vm->page_directory); struct amdgpu_device *adev = ring->adev; struct fence *updates = sync->last_vm_update; struct amdgpu_vm_id *id, *idle;

@@ -250,7 +249,7 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, if (atomic64_read(&id->owner) != vm->client_id) continue;

• if (pd_addr != id->pd_gpu_addr)
  

• if (*vm_pd_addr != id->pd_gpu_addr)
  continue;
  
  

  if (!same_ring &&

@@ -298,14 +297,13 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, fence_put(id->flushed_updates); id->flushed_updates = fence_get(updates);

• id->pd_gpu_addr = pd_addr;

• id->pd_gpu_addr = *vm_pd_addr;

  list_move_tail(&id->list, &adev->vm_manager.ids_lru); atomic64_set(&id->owner, vm->client_id); vm->ids[ring->idx] = id;

  *vm_id = id - adev->vm_manager.ids;

• *vm_pd_addr = pd_addr; trace_amdgpu_vm_grab_id(vm, ring->idx, *vm_id, *vm_pd_addr);

  error:

On 2016年06月16日 17:52, Christian König wrote:

Am 16.06.2016 um 10:33 schrieb zhoucm1:

...

On 2016年06月15日 19:44, Christian König wrote:

...

From: Christian König christian.koenig@amd.com

When we pipeline evictions the page directory could already be moving somewhere else when grab_id is called.

Isn't PD bo protected by job fence? I think before job fence is signalled, the PD bo is safe, there shouldn't be a chance to evict PD bo.

The crux here is that we start to pipeline BO evictions (we plan them but don't execute them immediately).

E.g. the eviction won't happen before the protecting fence is signaled, but we have it planned and so the address returned by amdgpu_bo_gpu_offset() is already the new one.

Thanks for mentioned, I see the code in ttm_bo_handle_move_mem: if (bo->mem.mm_node) { bo->offset = (bo->mem.start << PAGE_SHIFT) + bdev->man[bo->mem.mem_type].gpu_offset; bo->cur_placement = bo->mem.placement; } else bo->offset = 0;

it seems better to update the offset after the moving-fence is signalled, add a moving-fence callback?

Regards, David Zhou

Regards, Christian.

...

Regards, David Zhou

...

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index a3d7d13..850c4dd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -661,6 +661,8 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev, } }

• p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->page_directory);

• r = amdgpu_bo_vm_update_pte(p, vm);
  if (!r)
      amdgpu_cs_sync_rings(p);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index d3e0576..82efb40 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -177,7 +177,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, struct amdgpu_sync *sync, struct fence *fence, unsigned *vm_id, uint64_t *vm_pd_addr) {

• uint64_t pd_addr = amdgpu_bo_gpu_offset(vm->page_directory); struct amdgpu_device *adev = ring->adev; struct fence *updates = sync->last_vm_update; struct amdgpu_vm_id *id, *idle;

@@ -250,7 +249,7 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, if (atomic64_read(&id->owner) != vm->client_id) continue;

•    if (pd_addr != id->pd_gpu_addr)
  

•    if (*vm_pd_addr != id->pd_gpu_addr)
          continue;
        if (!same_ring &&
  

@@ -298,14 +297,13 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, fence_put(id->flushed_updates); id->flushed_updates = fence_get(updates);

• id->pd_gpu_addr = pd_addr;

• id->pd_gpu_addr = *vm_pd_addr; list_move_tail(&id->list, &adev->vm_manager.ids_lru); atomic64_set(&id->owner, vm->client_id); vm->ids[ring->idx] = id; *vm_id = id - adev->vm_manager.ids;

• *vm_pd_addr = pd_addr; trace_amdgpu_vm_grab_id(vm, ring->idx, *vm_id, *vm_pd_addr); error:

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel