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

[Felix Kuehling <felix.kuehling@xxxxxxx>]

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@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxx>
> > > > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > ---
> > > >   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);