On Mon, Jan 24, 2022 at 01:25:07PM +0100, Christian König wrote:
> This way we finally fix the problem that new resource are
> not immediately evict-able after allocation.
>
> That has caused numerous problems including OOM on GDS handling
> and not being able to use TTM as general resource manager.
>
> v2: stop assuming in ttm_resource_fini that res->bo is still valid.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 +-
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 115 ++--------------------
> drivers/gpu/drm/ttm/ttm_bo_util.c | 1 -
> drivers/gpu/drm/ttm/ttm_device.c | 64 ++++++-------
> drivers/gpu/drm/ttm/ttm_resource.c | 122 +++++++++++++++++++++++-
> include/drm/ttm/ttm_bo_api.h | 16 ----
> include/drm/ttm/ttm_bo_driver.h | 29 +-----
> include/drm/ttm/ttm_resource.h | 35 +++++++
> 9 files changed, 201 insertions(+), 191 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a96ae4c0e040..cdb489cdf27b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -683,12 +683,12 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>
> if (vm->bulk_moveable) {
> spin_lock(&adev->mman.bdev.lru_lock);
> - ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);
> + ttm_lru_bulk_move_tail(&vm->lru_bulk_move);
> spin_unlock(&adev->mman.bdev.lru_lock);
> return;
> }
>
> - memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
> + ttm_lru_bulk_move_init(&vm->lru_bulk_move);
>
> spin_lock(&adev->mman.bdev.lru_lock);
> list_for_each_entry(bo_base, &vm->idle, vm_status) {
> @@ -698,11 +698,9 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
> if (!bo->parent)
> continue;
>
> - ttm_bo_move_to_lru_tail(&bo->tbo, bo->tbo.resource,
> - &vm->lru_bulk_move);
> + ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move);
> if (shadow)
> ttm_bo_move_to_lru_tail(&shadow->tbo,
> - shadow->tbo.resource,
> &vm->lru_bulk_move);
> }
> spin_unlock(&adev->mman.bdev.lru_lock);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 218a9b3037c7..cdf1ff87c88d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -835,7 +835,7 @@ void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
> bo->priority = I915_TTM_PRIO_NO_PAGES;
> }
>
> - ttm_bo_move_to_lru_tail(bo, bo->resource, NULL);
> + ttm_bo_move_to_lru_tail(bo, NULL);
> spin_unlock(&bo->bdev->lru_lock);
> }
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index db3dc7ef5382..cb0fa932d495 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -69,108 +69,16 @@ static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
> }
> }
>
> -static inline void ttm_bo_move_to_pinned(struct ttm_buffer_object *bo)
> -{
> - struct ttm_device *bdev = bo->bdev;
> -
> - list_move_tail(&bo->lru, &bdev->pinned);
> -
> - if (bdev->funcs->del_from_lru_notify)
> - bdev->funcs->del_from_lru_notify(bo);
> -}
> -
> -static inline void ttm_bo_del_from_lru(struct ttm_buffer_object *bo)
> -{
> - struct ttm_device *bdev = bo->bdev;
> -
> - list_del_init(&bo->lru);
> -
> - if (bdev->funcs->del_from_lru_notify)
> - bdev->funcs->del_from_lru_notify(bo);
> -}
> -
> -static void ttm_bo_bulk_move_set_pos(struct ttm_lru_bulk_move_pos *pos,
> - struct ttm_buffer_object *bo)
> -{
> - if (!pos->first)
> - pos->first = bo;
> - pos->last = bo;
> -}
> -
> void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
> - struct ttm_resource *mem,
> struct ttm_lru_bulk_move *bulk)
> {
> - struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man;
> -
> - if (!bo->deleted)
> - dma_resv_assert_held(bo->base.resv);
> -
> - if (bo->pin_count) {
> - ttm_bo_move_to_pinned(bo);
> - return;
> - }
> -
> - if (!mem)
> - return;
> -
> - man = ttm_manager_type(bdev, mem->mem_type);
> - list_move_tail(&bo->lru, &man->lru[bo->priority]);
> -
> - if (bdev->funcs->del_from_lru_notify)
> - bdev->funcs->del_from_lru_notify(bo);
> -
> - if (bulk && !bo->pin_count) {
> - switch (bo->resource->mem_type) {
> - case TTM_PL_TT:
> - ttm_bo_bulk_move_set_pos(&bulk->tt[bo->priority], bo);
> - break;
> + dma_resv_assert_held(bo->base.resv);
>
> - case TTM_PL_VRAM:
> - ttm_bo_bulk_move_set_pos(&bulk->vram[bo->priority], bo);
> - break;
> - }
> - }
> + if (bo->resource)
> + ttm_resource_move_to_lru_tail(bo->resource, bulk);
> }
> EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>
> -void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> -{
> - unsigned i;
> -
> - for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> - struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
> - struct ttm_resource_manager *man;
> -
> - if (!pos->first)
> - continue;
> -
> - dma_resv_assert_held(pos->first->base.resv);
> - dma_resv_assert_held(pos->last->base.resv);
> -
> - man = ttm_manager_type(pos->first->bdev, TTM_PL_TT);
> - list_bulk_move_tail(&man->lru[i], &pos->first->lru,
> - &pos->last->lru);
> - }
> -
> - for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> - struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
> - struct ttm_resource_manager *man;
> -
> - if (!pos->first)
> - continue;
> -
> - dma_resv_assert_held(pos->first->base.resv);
> - dma_resv_assert_held(pos->last->base.resv);
> -
> - man = ttm_manager_type(pos->first->bdev, TTM_PL_VRAM);
> - list_bulk_move_tail(&man->lru[i], &pos->first->lru,
> - &pos->last->lru);
> - }
> -}
> -EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> -
> static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
> struct ttm_resource *mem, bool evict,
> struct ttm_operation_ctx *ctx,
> @@ -344,7 +252,6 @@ static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> return ret;
> }
>
> - ttm_bo_move_to_pinned(bo);
> list_del_init(&bo->ddestroy);
> spin_unlock(&bo->bdev->lru_lock);
> ttm_bo_cleanup_memtype_use(bo);
> @@ -445,7 +352,7 @@ static void ttm_bo_release(struct kref *kref)
> */
> if (bo->pin_count) {
> bo->pin_count = 0;
> - ttm_bo_move_to_lru_tail(bo, bo->resource, NULL);
> + ttm_resource_move_to_lru_tail(bo->resource, NULL);
> }
>
> kref_init(&bo->kref);
> @@ -458,7 +365,6 @@ static void ttm_bo_release(struct kref *kref)
> }
>
> spin_lock(&bo->bdev->lru_lock);
> - ttm_bo_del_from_lru(bo);
> list_del(&bo->ddestroy);
> spin_unlock(&bo->bdev->lru_lock);
>
> @@ -673,15 +579,17 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
> struct ww_acquire_ctx *ticket)
> {
> struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
> + struct ttm_resource *res;
> bool locked = false;
> unsigned i;
> int ret;
>
> spin_lock(&bdev->lru_lock);
> for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> - list_for_each_entry(bo, &man->lru[i], lru) {
> + list_for_each_entry(res, &man->lru[i], lru) {
> bool busy;
>
> + bo = res->bo;
> if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
> &locked, &busy)) {
> if (busy && !busy_bo && ticket !=
> @@ -699,7 +607,7 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
> }
>
> /* If the inner loop terminated early, we have our candidate */
> - if (&bo->lru != &man->lru[i])
> + if (&res->lru != &man->lru[i])
> break;
>
> bo = NULL;
> @@ -875,9 +783,6 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> }
>
> error:
> - if (bo->resource->mem_type == TTM_PL_SYSTEM && !bo->pin_count)
> - ttm_bo_move_to_lru_tail_unlocked(bo);
> -
> return ret;
> }
> EXPORT_SYMBOL(ttm_bo_mem_space);
> @@ -971,7 +876,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
>
> kref_init(&bo->kref);
> - INIT_LIST_HEAD(&bo->lru);
> INIT_LIST_HEAD(&bo->ddestroy);
> bo->bdev = bdev;
> bo->type = type;
> @@ -1021,8 +925,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> return ret;
> }
>
> - ttm_bo_move_to_lru_tail_unlocked(bo);
> -
> return ret;
> }
> EXPORT_SYMBOL(ttm_bo_init_reserved);
> @@ -1123,7 +1025,6 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
> return ret == -EBUSY ? -ENOSPC : ret;
> }
>
> - ttm_bo_move_to_pinned(bo);
> /* TODO: Cleanup the locking */
> spin_unlock(&bo->bdev->lru_lock);
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 544a84fa6589..0163e92b57af 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -231,7 +231,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
>
> atomic_inc(&ttm_glob.bo_count);
> INIT_LIST_HEAD(&fbo->base.ddestroy);
> - INIT_LIST_HEAD(&fbo->base.lru);
> fbo->base.moving = NULL;
> drm_vma_node_reset(&fbo->base.base.vma_node);
>
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index be24bb6cefd0..ba35887147ba 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -144,6 +144,7 @@ int ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
> {
> struct ttm_resource_manager *man;
> struct ttm_buffer_object *bo;
> + struct ttm_resource *res;
> unsigned i, j;
> int ret;
>
> @@ -154,8 +155,11 @@ int ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
> continue;
>
> for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
> - list_for_each_entry(bo, &man->lru[j], lru) {
> - uint32_t num_pages = PFN_UP(bo->base.size);
> + list_for_each_entry(res, &man->lru[j], lru) {
> + uint32_t num_pages;
> +
> + bo = res->bo;
> + num_pages = PFN_UP(bo->base.size);
>
> ret = ttm_bo_swapout(bo, ctx, gfp_flags);
> /* ttm_bo_swapout has dropped the lru_lock */
> @@ -259,49 +263,45 @@ void ttm_device_fini(struct ttm_device *bdev)
> }
> EXPORT_SYMBOL(ttm_device_fini);
>
> -void ttm_device_clear_dma_mappings(struct ttm_device *bdev)
> +static void ttm_device_clear_lru_dma_mappings(struct ttm_device *bdev,
> + struct list_head *list)
> {
> - struct ttm_resource_manager *man;
> - struct ttm_buffer_object *bo;
> - unsigned int i, j;
> + struct ttm_resource *res;
>
> spin_lock(&bdev->lru_lock);
> - while (!list_empty(&bdev->pinned)) {
> - bo = list_first_entry(&bdev->pinned, struct ttm_buffer_object, lru);
> + while ((res = list_first_entry_or_null(list, typeof(*res), lru))) {
> + struct ttm_buffer_object *bo = res->bo;
> +
> /* Take ref against racing releases once lru_lock is unlocked */
> - if (ttm_bo_get_unless_zero(bo)) {
> - list_del_init(&bo->lru);
> - spin_unlock(&bdev->lru_lock);
> + if (!ttm_bo_get_unless_zero(bo))
> + continue;
>
> - if (bo->ttm)
> - ttm_tt_unpopulate(bo->bdev, bo->ttm);
> + list_del_init(&res->lru);
> + spin_unlock(&bdev->lru_lock);
>
> - ttm_bo_put(bo);
> - spin_lock(&bdev->lru_lock);
> - }
> + if (bo->ttm)
> + ttm_tt_unpopulate(bo->bdev, bo->ttm);
> +
> + ttm_bo_put(bo);
> + spin_lock(&bdev->lru_lock);
> }
> + spin_unlock(&bdev->lru_lock);
> +}
> +
> +void ttm_device_clear_dma_mappings(struct ttm_device *bdev)
> +{
> + struct ttm_resource_manager *man;
> + unsigned int i, j;
> +
> + ttm_device_clear_lru_dma_mappings(bdev, &bdev->pinned);
>
> for (i = TTM_PL_SYSTEM; i < TTM_NUM_MEM_TYPES; ++i) {
> man = ttm_manager_type(bdev, i);
> if (!man || !man->use_tt)
> continue;
>
> - for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
> - while (!list_empty(&man->lru[j])) {
> - bo = list_first_entry(&man->lru[j], struct ttm_buffer_object, lru);
> - if (ttm_bo_get_unless_zero(bo)) {
> - list_del_init(&bo->lru);
> - spin_unlock(&bdev->lru_lock);
> -
> - if (bo->ttm)
> - ttm_tt_unpopulate(bo->bdev, bo->ttm);
> -
> - ttm_bo_put(bo);
> - spin_lock(&bdev->lru_lock);
> - }
> - }
> - }
> + for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j)
> + ttm_device_clear_lru_dma_mappings(bdev, &man->lru[j]);
> }
> - spin_unlock(&bdev->lru_lock);
> }
> EXPORT_SYMBOL(ttm_device_clear_dma_mappings);
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index b8362492980d..450e665c357b 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -29,6 +29,106 @@
> #include <drm/ttm/ttm_resource.h>
> #include <drm/ttm/ttm_bo_driver.h>
>
> +/**
> + * ttm_lru_bulk_move_init - initialize a bulk move structure
> + * @bulk: the structure to init
> + *
> + * For now just memset the structure to zero.
> + */
> +void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk)
I like the ttm_lru_bulk_move prefixe bikeshed.
> +{
> + memset(bulk, 0, sizeof(*bulk));
> +}
> +EXPORT_SYMBOL(ttm_lru_bulk_move_init);
> +
> +/**
> + * ttm_lru_bulk_move_tail
> + *
> + * @bulk: bulk move structure
> + *
> + * Bulk move BOs to the LRU tail, only valid to use when driver makes sure that
> + * resource order never changes. Should be called with ttm_device::lru_lock held.
^
This is a doxygen link or something, defo not sphinx. Pls fix.
Also I think a bit more explaining how exactly this works (e.g. mention
that you need to guarantee that _all_ the bos in your bulk lru section
need to be locked) and other things would be good here. I know this is
just moved, but given how much fun you guys had to debug corner cases I
think it's worth to elaborate here a bit.
> + */
> +void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk)
> +{
> + unsigned i;
Also assert_lockdep_held here for the lru lock, due to all the callers all
over.
> +
> + for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> + struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
> + struct ttm_resource_manager *man;
> +
> + if (!pos->first)
> + continue;
> +
> + dma_resv_assert_held(pos->first->bo->base.resv);
btw random idea for debug mode with lockdep enabled: Loop from first ->
last and check that they're _all_ locked? Also catches the case where
first/last aren't on the same list, and other screw-ups. And performance
with lockdep enabled is pretty bad anyway, so the full list walk here
shouldn't be a big disaster.
Or maybe only do that when we have the special dma_resv debug Kconfig
enabled.
> + dma_resv_assert_held(pos->last->bo->base.resv);
> +
> + man = ttm_manager_type(pos->first->bo->bdev, TTM_PL_TT);
> + list_bulk_move_tail(&man->lru[i], &pos->first->lru,
> + &pos->last->lru);
> + }
> +
> + for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> + struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
> + struct ttm_resource_manager *man;
> +
> + if (!pos->first)
> + continue;
> +
> + dma_resv_assert_held(pos->first->bo->base.resv);
> + dma_resv_assert_held(pos->last->bo->base.resv);
> +
> + man = ttm_manager_type(pos->first->bo->bdev, TTM_PL_VRAM);
> + list_bulk_move_tail(&man->lru[i], &pos->first->lru,
> + &pos->last->lru);
> + }
> +}
> +EXPORT_SYMBOL(ttm_lru_bulk_move_tail);
> +
> +/* Record a resource position in a bulk move structure */
> +static void ttm_lru_bulk_move_set_pos(struct ttm_lru_bulk_move_pos *pos,
> + struct ttm_resource *res)
> +{
> + if (!pos->first)
> + pos->first = res;
> + pos->last = res;
> +}
> +
> +/* Move a resource to the LRU tail and track the bulk position */
> +void ttm_resource_move_to_lru_tail(struct ttm_resource *res,
> + struct ttm_lru_bulk_move *bulk)
> +{
> + struct ttm_buffer_object *bo = res->bo;
> + struct ttm_device *bdev = bo->bdev;
> + struct ttm_resource_manager *man;
I think lockdep_assert_held here on the lru lock would be good, since
there's quite a few callers all over.
Also maybe double-check that the dma_resv is also held, like in the bulk
move. I get why the bulk move checks more since it's easier to screw that
up, but checking here is probably also good.
> +
> + if (bo->pin_count) {
> + list_move_tail(&res->lru, &bdev->pinned);
> + if (bdev->funcs->del_from_lru_notify)
> + bdev->funcs->del_from_lru_notify(res->bo);
> + return;
> + }
> +
> + man = ttm_manager_type(bdev, res->mem_type);
> + list_move_tail(&res->lru, &man->lru[bo->priority]);
> +
> + if (bdev->funcs->del_from_lru_notify)
> + bdev->funcs->del_from_lru_notify(bo);
> +
> + if (!bulk)
> + return;
> +
> + switch (res->mem_type) {
> + case TTM_PL_TT:
> + ttm_lru_bulk_move_set_pos(&bulk->tt[bo->priority], res);
> + break;
> +
> + case TTM_PL_VRAM:
> + ttm_lru_bulk_move_set_pos(&bulk->vram[bo->priority], res);
> + break;
> + }
> +}
> +
> void ttm_resource_init(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> struct ttm_resource *res)
> @@ -44,16 +144,36 @@ void ttm_resource_init(struct ttm_buffer_object *bo,
> res->bus.is_iomem = false;
> res->bus.caching = ttm_cached;
> res->bo = bo;
> + INIT_LIST_HEAD(&res->lru);
>
> man = ttm_manager_type(bo->bdev, place->mem_type);
> atomic64_add(bo->base.size, &man->usage);
> +
> + spin_lock(&bo->bdev->lru_lock);
> + ttm_resource_move_to_lru_tail(res, NULL);
> + spin_unlock(&bo->bdev->lru_lock);
> }
> EXPORT_SYMBOL(ttm_resource_init);
>
> +/**
> + * ttm_resource_fini
> + *
> + * @res: the resource to clean up
> + *
> + * Make sure the resource is removed from the LRU before destruction.
> + */
Ah kerneldoc should probably be put into the earlier patch and explain
more when driver authors need to call this, and not so much what it
internally all needs to guaranteed.
> void ttm_resource_fini(struct ttm_resource_manager *man,
> struct ttm_resource *res)
> {
> - atomic64_sub(res->bo->base.size, &man->usage);
> + struct ttm_device *bdev = man->bdev;
> +
> + spin_lock(&bdev->lru_lock);
> + list_del_init(&res->lru);
> + if (res->bo && bdev->funcs->del_from_lru_notify)
> + bdev->funcs->del_from_lru_notify(res->bo);
> + spin_unlock(&bdev->lru_lock);
> +
> + atomic64_sub(res->num_pages << PAGE_SHIFT, &man->usage);
Yeah more reasons that you really don't need an atomic here. I think if
you reorder the patches to first move the lru and then add account it will
be blantantly obvious that we don't need the resource tracking to be
atomic.
> }
> EXPORT_SYMBOL(ttm_resource_fini);
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index c17b2df9178b..3da77fc54552 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -55,8 +55,6 @@ struct ttm_placement;
>
> struct ttm_place;
>
> -struct ttm_lru_bulk_move;
> -
> /**
> * enum ttm_bo_type
> *
> @@ -94,7 +92,6 @@ struct ttm_tt;
> * @ttm: TTM structure holding system pages.
> * @evicted: Whether the object was evicted without user-space knowing.
> * @deleted: True if the object is only a zombie and already deleted.
> - * @lru: List head for the lru list.
> * @ddestroy: List head for the delayed destroy list.
> * @swap: List head for swap LRU list.
> * @moving: Fence set when BO is moving
> @@ -143,7 +140,6 @@ struct ttm_buffer_object {
> * Members protected by the bdev::lru_lock.
> */
>
> - struct list_head lru;
> struct list_head ddestroy;
>
> /**
> @@ -295,7 +291,6 @@ void ttm_bo_put(struct ttm_buffer_object *bo);
> * ttm_bo_move_to_lru_tail
> *
> * @bo: The buffer object.
> - * @mem: Resource object.
> * @bulk: optional bulk move structure to remember BO positions
> *
> * Move this BO to the tail of all lru lists used to lookup and reserve an
> @@ -303,19 +298,8 @@ void ttm_bo_put(struct ttm_buffer_object *bo);
> * held, and is used to make a BO less likely to be considered for eviction.
> */
> void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
> - struct ttm_resource *mem,
> struct ttm_lru_bulk_move *bulk);
>
> -/**
> - * ttm_bo_bulk_move_lru_tail
> - *
> - * @bulk: bulk move structure
> - *
> - * Bulk move BOs to the LRU tail, only valid to use when driver makes sure that
> - * BO order never changes. Should be called with ttm_global::lru_lock held.
> - */
> -void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk);
> -
> /**
> * ttm_bo_lock_delayed_workqueue
> *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 5f087575194b..6c7352e13708 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -45,33 +45,6 @@
> #include "ttm_tt.h"
> #include "ttm_pool.h"
>
> -/**
> - * struct ttm_lru_bulk_move_pos
> - *
> - * @first: first BO in the bulk move range
> - * @last: last BO in the bulk move range
> - *
> - * Positions for a lru bulk move.
> - */
> -struct ttm_lru_bulk_move_pos {
> - struct ttm_buffer_object *first;
> - struct ttm_buffer_object *last;
> -};
> -
> -/**
> - * struct ttm_lru_bulk_move
> - *
> - * @tt: first/last lru entry for BOs in the TT domain
> - * @vram: first/last lru entry for BOs in the VRAM domain
> - * @swap: first/last lru entry for BOs on the swap list
> - *
> - * Helper structure for bulk moves on the LRU list.
> - */
> -struct ttm_lru_bulk_move {
> - struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY];
> - struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY];
> -};
> -
> /*
> * ttm_bo.c
> */
> @@ -182,7 +155,7 @@ static inline void
> ttm_bo_move_to_lru_tail_unlocked(struct ttm_buffer_object *bo)
> {
> spin_lock(&bo->bdev->lru_lock);
> - ttm_bo_move_to_lru_tail(bo, bo->resource, NULL);
> + ttm_bo_move_to_lru_tail(bo, NULL);
> spin_unlock(&bo->bdev->lru_lock);
> }
>
> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> index 3d391279b33f..a54d52517a30 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -26,10 +26,12 @@
> #define _TTM_RESOURCE_H_
>
> #include <linux/types.h>
> +#include <linux/list.h>
> #include <linux/mutex.h>
> #include <linux/atomic.h>
> #include <linux/dma-buf-map.h>
> #include <linux/dma-fence.h>
> +
> #include <drm/drm_print.h>
> #include <drm/ttm/ttm_caching.h>
> #include <drm/ttm/ttm_kmap_iter.h>
> @@ -178,6 +180,33 @@ struct ttm_resource {
> uint32_t placement;
> struct ttm_bus_placement bus;
> struct ttm_buffer_object *bo;
Kerneldoc missing here. Please use an in-line one which explains:
- locking
- refcounting
- anything else that ever matters really, like that we guarantee that
statistics gathered are consistent when you hold the lru lock
- Maybe link to the bulk lru movement for added doc goodness?
> + struct list_head lru;
> +};
> +
> +/**
> + * struct ttm_lru_bulk_move_pos
> + *
> + * @first: first res in the bulk move range
> + * @last: last res in the bulk move range
> + *
> + * Positions for a lru bulk move.
> + */
> +struct ttm_lru_bulk_move_pos {
> + struct ttm_resource *first;
> + struct ttm_resource *last;
> +};
> +
> +/**
> + * struct ttm_lru_bulk_move
> + *
> + * @tt: first/last lru entry for resources in the TT domain
> + * @vram: first/last lru entry for resources in the VRAM domain
> + *
> + * Helper structure for bulk moves on the LRU list.
I know it's just moved, but please also link to the bulk lru functions
here.
> + */
> +struct ttm_lru_bulk_move {
> + struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY];
> + struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY];
> };
>
> /**
> @@ -279,6 +308,12 @@ ttm_resource_manager_usage(struct ttm_resource_manager *man)
> return atomic64_read(&man->usage);
> }
>
> +void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk);
> +void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk);
> +
> +void ttm_resource_move_to_lru_tail(struct ttm_resource *res,
> + struct ttm_lru_bulk_move *bulk);
> +
> void ttm_resource_init(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> struct ttm_resource *res);
I didn't check all the code movements in detail, but looks good with a few
spot checks. With the bikesheds addressed somehow:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
