[PATCH 2/3] drm/amdgpu: Implement new dummy vram manager

Felix Kuehling felix.kuehling at amd.com
Thu Jun 15 14:52:40 UTC 2023


Am 2023-06-15 um 03:37 schrieb Christian König:
> Am 14.06.23 um 17:42 schrieb Felix Kuehling:
>> Am 2023-06-14 um 06:38 schrieb Christian König:
>>> Am 10.05.23 um 00:01 schrieb Alex Deucher:
>>>> From: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
>>>>
>>>> This adds dummy vram manager to support ASICs that do not have a
>>>> dedicated or carvedout vram domain.
>>>
>>> Well that doesn't seem to make much sense. Why we should have that?
>>
>> TTM always expects a resource manager for VRAM. There are no NULL 
>> pointer checks in TTM for not having a resource manager for VRAM. The 
>> existing amdgpu_vram_mgr gets confused if there is no VRAM. It seemed 
>> cleaner to add a dummy manager than to scatter conditions for a 
>> memory-less GPU corner case through the regular VRAM manager.
>
> Well no that's absolutely *not* cleaner. TTM has a predefined manager 
> if you need to use a dummy.

I think you are referring to ttm_range_manager. ttm_range_man_alloc does 
a bunch of useless stuff when there is no hope of succeeding:

  * kzalloc a node struct
  * ttm_resource_init
      o add the node to an LRU
  * drm_mm_insert_node_in_range (which fails because the drm_mm was
    created with 0 size)
  * ttm_resource_fini
      o remove the node from an LRU
  * kfree the node struct

In that process it also takes 3 spin_locks. All of that for TTM to 
figure out that VRAM is not a feasible placement. All we need to do here 
in the dummy manager is to return -ENOSPC.

I really don't get why this bothers you so much, or why this is even 
controversial.

Regards,
   Felix


>
> Why the heck didn't you ask me before doing stuff like that?
>
> Regards,
> Christian.
>
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Reviewed-by: Felix Kuehling <felix.kuehling at amd.com>
>>>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
>>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 67 
>>>> ++++++++++++++++++--
>>>>   1 file changed, 60 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>> index 43d6a9d6a538..89d35d194f2c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>> @@ -370,6 +370,45 @@ int amdgpu_vram_mgr_query_page_status(struct 
>>>> amdgpu_vram_mgr *mgr,
>>>>       return ret;
>>>>   }
>>>>   +static void amdgpu_dummy_vram_mgr_debug(struct 
>>>> ttm_resource_manager *man,
>>>> +                  struct drm_printer *printer)
>>>> +{
>>>> +    DRM_DEBUG_DRIVER("Dummy vram mgr debug\n");
>>>> +}
>>>> +
>>>> +static bool amdgpu_dummy_vram_mgr_compatible(struct 
>>>> ttm_resource_manager *man,
>>>> +                       struct ttm_resource *res,
>>>> +                       const struct ttm_place *place,
>>>> +                       size_t size)
>>>> +{
>>>> +    DRM_DEBUG_DRIVER("Dummy vram mgr compatible\n");
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static bool amdgpu_dummy_vram_mgr_intersects(struct 
>>>> ttm_resource_manager *man,
>>>> +                       struct ttm_resource *res,
>>>> +                       const struct ttm_place *place,
>>>> +                       size_t size)
>>>> +{
>>>> +    DRM_DEBUG_DRIVER("Dummy vram mgr intersects\n");
>>>> +    return true;
>>>> +}
>>>> +
>>>> +static void amdgpu_dummy_vram_mgr_del(struct ttm_resource_manager 
>>>> *man,
>>>> +                struct ttm_resource *res)
>>>> +{
>>>> +    DRM_DEBUG_DRIVER("Dummy vram mgr deleted\n");
>>>> +}
>>>> +
>>>> +static int amdgpu_dummy_vram_mgr_new(struct ttm_resource_manager 
>>>> *man,
>>>> +                   struct ttm_buffer_object *tbo,
>>>> +                   const struct ttm_place *place,
>>>> +                   struct ttm_resource **res)
>>>> +{
>>>> +    DRM_DEBUG_DRIVER("Dummy vram mgr new\n");
>>>> +    return -ENOSPC;
>>>> +}
>>>> +
>>>>   /**
>>>>    * amdgpu_vram_mgr_new - allocate new ranges
>>>>    *
>>>> @@ -817,6 +856,14 @@ static void amdgpu_vram_mgr_debug(struct 
>>>> ttm_resource_manager *man,
>>>>       mutex_unlock(&mgr->lock);
>>>>   }
>>>>   +static const struct ttm_resource_manager_func 
>>>> amdgpu_dummy_vram_mgr_func = {
>>>> +    .alloc    = amdgpu_dummy_vram_mgr_new,
>>>> +    .free    = amdgpu_dummy_vram_mgr_del,
>>>> +    .intersects = amdgpu_dummy_vram_mgr_intersects,
>>>> +    .compatible = amdgpu_dummy_vram_mgr_compatible,
>>>> +    .debug    = amdgpu_dummy_vram_mgr_debug
>>>> +};
>>>> +
>>>>   static const struct ttm_resource_manager_func 
>>>> amdgpu_vram_mgr_func = {
>>>>       .alloc    = amdgpu_vram_mgr_new,
>>>>       .free    = amdgpu_vram_mgr_del,
>>>> @@ -841,17 +888,22 @@ int amdgpu_vram_mgr_init(struct amdgpu_device 
>>>> *adev)
>>>>       ttm_resource_manager_init(man, &adev->mman.bdev,
>>>>                     adev->gmc.real_vram_size);
>>>>   -    man->func = &amdgpu_vram_mgr_func;
>>>> -
>>>> -    err = drm_buddy_init(&mgr->mm, man->size, PAGE_SIZE);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>>       mutex_init(&mgr->lock);
>>>>       INIT_LIST_HEAD(&mgr->reservations_pending);
>>>>       INIT_LIST_HEAD(&mgr->reserved_pages);
>>>>       mgr->default_page_size = PAGE_SIZE;
>>>>   +    if (!adev->gmc.is_app_apu) {
>>>> +        man->func = &amdgpu_vram_mgr_func;
>>>> +
>>>> +        err = drm_buddy_init(&mgr->mm, man->size, PAGE_SIZE);
>>>> +        if (err)
>>>> +            return err;
>>>> +    } else {
>>>> +        man->func = &amdgpu_dummy_vram_mgr_func;
>>>> +        DRM_INFO("Setup dummy vram mgr\n");
>>>> +    }
>>>> +
>>>>       ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_VRAM, 
>>>> &mgr->manager);
>>>>       ttm_resource_manager_set_used(man, true);
>>>>       return 0;
>>>> @@ -886,7 +938,8 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device 
>>>> *adev)
>>>>           drm_buddy_free_list(&mgr->mm, &rsv->allocated);
>>>>           kfree(rsv);
>>>>       }
>>>> -    drm_buddy_fini(&mgr->mm);
>>>> +    if (!adev->gmc.is_app_apu)
>>>> +        drm_buddy_fini(&mgr->mm);
>>>>       mutex_unlock(&mgr->lock);
>>>>         ttm_resource_manager_cleanup(man);
>>>
>


More information about the amd-gfx mailing list