Re: [PATCH 1/3] drm/amdgpu: do not pass ttm_resource_manager to gtt_mgr

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 10/20/2021 2:18 PM, Das, Nirmoy wrote:
> 
> On 10/20/2021 8:49 AM, Christian König wrote:
>> Am 19.10.21 um 20:14 schrieb Nirmoy Das:
>>> Do not allow exported amdgpu_gtt_mgr_*() to accept
>>> any ttm_resource_manager pointer. Also there is no need
>>> to force other module to call a ttm function just to
>>> eventually call gtt_mgr functions.
>>
>> That's a rather bad idea I think.
>>
>> The GTT and VRAM manager work on their respective objects and not on 
>> the adev directly.
> 
> 
> What is bothering me is : it is obvious that  the amdgpu_gtt_mgr_usage() 
> for example should only calculate
> 
> usages for TTM_PL_TT type resource manager, why to pass that explicitly. 
> I am trying to leverage the fact that
> 
> we only have one gtt/vram manager for a adev and the functions that I 
> changed  work on whole gtt/vram manager
> 
> as a unit.
> 

Don't know about the functional aspects. From a sofware perspective, 
amdgpu_gtt_mgr_*() operating on struct amdgpu_gtt_mgr *mgr seems more 
logical.

Thanks,
Lijo

