On Mon, Jun 23, 2025 at 01:37:35PM +0100, Matthew Auld wrote:
> +Matt B who is adding clear-on-free support in xe. I'm not sure if we might
> also see something like this.
>
Thanks for the heads up.
> On 23/06/2025 06:52, Arunpravin Paneer Selvam wrote:
> > - Added a handler in DRM buddy manager to reset the cleared
> > flag for the blocks in the freelist.
> >
> > - This is necessary because, upon resuming, the VRAM becomes
> > cluttered with BIOS data, yet the VRAM backend manager
> > believes that everything has been cleared.
> >
> > Signed-off-by: Arunpravin Paneer Selvam <arunpravin.paneersel...@amd.com>
> > Suggested-by: Christian König <christian.koe...@amd.com>
> > Cc: sta...@vger.kernel.org
> > Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3812
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 ++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++
> > drivers/gpu/drm/drm_buddy.c | 24 ++++++++++++++++++++
> > include/drm/drm_buddy.h | 2 ++
> > 4 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index a59f194e3360..eb67d6c97392 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -5193,6 +5193,8 @@ int amdgpu_device_resume(struct drm_device *dev, bool
> > notify_clients)
> > dev->dev->power.disable_depth--;
> > #endif
> > }
> > +
> > + amdgpu_vram_mgr_clear_reset_blocks(&adev->mman.vram_mgr.mm);
> > adev->in_suspend = false;
> > if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D0))
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> > index 1019c5806ec7..e9e2928fa4d1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> > @@ -58,6 +58,11 @@ static inline bool amdgpu_vram_mgr_is_cleared(struct
> > drm_buddy_block *block)
> > return drm_buddy_block_is_clear(block);
> > }
> > +static inline void amdgpu_vram_mgr_clear_reset_blocks(struct drm_buddy *mm)
> > +{
> > + drm_buddy_clear_reset_blocks(mm);
>
> No lock needed?
>
> > +}
> > +
> > static inline struct amdgpu_vram_mgr_resource *
> > to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
> > {
> > diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> > index a1e652b7631d..b5e44867adf2 100644
> > --- a/drivers/gpu/drm/drm_buddy.c
> > +++ b/drivers/gpu/drm/drm_buddy.c
> > @@ -405,6 +405,30 @@ drm_get_buddy(struct drm_buddy_block *block)
> > }
> > EXPORT_SYMBOL(drm_get_buddy);
> > +/**
> > + * drm_buddy_clear_reset_blocks - reset cleared blocks
> > + *
> > + * @mm: DRM buddy manager
> > + *
> > + * Reset all the cleared blocks in the freelist.
> > + */
> > +void drm_buddy_clear_reset_blocks(struct drm_buddy *mm)
> > +{
> > + unsigned int i;
> > +
>
> This might be a good spot to also force merge freed blocks back together,
> for the ones that have the clear vs dirty mismatch. Otherwise with the below
> loop we can have two buddies that are now dirty but don't get merged back
> together? Fairly sure fini() can chuck a warning otherwise. Also a simple
> kunit test for this would be good.
>
> > + for (i = 0; i <= mm->max_order; ++i) {
> > + struct drm_buddy_block *block;
> > +
> > + list_for_each_entry_reverse(block, &mm->free_list[i], link) {
> > + if (drm_buddy_block_is_clear(block)) {
> > + clear_reset(block);
> > + mm->clear_avail -= drm_buddy_block_size(mm,
> > block);
> > + }
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL(drm_buddy_clear_reset_blocks);
> > +
> > /**
> > * drm_buddy_free_block - free a block
> > *
> > diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> > index 9689a7c5dd36..da569dea16b7 100644
> > --- a/include/drm/drm_buddy.h
> > +++ b/include/drm/drm_buddy.h
> > @@ -160,6 +160,8 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
> > u64 new_size,
> > struct list_head *blocks);
> > +void drm_buddy_clear_reset_blocks(struct drm_buddy *mm);
> > +
I think a polarity argument here would be good. Following up if on Intel
GPUs the inverse is true - our VRAM is entirely cleared. Either way
having this function being able to flip the state either way would be
good.
Matt
> > void drm_buddy_free_block(struct drm_buddy *mm, struct drm_buddy_block
> > *block);
> > void drm_buddy_free_list(struct drm_buddy *mm,
>
