Re: [PATCH 1/1] drm/amdgpu: add and use amdgpu_bo_evict_gtt

["Das, Nirmoy" <nirmoy.das@xxxxxxx>]

On 10/6/2021 4:58 PM, Christian König wrote:
> Am 06.10.21 um 16:45 schrieb Nirmoy Das:
> > Unify BO evicting functionality for VRAM and TT memory
> > types in amdgpu_object.c. Use amdgpu_bo_evict_gtt()
> > for evicting gtt memory similar to how we do that
> > for amdgpu_debugfs_evict_vram().
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  6 +--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 52 +++++++++++++++++++--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  1 +
> >   3 files changed, 50 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > index 5497e2d31d1a..67045983d63d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > @@ -1341,17 +1341,15 @@ static int amdgpu_debugfs_evict_gtt(void 
> > *data, u64 *val)
> >   {
> >       struct amdgpu_device *adev = (struct amdgpu_device *)data;
> >       struct drm_device *dev = adev_to_drm(adev);
> > -    struct ttm_resource_manager *man;
> >       int r;
> >         r = pm_runtime_get_sync(dev->dev);
> >       if (r < 0) {
> > -        pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> > +        pm_runtime_put_autosuspend(dev->dev);
> >           return r;
> >       }
> >   -    man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
> > -    *val = ttm_resource_manager_evict_all(&adev->mman.bdev, man);
> > +    *val = amdgpu_bo_evict_gtt(adev);
> >         pm_runtime_mark_last_busy(dev->dev);
> >       pm_runtime_put_autosuspend(dev->dev);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 4ec904f36ceb..3b8c9cf44d74 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -1005,10 +1005,37 @@ void amdgpu_bo_unpin(struct amdgpu_bo *bo)
> >   }
> >     /**
> > - * amdgpu_bo_evict_vram - evict VRAM buffers
> > + * amdgpu_bo_evict_memory - evict memory buffers
> >    * @adev: amdgpu device object
> > + * @mem_type: evicted BO's memory type
> >    *
> > - * Evicts all VRAM buffers on the lru list of the memory type.
> > + * Evicts all @mem_type buffers on the lru list of the memory type.
> > + *
> > + * Returns:
> > + * 0 for success or a negative error code on failure.
> > + */
> > +static int amdgpu_bo_evict_memory(struct amdgpu_device *adev, int 
> > mem_type)
>
> That function should probably be inside amdgpu_ttm.c instead.
>
> > +{
> > +    struct ttm_resource_manager *man;
> > +
> > +    switch (mem_type) {
> > +    case TTM_PL_VRAM:
> > +    case TTM_PL_TT:
> > +        man = ttm_manager_type(&adev->mman.bdev, mem_type);
> > +        break;
> > +    default:
> > +        DRM_ERROR("Trying to evict invalid memory type\n");
> > +        return -EINVAL;
>
> At least in theory we could do that for OA, GWS and GDS as well.


I will add those and take care of other comments and resend v2.


Thanks,

Nirmoy


> > +    }
> > +
> > +    return ttm_resource_manager_evict_all(&adev->mman.bdev, man);
> > +}
> > +
> > +/**
> > + * amdgpu_bo_evict_vram - evict vram buffers
> > + * @adev: amdgpu device object
> > + *
> > + * Evicts all vram buffers on the lru list of the memory type.
> >    * Mainly used for evicting vram at suspend time.
> >    *
> >    * Returns:
> > @@ -1016,17 +1043,32 @@ void amdgpu_bo_unpin(struct amdgpu_bo *bo)
> >    */
> >   int amdgpu_bo_evict_vram(struct amdgpu_device *adev)
> >   {
> > -    struct ttm_resource_manager *man;
> >         if (adev->in_s3 && (adev->flags & AMD_IS_APU)) {
> >           /* No need to evict vram on APUs for suspend to ram */
> >           return 0;
> >       }
> >   -    man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
> > -    return ttm_resource_manager_evict_all(&adev->mman.bdev, man);
> > +    return amdgpu_bo_evict_memory(adev, TTM_PL_VRAM);
> > +}
> > +
> > +/**
> > + * amdgpu_bo_evict_gtt - evict gtt buffers
> > + * @adev: amdgpu device object
> > + *
> > + * Evicts all gtt buffers on the lru list of the memory type.
> > + * Mainly used for evicting gtt buffers through debugfs.
> > + *
> > + * Returns:
> > + * 0 for success or a negative error code on failure.
> > + */
> > +
> > +int amdgpu_bo_evict_gtt(struct amdgpu_device *adev)
> > +{
>
> I won't add a wrapper for that. This looks like misplaced and overkill.
>
> Christian.
>
> > +    return amdgpu_bo_evict_memory(adev, TTM_PL_TT);
> >   }
> >   +
> >   static const char *amdgpu_vram_names[] = {
> >       "UNKNOWN",
> >       "GDDR1",
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > index 8ff61bad4138..5e9b7710b8e3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > @@ -306,6 +306,7 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo 
> > *bo, u32 domain,
> >                    u64 min_offset, u64 max_offset);
> >   void amdgpu_bo_unpin(struct amdgpu_bo *bo);
> >   int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
> > +int amdgpu_bo_evict_gtt(struct amdgpu_device *adev);
> >   int amdgpu_bo_init(struct amdgpu_device *adev);
> >   void amdgpu_bo_fini(struct amdgpu_device *adev);
> >   int amdgpu_bo_set_tiling_flags(struct amdgpu_bo *bo, u64 
> > tiling_flags);