> 
> Regards,
> 
> Nirmoy
> 
> 
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |  4 +--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 31 ++++++++++++---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c     |  4 +--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  4 +--
>>>   4 files changed, 24 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 41ce86244144..5807df52031c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -4287,7 +4287,7 @@ static int amdgpu_device_reset_sriov(struct 
>>> amdgpu_device *adev,
>>>         amdgpu_virt_init_data_exchange(adev);
>>>       /* we need recover gart prior to run SMC/CP/SDMA resume */
>>> - amdgpu_gtt_mgr_recover(ttm_manager_type(&adev->mman.bdev, TTM_PL_TT));
>>> +    amdgpu_gtt_mgr_recover(adev);
>>>         r = amdgpu_device_fw_loading(adev);
>>>       if (r)
>>> @@ -4604,7 +4604,7 @@ int amdgpu_do_asic_reset(struct list_head 
>>> *device_list_handle,
>>>                       amdgpu_inc_vram_lost(tmp_adev);
>>>                   }
>>>   -                r = 
>>> amdgpu_gtt_mgr_recover(ttm_manager_type(&tmp_adev->mman.bdev, 
>>> TTM_PL_TT));
>>> +                r = amdgpu_gtt_mgr_recover(tmp_adev);
>>>                   if (r)
>>>                       goto out;
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>> index c18f16b3be9c..5e41f8ef743a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>> @@ -77,10 +77,8 @@ 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;
>>>   -    man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>> -    return sysfs_emit(buf, "%llu\n", amdgpu_gtt_mgr_usage(man));
>>> +    return sysfs_emit(buf, "%llu\n", amdgpu_gtt_mgr_usage(adev));
>>>   }
>>>     static DEVICE_ATTR(mem_info_gtt_total, S_IRUGO,
>>> @@ -206,14 +204,19 @@ static void amdgpu_gtt_mgr_del(struct 
>>> ttm_resource_manager *man,
>>>   /**
>>>    * amdgpu_gtt_mgr_usage - return usage of GTT domain
>>>    *
>>> - * @man: TTM memory type manager
>>> + * @adev: amdgpu_device pointer
>>>    *
>>>    * Return how many bytes are used in the GTT domain
>>>    */
>>> -uint64_t amdgpu_gtt_mgr_usage(struct ttm_resource_manager *man)
>>> +uint64_t amdgpu_gtt_mgr_usage(struct amdgpu_device *adev)
>>>   {
>>> -    struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
>>> -    s64 result = man->size - atomic64_read(&mgr->available);
>>> +    struct ttm_resource_manager *man;
>>> +    struct amdgpu_gtt_mgr *mgr;
>>> +    s64 result;
>>> +
>>> +    man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>> +    mgr = to_gtt_mgr(man);
>>> +    result = man->size - atomic64_read(&mgr->available);
>>>         return (result > 0 ? result : 0) * PAGE_SIZE;
>>>   }
>>> @@ -221,19 +224,20 @@ uint64_t amdgpu_gtt_mgr_usage(struct 
>>> ttm_resource_manager *man)
>>>   /**
>>>    * amdgpu_gtt_mgr_recover - re-init gart
>>>    *
>>> - * @man: TTM memory type manager
>>> + * @adev: amdgpu_device pointer
>>>    *
>>>    * Re-init the gart for each known BO in the GTT.
>>>    */
>>> -int amdgpu_gtt_mgr_recover(struct ttm_resource_manager *man)
>>> +int amdgpu_gtt_mgr_recover(struct amdgpu_device *adev)
>>>   {
>>> -    struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
>>> -    struct amdgpu_device *adev;
>>> +    struct ttm_resource_manager *man;
>>> +    struct amdgpu_gtt_mgr *mgr;
>>>       struct amdgpu_gtt_node *node;
>>>       struct drm_mm_node *mm_node;
>>>       int r = 0;
>>>   -    adev = container_of(mgr, typeof(*adev), mman.gtt_mgr);
>>> +    man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>> +    mgr = to_gtt_mgr(man);
>>>       spin_lock(&mgr->lock);
>>>       drm_mm_for_each_node(mm_node, &mgr->mm) {
>>>           node = container_of(mm_node, typeof(*node), base.mm_nodes[0]);
>>> @@ -260,6 +264,7 @@ static void amdgpu_gtt_mgr_debug(struct 
>>> ttm_resource_manager *man,
>>>                    struct drm_printer *printer)
>>>   {
>>>       struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
>>> +    struct amdgpu_device *adev = container_of(mgr, typeof(*adev), 
>>> mman.gtt_mgr);
>>>         spin_lock(&mgr->lock);
>>>       drm_mm_print(&mgr->mm, printer);
>>> @@ -267,7 +272,7 @@ static void amdgpu_gtt_mgr_debug(struct 
>>> ttm_resource_manager *man,
>>>         drm_printf(printer, "man size:%llu pages, gtt available:%lld 
>>> pages, usage:%lluMB\n",
>>>              man->size, (u64)atomic64_read(&mgr->available),
>>> -           amdgpu_gtt_mgr_usage(man) >> 20);
>>> +           amdgpu_gtt_mgr_usage(adev) >> 20);
>>>   }
>>>     static const struct ttm_resource_manager_func amdgpu_gtt_mgr_func 
>>> = {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index d2955ea4a62b..b9b38f70e416 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -678,7 +678,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, 
>>> void *data, struct drm_file *filp)
>>>           ui64 = 
>>> amdgpu_vram_mgr_vis_usage(ttm_manager_type(&adev->mman.bdev, 
>>> TTM_PL_VRAM));
>>>           return copy_to_user(out, &ui64, min(size, 8u)) ? -EFAULT : 0;
>>>       case AMDGPU_INFO_GTT_USAGE:
>>> -        ui64 = 
>>> amdgpu_gtt_mgr_usage(ttm_manager_type(&adev->mman.bdev, TTM_PL_TT));
>>> +        ui64 = amdgpu_gtt_mgr_usage(adev);
>>>           return copy_to_user(out, &ui64, min(size, 8u)) ? -EFAULT : 0;
>>>       case AMDGPU_INFO_GDS_CONFIG: {
>>>           struct drm_amdgpu_info_gds gds_info;
>>> @@ -738,7 +738,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, 
>>> void *data, struct drm_file *filp)
>>>           mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
>>>               atomic64_read(&adev->gart_pin_size);
>>>           mem.gtt.heap_usage =
>>> -            amdgpu_gtt_mgr_usage(gtt_man);
>>> +            amdgpu_gtt_mgr_usage(adev);
>>>           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_ttm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> index 91a087f9dc7c..6e337cacef51 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> @@ -114,8 +114,8 @@ 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 ttm_resource_manager *man);
>>> -int amdgpu_gtt_mgr_recover(struct ttm_resource_manager *man);
>>> +uint64_t amdgpu_gtt_mgr_usage(struct amdgpu_device *adev);
>>> +int amdgpu_gtt_mgr_recover(struct amdgpu_device *adev);
>>>     uint64_t amdgpu_preempt_mgr_usage(struct ttm_resource_manager *man);
>>