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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

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.

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);