diff mbox series

drm/amdgpu: Fix GTT size reporting in amdgpu_ioctl

Message ID 20220610135426.670120-1-michel@daenzer.net (mailing list archive)
State New, archived
Headers show
Series drm/amdgpu: Fix GTT size reporting in amdgpu_ioctl | expand

Commit Message

Michel Dänzer June 10, 2022, 1:54 p.m. UTC 
From: Michel Dänzer <mdaenzer@redhat.com>

The commit below changed the TTM manager size unit from pages to
bytes, but failed to adjust the corresponding calculations in
amdgpu_ioctl.

Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Alex Deucher June 10, 2022, 4 p.m. UTC | #1 
Applied.  Thanks!

Alex

On Fri, Jun 10, 2022 at 10:01 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> From: Michel Dänzer <mdaenzer@redhat.com>
>
> The commit below changed the TTM manager size unit from pages to
> bytes, but failed to adjust the corresponding calculations in
> amdgpu_ioctl.
>
> Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 801f6fa692e9..6de63ea6687e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -642,7 +642,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>                             atomic64_read(&adev->visible_pin_size),
>                             vram_gtt.vram_size);
>                 vram_gtt.gtt_size = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT)->size;
> -               vram_gtt.gtt_size *= PAGE_SIZE;
>                 vram_gtt.gtt_size -= atomic64_read(&adev->gart_pin_size);
>                 return copy_to_user(out, &vram_gtt,
>                                     min((size_t)size, sizeof(vram_gtt))) ? -EFAULT : 0;
> @@ -675,7 +674,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>                         mem.cpu_accessible_vram.usable_heap_size * 3 / 4;
>
>                 mem.gtt.total_heap_size = gtt_man->size;
> -               mem.gtt.total_heap_size *= PAGE_SIZE;
>                 mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
>                         atomic64_read(&adev->gart_pin_size);
>                 mem.gtt.heap_usage = ttm_resource_manager_usage(gtt_man);
> --
> 2.36.1
>
Christian König June 11, 2022, 7:19 a.m. UTC | #2 
Am 10.06.22 um 15:54 schrieb Michel Dänzer:
> From: Michel Dänzer <mdaenzer@redhat.com>
>
> The commit below changed the TTM manager size unit from pages to
> bytes, but failed to adjust the corresponding calculations in
> amdgpu_ioctl.
>
> Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>

Ah, WTF! You won't believe how long I have been searching for this one.

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 801f6fa692e9..6de63ea6687e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -642,7 +642,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   			    atomic64_read(&adev->visible_pin_size),
>   			    vram_gtt.vram_size);
>   		vram_gtt.gtt_size = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT)->size;
> -		vram_gtt.gtt_size *= PAGE_SIZE;
>   		vram_gtt.gtt_size -= atomic64_read(&adev->gart_pin_size);
>   		return copy_to_user(out, &vram_gtt,
>   				    min((size_t)size, sizeof(vram_gtt))) ? -EFAULT : 0;
> @@ -675,7 +674,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   			mem.cpu_accessible_vram.usable_heap_size * 3 / 4;
>   
>   		mem.gtt.total_heap_size = gtt_man->size;
> -		mem.gtt.total_heap_size *= PAGE_SIZE;
>   		mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
>   			atomic64_read(&adev->gart_pin_size);
>   		mem.gtt.heap_usage = ttm_resource_manager_usage(gtt_man);
Mike Lothian June 11, 2022, 11:40 a.m. UTC | #3 
Thanks for finding this

I'll have access to my machine on Monday and will close those issues off
once I've tested things

Cheers

Mike

On Sat, 11 Jun 2022, 09:19 Christian König, <
ckoenig.leichtzumerken@gmail.com> wrote:

> Am 10.06.22 um 15:54 schrieb Michel Dänzer:
> > From: Michel Dänzer <mdaenzer@redhat.com>
> >
> > The commit below changed the TTM manager size unit from pages to
> > bytes, but failed to adjust the corresponding calculations in
> > amdgpu_ioctl.
> >
> > Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
> > Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
> > Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
> > Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
>
> Ah, WTF! You won't believe how long I have been searching for this one.
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index 801f6fa692e9..6de63ea6687e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -642,7 +642,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void
> *data, struct drm_file *filp)
> >                           atomic64_read(&adev->visible_pin_size),
> >                           vram_gtt.vram_size);
> >               vram_gtt.gtt_size = ttm_manager_type(&adev->mman.bdev,
> TTM_PL_TT)->size;
> > -             vram_gtt.gtt_size *= PAGE_SIZE;
> >               vram_gtt.gtt_size -= atomic64_read(&adev->gart_pin_size);
> >               return copy_to_user(out, &vram_gtt,
> >                                   min((size_t)size, sizeof(vram_gtt))) ?
> -EFAULT : 0;
> > @@ -675,7 +674,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void
> *data, struct drm_file *filp)
> >                       mem.cpu_accessible_vram.usable_heap_size * 3 / 4;
> >
> >               mem.gtt.total_heap_size = gtt_man->size;
> > -             mem.gtt.total_heap_size *= PAGE_SIZE;
> >               mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
> >                       atomic64_read(&adev->gart_pin_size);
> >               mem.gtt.heap_usage = ttm_resource_manager_usage(gtt_man);
>
>
Michel Dänzer June 13, 2022, 9:11 a.m. UTC | #4 
On 2022-06-11 09:19, Christian König wrote:
> Am 10.06.22 um 15:54 schrieb Michel Dänzer:
>> From: Michel Dänzer <mdaenzer@redhat.com>
>>
>> The commit below changed the TTM manager size unit from pages to
>> bytes, but failed to adjust the corresponding calculations in
>> amdgpu_ioctl.
>>
>> Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
>> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
>> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
>> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
> 
> Ah, WTF! You won't believe how long I have been searching for this one.

