Re: [PATCH v2] drm/amdgpu: Fix VRAM BO swap issue

[Christian König <christian.koenig@xxxxxxx>]

Am 22.09.22 um 19:14 schrieb Christian König:
> Am 22.09.22 um 17:26 schrieb Arunpravin Paneer Selvam:
> > DRM buddy manager allocates the contiguous memory requests in
> > a single block or multiple blocks. So for the ttm move operation
> > (incase of low vram memory) we should consider all the blocks to
> > compute the total memory size which compared with the struct
> > ttm_resource num_pages in order to verify that the blocks are
> > contiguous for the eviction process.
> >
> > v2: Added a Fixes tag
> >
> > Fixes: c9cad937c0c5 ("drm/amdgpu: add drm buddy support to amdgpu")
> > Signed-off-by: Arunpravin Paneer Selvam 
> > <Arunpravin.PaneerSelvam@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index b1c455329023..b1223c8e30c9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -426,6 +426,7 @@ static bool amdgpu_mem_visible(struct 
> > amdgpu_device *adev,
> >   {
> >       uint64_t mem_size = (u64)mem->num_pages << PAGE_SHIFT;
> >       struct amdgpu_res_cursor cursor;
> > +    u64 start, size, total_size = 0;
> >         if (mem->mem_type == TTM_PL_SYSTEM ||
> >           mem->mem_type == TTM_PL_TT)
> > @@ -435,8 +436,23 @@ static bool amdgpu_mem_visible(struct 
> > amdgpu_device *adev,
> >         amdgpu_res_first(mem, 0, mem_size, &cursor);
> >   -    /* ttm_resource_ioremap only supports contiguous memory */
> > -    if (cursor.size != mem_size)
> > +    do {
> > +        start = cursor.start;
> > +        size = cursor.size;
> > +
> > +        total_size += size;
> > +
> > +        amdgpu_res_next(&cursor, cursor.size);
> > +
> > +        if (!cursor.remaining)
> > +            break;
> > +
> > +        /* ttm_resource_ioremap only supports contiguous memory */
> > +        if (start + size != cursor.start)
> > +            return false;
> > +    } while (1);
> > +
> > +    if (total_size != mem_size)
> >           return false;
>
> I would completely drop this extra check.
>
> >       return cursor.start + cursor.size <= adev->gmc.visible_vram_size;
>
> Instead of this you should be able to do all of this in one go.
>
> Something like this here should work:
>
> amdgpu_res_first(...
> end = cursor.start + cursor.size;
> do (
>     amdgpu_res_next(....
>     if (end != cursor.start)
>         return false;
>     end = cursor.start + cursor.size;
> } while (cursor.remaining);

My fault, this should have been

while (cursor.remaining) {
...
}

Regards,
Christian.

> return end <= visible_vram_size;
>
> Saves a bit of extra calculations and variables.
>
> Regards,
> Christian.