Re: [PATCH 06/11] drm/amdgpu: remove GTT accounting v2

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 14.02.22 um 12:10 schrieb Matthew Auld:
> On Mon, 14 Feb 2022 at 09:34, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> This is provided by TTM now.
>>
>> Also switch man->size to bytes instead of pages and fix the double
>> printing of size and usage in debugfs.
>>
>> v2: fix size checking as well
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Tested-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 49 +++++----------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c     |  8 ++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  2 -
>>   4 files changed, 16 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> index e0c7fbe01d93..3bcd27ae379d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> @@ -60,7 +60,7 @@ static ssize_t amdgpu_mem_info_gtt_total_show(struct device *dev,
>>          struct ttm_resource_manager *man;
>>
>>          man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>> -       return sysfs_emit(buf, "%llu\n", man->size * PAGE_SIZE);
>> +       return sysfs_emit(buf, "%llu\n", man->size);
>>   }
>>
>>   /**
>> @@ -77,8 +77,9 @@ static ssize_t amdgpu_mem_info_gtt_used_show(struct device *dev,
>>   {
>>          struct drm_device *ddev = dev_get_drvdata(dev);
>>          struct amdgpu_device *adev = drm_to_adev(ddev);
>> +       struct ttm_resource_manager *man = &adev->mman.gtt_mgr.manager;
>>
>> -       return sysfs_emit(buf, "%llu\n", amdgpu_gtt_mgr_usage(&adev->mman.gtt_mgr));
>> +       return sysfs_emit(buf, "%llu\n", ttm_resource_manager_usage(man));
>>   }
>>
>>   static DEVICE_ATTR(mem_info_gtt_total, S_IRUGO,
>> @@ -130,20 +131,17 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
>>          struct amdgpu_gtt_node *node;
>>          int r;
>>
>> -       if (!(place->flags & TTM_PL_FLAG_TEMPORARY) &&
>> -           atomic64_add_return(num_pages, &mgr->used) >  man->size) {
>> -               atomic64_sub(num_pages, &mgr->used);
> I guess this behaviour is now slightly different, since TEMPORARY will
> now get accounted like everything else. Hopefully that is not a
> concern.

Yeah, that's correct but also unproblematic. See a few lines below.

>
> Otherwise seems mechanical,
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>
>
>> -               return -ENOSPC;
>> -       }
>> -
>>          node = kzalloc(struct_size(node, base.mm_nodes, 1), GFP_KERNEL);
>> -       if (!node) {
>> -               r = -ENOMEM;
>> -               goto err_out;
>> -       }
>> +       if (!node)
>> +               return -ENOMEM;
>>
>>          node->tbo = tbo;
>>          ttm_resource_init(tbo, place, &node->base.base);
>> +       if (!(place->flags & TTM_PL_FLAG_TEMPORARY) &&
>> +           ttm_resource_manager_usage(man) > man->size) {
>> +               r = -ENOSPC;
>> +               goto err_free;
>> +       }

This here now takes care of TTM_PL_FLAG_TEMPORARY.

Regards,
Christian.

>>
>>          if (place->lpfn) {
>>                  spin_lock(&mgr->lock);
>> @@ -169,11 +167,6 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
>>   err_free:
>>          ttm_resource_fini(man, &node->base.base);
>>          kfree(node);
>> -
>> -err_out:
>> -       if (!(place->flags & TTM_PL_FLAG_TEMPORARY))
>> -               atomic64_sub(num_pages, &mgr->used);
>> -
>>          return r;
>>   }
>>
>> @@ -196,25 +189,10 @@ static void amdgpu_gtt_mgr_del(struct ttm_resource_manager *man,
>>                  drm_mm_remove_node(&node->base.mm_nodes[0]);
>>          spin_unlock(&mgr->lock);
>>
>> -       if (!(res->placement & TTM_PL_FLAG_TEMPORARY))
>> -               atomic64_sub(res->num_pages, &mgr->used);
>> -
>>          ttm_resource_fini(man, res);
>>          kfree(node);
>>   }
>>
>> -/**
>> - * amdgpu_gtt_mgr_usage - return usage of GTT domain
>> - *
>> - * @mgr: amdgpu_gtt_mgr pointer
>> - *
>> - * Return how many bytes are used in the GTT domain
>> - */
>> -uint64_t amdgpu_gtt_mgr_usage(struct amdgpu_gtt_mgr *mgr)
>> -{
>> -       return atomic64_read(&mgr->used) * PAGE_SIZE;
>> -}
>> -
>>   /**
>>    * amdgpu_gtt_mgr_recover - re-init gart
>>    *
>> @@ -260,9 +238,6 @@ static void amdgpu_gtt_mgr_debug(struct ttm_resource_manager *man,
>>          spin_lock(&mgr->lock);
>>          drm_mm_print(&mgr->mm, printer);
>>          spin_unlock(&mgr->lock);
>> -
>> -       drm_printf(printer, "man size:%llu pages,  gtt used:%llu pages\n",
>> -                  man->size, atomic64_read(&mgr->used));
>>   }
>>
>>   static const struct ttm_resource_manager_func amdgpu_gtt_mgr_func = {
>> @@ -288,14 +263,12 @@ int amdgpu_gtt_mgr_init(struct amdgpu_device *adev, uint64_t gtt_size)
>>          man->use_tt = true;
>>          man->func = &amdgpu_gtt_mgr_func;
>>
>> -       ttm_resource_manager_init(man, &adev->mman.bdev,
>> -                                 gtt_size >> PAGE_SHIFT);
>> +       ttm_resource_manager_init(man, &adev->mman.bdev, gtt_size);
>>
>>          start = AMDGPU_GTT_MAX_TRANSFER_SIZE * AMDGPU_GTT_NUM_TRANSFER_WINDOWS;
>>          size = (adev->gmc.gart_size >> PAGE_SHIFT) - start;
>>          drm_mm_init(&mgr->mm, start, size);
>>          spin_lock_init(&mgr->lock);
>> -       atomic64_set(&mgr->used, 0);
>>
>>          ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_TT, &mgr->manager);
>>          ttm_resource_manager_set_used(man, true);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 1ebb91db2274..9ff4aced5da7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -684,7 +684,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>                  ui64 = amdgpu_vram_mgr_vis_usage(&adev->mman.vram_mgr);
>>                  return copy_to_user(out, &ui64, min(size, 8u)) ? -EFAULT : 0;
>>          case AMDGPU_INFO_GTT_USAGE:
>> -               ui64 = amdgpu_gtt_mgr_usage(&adev->mman.gtt_mgr);
>> +               ui64 = ttm_resource_manager_usage(&adev->mman.gtt_mgr.manager);
>>                  return copy_to_user(out, &ui64, min(size, 8u)) ? -EFAULT : 0;
>>          case AMDGPU_INFO_GDS_CONFIG: {
>>                  struct drm_amdgpu_info_gds gds_info;
>> @@ -716,7 +716,8 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>          case AMDGPU_INFO_MEMORY: {
>>                  struct drm_amdgpu_memory_info mem;
>>                  struct ttm_resource_manager *gtt_man =
>> -                       ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>> +                       &adev->mman.gtt_mgr.manager;
>> +
>>                  memset(&mem, 0, sizeof(mem));
>>                  mem.vram.total_heap_size = adev->gmc.real_vram_size;
>>                  mem.vram.usable_heap_size = adev->gmc.real_vram_size -
>> @@ -741,8 +742,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>                  mem.gtt.total_heap_size *= PAGE_SIZE;
>>                  mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
>>                          atomic64_read(&adev->gart_pin_size);
>> -               mem.gtt.heap_usage =
>> -                       amdgpu_gtt_mgr_usage(&adev->mman.gtt_mgr);
>> +               mem.gtt.heap_usage = ttm_resource_manager_usage(gtt_man);
>>                  mem.gtt.max_allocation = mem.gtt.usable_heap_size * 3 / 4;
>>
>>                  return copy_to_user(out, &mem,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 5661b82d84d4..514754142f69 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -451,7 +451,7 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>>          if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>>                  man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>
>> -               if (size < (man->size << PAGE_SHIFT))
>> +               if (size < man->size)
>>                          return true;
>>                  else
>>                          goto fail;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index f8f48be16d80..120b69ec9885 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -52,7 +52,6 @@ struct amdgpu_gtt_mgr {
>>          struct ttm_resource_manager manager;
>>          struct drm_mm mm;
>>          spinlock_t lock;
>> -       atomic64_t used;
>>   };
>>
>>   struct amdgpu_preempt_mgr {
>> @@ -114,7 +113,6 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev);
>>   void amdgpu_vram_mgr_fini(struct amdgpu_device *adev);
>>
>>   bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *mem);
>> -uint64_t amdgpu_gtt_mgr_usage(struct amdgpu_gtt_mgr *mgr);
>>   int amdgpu_gtt_mgr_recover(struct amdgpu_gtt_mgr *mgr);
>>
>>   uint64_t amdgpu_preempt_mgr_usage(struct ttm_resource_manager *man);
>> --
>> 2.25.1
>>