Don't feel too bad, similar things have happened to me before. :) Even after Marek's GitLab comment got me on the right track, it still took a fair amount of trial, error & head scratching to put this one to bed.
Mike Lothian June 14, 2022, 8:16 a.m. UTC | #5 
On Mon, 13 Jun 2022 at 10:11, Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2022-06-11 09:19, Christian König wrote:
> > Am 10.06.22 um 15:54 schrieb Michel Dänzer:
> >> From: Michel Dänzer <mdaenzer@redhat.com>
> >>
> >> The commit below changed the TTM manager size unit from pages to
> >> bytes, but failed to adjust the corresponding calculations in
> >> amdgpu_ioctl.
> >>
> >> Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
> >> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1930
> >> Bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6642
> >> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
> >
> > Ah, WTF! You won't believe how long I have been searching for this one.
>
> Don't feel too bad, similar things have happened to me before. :) Even after Marek's GitLab comment got me on the right track, it still took a fair amount of trial, error & head scratching to put this one to bed.
>
>
> --
> Earthling Michel Dänzer            |                  https://redhat.com
> Libre software enthusiast          |         Mesa and Xwayland developer

That's indeed fixed the Tomb Raider / vulkan issue

I'm still seeing some strange warnings and errors on Linus's tree so
I've updated https://gitlab.freedesktop.org/drm/amd/-/issues/1992
https://gitlab.freedesktop.org/drm/amd/-/issues/2034, I'm not sure if
that's Buddy allocator, TTM Bulk moves or a whole new issue

If it's not too late feel free to add my tested by
Chen, Guchun July 5, 2022, 2:28 p.m. UTC | #6 
Hi Alex,

I think we need to revert this patch on amd-staging-drm-next branch, as its base commit like " drm/amdgpu: remove GTT accounting v2" does not present on 5.16. Instead, the series is part of upcoming 5.18 based amd-staging-drm-next branch. Otherwise, incorrect GTT size reporting switched from page to bytes will crash several vulkan APPs.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
Sent: Saturday, June 11, 2022 12:01 AM
To: Michel Dänzer <michel@daenzer.net>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com>; Maling list - DRI developers <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Fix GTT size reporting in amdgpu_ioctl

Applied.  Thanks!

Alex

On Fri, Jun 10, 2022 at 10:01 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> From: Michel Dänzer <mdaenzer@redhat.com>
>
> The commit below changed the TTM manager size unit from pages to 
> bytes, but failed to adjust the corresponding calculations in 
> amdgpu_ioctl.
>
> Fixes: dfa714b88eb0 ("drm/amdgpu: remove GTT accounting v2")
> Bug: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
> ab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1930&amp;data=05%7C01%7C
> guchun.chen%40amd.com%7C28ed180d765c4588474008da4afa68e1%7C3dd8961fe48
> 84e608e11a82d994e183d%7C0%7C0%7C637904736611555668%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&amp;sdata=%2Bmr%2BJWj5q%2BfB04L4hmNSG%2BYpfhny6YayNV
> gt2xty6bo%3D&amp;reserved=0
> Bug: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
> ab.freedesktop.org%2Fmesa%2Fmesa%2F-%2Fissues%2F6642&amp;data=05%7C01%
> 7Cguchun.chen%40amd.com%7C28ed180d765c4588474008da4afa68e1%7C3dd8961fe
> 4884e608e11a82d994e183d%7C0%7C0%7C637904736611555668%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7C3000%7C%7C%7C&amp;sdata=yN1jFKsffHu2Ik2crsrRxGBxCRylXckSj9zILxTZ
> QzE%3D&amp;reserved=0
> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 801f6fa692e9..6de63ea6687e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -642,7 +642,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>                             atomic64_read(&adev->visible_pin_size),
>                             vram_gtt.vram_size);
>                 vram_gtt.gtt_size = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT)->size;
> -               vram_gtt.gtt_size *= PAGE_SIZE;
>                 vram_gtt.gtt_size -= atomic64_read(&adev->gart_pin_size);
>                 return copy_to_user(out, &vram_gtt,
>                                     min((size_t)size, 
> sizeof(vram_gtt))) ? -EFAULT : 0; @@ -675,7 +674,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>                         mem.cpu_accessible_vram.usable_heap_size * 3 / 
> 4;
>
>                 mem.gtt.total_heap_size = gtt_man->size;
> -               mem.gtt.total_heap_size *= PAGE_SIZE;
>                 mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
>                         atomic64_read(&adev->gart_pin_size);
>                 mem.gtt.heap_usage = 
> ttm_resource_manager_usage(gtt_man);
> --
> 2.36.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 801f6fa692e9..6de63ea6687e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -642,7 +642,6 @@  int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 			    atomic64_read(&adev->visible_pin_size),
 			    vram_gtt.vram_size);
 		vram_gtt.gtt_size = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT)->size;
-		vram_gtt.gtt_size *= PAGE_SIZE;
 		vram_gtt.gtt_size -= atomic64_read(&adev->gart_pin_size);
 		return copy_to_user(out, &vram_gtt,
 				    min((size_t)size, sizeof(vram_gtt))) ? -EFAULT : 0;
@@ -675,7 +674,6 @@  int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 			mem.cpu_accessible_vram.usable_heap_size * 3 / 4;
 
 		mem.gtt.total_heap_size = gtt_man->size;
-		mem.gtt.total_heap_size *= PAGE_SIZE;
 		mem.gtt.usable_heap_size = mem.gtt.total_heap_size -
 			atomic64_read(&adev->gart_pin_size);
 		mem.gtt.heap_usage = ttm_resource_manager_usage(gtt_man);