All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: don't limit gtt size on apus
@ 2021-01-05 22:03 Joshua Ashton
  2021-01-05 22:10 ` Alex Deucher
  0 siblings, 1 reply; 16+ messages in thread
From: Joshua Ashton @ 2021-01-05 22:03 UTC (permalink / raw)
  To: amd-gfx, christian.koenig; +Cc: Joshua Ashton

Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
according to system memory size only""), the GTT size was limited by
3GiB or VRAM size.

This is problematic on APUs, especially with a small carveout
which can be as low as a fixed 128MiB, as there would be very a limited
3GiB available for video memory.
This obviously does not meet the demands of modern applications.

This patch makes it so the GTT size heuristic always uses 3/4ths of
the system memory size on APUs (limiting the size by 3GiB/VRAM size
only on devices with dedicated video memory).

Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size according to
system memory size only")

Signed-off-by: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 72efd579ec5e..a5a41e9272d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
 
 /**
  * DOC: gttsize (int)
- * Restrict the size of GTT domain in MiB for testing. The default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
- * otherwise 3/4 RAM size).
+ * Restrict the size of GTT domain in MiB for testing. The default is -1 (On APUs this is 3/4th
+ * of the system memory; on dGPUs this is 3GiB or VRAM sized, whichever is bigger,
+ * with an upper bound of 3/4th of system memory.
  */
 MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = auto)");
 module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 4d8f19ab1014..294f26f4f310 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 		struct sysinfo si;
 
 		si_meminfo(&si);
-		gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 20),
-			       adev->gmc.mc_vram_size),
-			       ((uint64_t)si.totalram * si.mem_unit * 3/4));
+		gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
+		/* If we have dedicated memory, limit our GTT size to
+		 * 3GiB or VRAM size, whichever is bigger
+		 */
+		if (!(adev->flags & AMD_IS_APU)) {
+			gtt_size = min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
+				adev->gmc.mc_vram_size),
+				gtt_size);
+		}
 	}
 	else
 		gtt_size = (uint64_t)amdgpu_gtt_size << 20;
-- 
2.30.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-05 22:03 [PATCH] drm/amdgpu: don't limit gtt size on apus Joshua Ashton
@ 2021-01-05 22:10 ` Alex Deucher
  2021-01-05 22:31   ` Joshua Ashton
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Deucher @ 2021-01-05 22:10 UTC (permalink / raw)
  To: Joshua Ashton; +Cc: Christian Koenig, amd-gfx list

On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>
> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
> according to system memory size only""), the GTT size was limited by
> 3GiB or VRAM size.

The commit in question was to fix a hang with certain tests on APUs.
That should be tested again before we re-enable this.  If it is fixed,
we should just revert the revert rather than special case dGPUs.

Alex


>
> This is problematic on APUs, especially with a small carveout
> which can be as low as a fixed 128MiB, as there would be very a limited
> 3GiB available for video memory.
> This obviously does not meet the demands of modern applications.
>
> This patch makes it so the GTT size heuristic always uses 3/4ths of
> the system memory size on APUs (limiting the size by 3GiB/VRAM size
> only on devices with dedicated video memory).
>
> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size according to
> system memory size only")
>
> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 72efd579ec5e..a5a41e9272d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
>
>  /**
>   * DOC: gttsize (int)
> - * Restrict the size of GTT domain in MiB for testing. The default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
> - * otherwise 3/4 RAM size).
> + * Restrict the size of GTT domain in MiB for testing. The default is -1 (On APUs this is 3/4th
> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, whichever is bigger,
> + * with an upper bound of 3/4th of system memory.
>   */
>  MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = auto)");
>  module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 4d8f19ab1014..294f26f4f310 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>                 struct sysinfo si;
>
>                 si_meminfo(&si);
> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 20),
> -                              adev->gmc.mc_vram_size),
> -                              ((uint64_t)si.totalram * si.mem_unit * 3/4));
> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
> +               /* If we have dedicated memory, limit our GTT size to
> +                * 3GiB or VRAM size, whichever is bigger
> +                */
> +               if (!(adev->flags & AMD_IS_APU)) {
> +                       gtt_size = min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
> +                               adev->gmc.mc_vram_size),
> +                               gtt_size);
> +               }
>         }
>         else
>                 gtt_size = (uint64_t)amdgpu_gtt_size << 20;
> --
> 2.30.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-05 22:10 ` Alex Deucher
@ 2021-01-05 22:31   ` Joshua Ashton
  2021-01-06  7:52     ` Christian König
  2021-01-08  9:27     ` Paul Menzel
  0 siblings, 2 replies; 16+ messages in thread
From: Joshua Ashton @ 2021-01-05 22:31 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Christian Koenig, amd-gfx list

On 1/5/21 10:10 PM, Alex Deucher wrote:
> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>
>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>> according to system memory size only""), the GTT size was limited by
>> 3GiB or VRAM size.
> 
> The commit in question was to fix a hang with certain tests on APUs.
> That should be tested again before we re-enable this.  If it is fixed,
> we should just revert the revert rather than special case dGPUs.
> 
> Alex
> 

I think the commit before the revert (ba851eed895c) has some fundamental 
problems:

It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
wouldn't fit into say, 1GiB or 2GiB of available RAM.

Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
sense also and is a sensible limit to avoid silly situations with 
overallocation and potential OOM.

This patch solves both of those issues.

- Joshie 🐸✨

> 
>>
>> This is problematic on APUs, especially with a small carveout
>> which can be as low as a fixed 128MiB, as there would be very a limited
>> 3GiB available for video memory.
>> This obviously does not meet the demands of modern applications.
>>
>> This patch makes it so the GTT size heuristic always uses 3/4ths of
>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
>> only on devices with dedicated video memory).
>>
>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size according to
>> system memory size only")
>>
>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 72efd579ec5e..a5a41e9272d6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, uint, 0600);
>>
>>   /**
>>    * DOC: gttsize (int)
>> - * Restrict the size of GTT domain in MiB for testing. The default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>> - * otherwise 3/4 RAM size).
>> + * Restrict the size of GTT domain in MiB for testing. The default is -1 (On APUs this is 3/4th
>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, whichever is bigger,
>> + * with an upper bound of 3/4th of system memory.
>>    */
>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 = auto)");
>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 4d8f19ab1014..294f26f4f310 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>>                  struct sysinfo si;
>>
>>                  si_meminfo(&si);
>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 20),
>> -                              adev->gmc.mc_vram_size),
>> -                              ((uint64_t)si.totalram * si.mem_unit * 3/4));
>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
>> +               /* If we have dedicated memory, limit our GTT size to
>> +                * 3GiB or VRAM size, whichever is bigger
>> +                */
>> +               if (!(adev->flags & AMD_IS_APU)) {
>> +                       gtt_size = min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>> +                               adev->gmc.mc_vram_size),
>> +                               gtt_size);
>> +               }
>>          }
>>          else
>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>> --
>> 2.30.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-05 22:31   ` Joshua Ashton
@ 2021-01-06  7:52     ` Christian König
  2021-01-06 12:47       ` Joshua Ashton
  2021-01-08  9:27     ` Paul Menzel
  1 sibling, 1 reply; 16+ messages in thread
From: Christian König @ 2021-01-06  7:52 UTC (permalink / raw)
  To: Joshua Ashton, Alex Deucher; +Cc: amd-gfx list

Am 05.01.21 um 23:31 schrieb Joshua Ashton:
> On 1/5/21 10:10 PM, Alex Deucher wrote:
>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>>
>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>> according to system memory size only""), the GTT size was limited by
>>> 3GiB or VRAM size.
>>
>> The commit in question was to fix a hang with certain tests on APUs.
>> That should be tested again before we re-enable this.  If it is fixed,
>> we should just revert the revert rather than special case dGPUs.
>>
>> Alex
>>
>
> I think the commit before the revert (ba851eed895c) has some 
> fundamental problems:
>
> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>
> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
> sense also and is a sensible limit to avoid silly situations with 
> overallocation and potential OOM.
>
> This patch solves both of those issues.

No, Alex is right this approach was already tried and it causes problems.

Additional to that why should this be an issue? Even when VRAM is very 
small on APUs we still use 3GiB of GTT.

Regards,
Christian.

>
> - Joshie 🐸✨
>
>>
>>>
>>> This is problematic on APUs, especially with a small carveout
>>> which can be as low as a fixed 128MiB, as there would be very a limited
>>> 3GiB available for video memory.
>>> This obviously does not meet the demands of modern applications.
>>>
>>> This patch makes it so the GTT size heuristic always uses 3/4ths of
>>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
>>> only on devices with dedicated video memory).
>>>
>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size according to
>>> system memory size only")
>>>
>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 72efd579ec5e..a5a41e9272d6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, 
>>> uint, 0600);
>>>
>>>   /**
>>>    * DOC: gttsize (int)
>>> - * Restrict the size of GTT domain in MiB for testing. The default 
>>> is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>> - * otherwise 3/4 RAM size).
>>> + * Restrict the size of GTT domain in MiB for testing. The default 
>>> is -1 (On APUs this is 3/4th
>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, 
>>> whichever is bigger,
>>> + * with an upper bound of 3/4th of system memory.
>>>    */
>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 
>>> = auto)");
>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 4d8f19ab1014..294f26f4f310 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>>>                  struct sysinfo si;
>>>
>>>                  si_meminfo(&si);
>>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 20),
>>> -                              adev->gmc.mc_vram_size),
>>> -                              ((uint64_t)si.totalram * si.mem_unit 
>>> * 3/4));
>>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
>>> +               /* If we have dedicated memory, limit our GTT size to
>>> +                * 3GiB or VRAM size, whichever is bigger
>>> +                */
>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>> +                       gtt_size = 
>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>> +                               adev->gmc.mc_vram_size),
>>> +                               gtt_size);
>>> +               }
>>>          }
>>>          else
>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>> -- 
>>> 2.30.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0dfe0d4b6f694ef4bd3c08d8b1c9ab0b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454827005214704%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eT0XmwNPzBC1mODZQSeHLlcHzqU2Dtfb1E3goJEy2lA%3D&amp;reserved=0 
>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06  7:52     ` Christian König
@ 2021-01-06 12:47       ` Joshua Ashton
  2021-01-06 12:54         ` Christian König
  0 siblings, 1 reply; 16+ messages in thread
From: Joshua Ashton @ 2021-01-06 12:47 UTC (permalink / raw)
  To: Christian König, Alex Deucher; +Cc: amd-gfx list



On 1/6/21 7:52 AM, Christian König wrote:
> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>>>
>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>>> according to system memory size only""), the GTT size was limited by
>>>> 3GiB or VRAM size.
>>>
>>> The commit in question was to fix a hang with certain tests on APUs.
>>> That should be tested again before we re-enable this.  If it is fixed,
>>> we should just revert the revert rather than special case dGPUs.
>>>
>>> Alex
>>>
>>
>> I think the commit before the revert (ba851eed895c) has some 
>> fundamental problems:
>>
>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>
>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
>> sense also and is a sensible limit to avoid silly situations with 
>> overallocation and potential OOM.
>>
>> This patch solves both of those issues.
> 
> No, Alex is right this approach was already tried and it causes problems.
> 
> Additional to that why should this be an issue? Even when VRAM is very 
> small on APUs we still use 3GiB of GTT.
> 
> Regards,
> Christian.

The problem is that 3GiB of GTT isn't enough for most modern games. My 
laptop has a 128MiB carveout which is not possible to be configured in 
the BIOS so I am stuck with that size without extra kernel parameters 
which shouldn't be necessary.

If you dislike the approach of keeping the extra check for dGPUs and 
limiting GTT there, then I would say that we should use
	gtt_size = 3/4ths system memory
for all devices instead of
	gtt_size = max(3/4ths system memory, 3GiB)
as it was before the revert, as it is problematic on systems with < 3GiB 
of system memory.

- Joshie 🐸✨

> 
>>
>> - Joshie 🐸✨
>>
>>>
>>>>
>>>> This is problematic on APUs, especially with a small carveout
>>>> which can be as low as a fixed 128MiB, as there would be very a limited
>>>> 3GiB available for video memory.
>>>> This obviously does not meet the demands of modern applications.
>>>>
>>>> This patch makes it so the GTT size heuristic always uses 3/4ths of
>>>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
>>>> only on devices with dedicated video memory).
>>>>
>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size according to
>>>> system memory size only")
>>>>
>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, 
>>>> uint, 0600);
>>>>
>>>>   /**
>>>>    * DOC: gttsize (int)
>>>> - * Restrict the size of GTT domain in MiB for testing. The default 
>>>> is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>>> - * otherwise 3/4 RAM size).
>>>> + * Restrict the size of GTT domain in MiB for testing. The default 
>>>> is -1 (On APUs this is 3/4th
>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, 
>>>> whichever is bigger,
>>>> + * with an upper bound of 3/4th of system memory.
>>>>    */
>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes (-1 
>>>> = auto)");
>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 4d8f19ab1014..294f26f4f310 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>>>>                  struct sysinfo si;
>>>>
>>>>                  si_meminfo(&si);
>>>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 20),
>>>> -                              adev->gmc.mc_vram_size),
>>>> -                              ((uint64_t)si.totalram * si.mem_unit 
>>>> * 3/4));
>>>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
>>>> +               /* If we have dedicated memory, limit our GTT size to
>>>> +                * 3GiB or VRAM size, whichever is bigger
>>>> +                */
>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>>> +                       gtt_size = 
>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>>> +                               adev->gmc.mc_vram_size),
>>>> +                               gtt_size);
>>>> +               }
>>>>          }
>>>>          else
>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>>> -- 
>>>> 2.30.0
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0dfe0d4b6f694ef4bd3c08d8b1c9ab0b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454827005214704%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eT0XmwNPzBC1mODZQSeHLlcHzqU2Dtfb1E3goJEy2lA%3D&amp;reserved=0 
>>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 12:47       ` Joshua Ashton
@ 2021-01-06 12:54         ` Christian König
  2021-01-06 13:02           ` Bas Nieuwenhuizen
  2021-01-06 13:06           ` Joshua Ashton
  0 siblings, 2 replies; 16+ messages in thread
From: Christian König @ 2021-01-06 12:54 UTC (permalink / raw)
  To: Joshua Ashton, Alex Deucher; +Cc: amd-gfx list

Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>
>
> On 1/6/21 7:52 AM, Christian König wrote:
>> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>>>>
>>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>>>> according to system memory size only""), the GTT size was limited by
>>>>> 3GiB or VRAM size.
>>>>
>>>> The commit in question was to fix a hang with certain tests on APUs.
>>>> That should be tested again before we re-enable this.  If it is fixed,
>>>> we should just revert the revert rather than special case dGPUs.
>>>>
>>>> Alex
>>>>
>>>
>>> I think the commit before the revert (ba851eed895c) has some 
>>> fundamental problems:
>>>
>>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
>>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>>
>>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
>>> sense also and is a sensible limit to avoid silly situations with 
>>> overallocation and potential OOM.
>>>
>>> This patch solves both of those issues.
>>
>> No, Alex is right this approach was already tried and it causes 
>> problems.
>>
>> Additional to that why should this be an issue? Even when VRAM is 
>> very small on APUs we still use 3GiB of GTT.
>>
>> Regards,
>> Christian.
>
> The problem is that 3GiB of GTT isn't enough for most modern games.

You seem to misunderstand what the GTT size means here. This is the 
amount of memory an application can lock down in a single command 
submissions.

It is still possible for the game to use all of system memory for 
textures etc... it can just happen that some buffers are temporary 
marked as inaccessible for the GPU.

> My laptop has a 128MiB carveout which is not possible to be configured 
> in the BIOS so I am stuck with that size without extra kernel 
> parameters which shouldn't be necessary.

Did you ran into problems without the parameter?

>
> If you dislike the approach of keeping the extra check for dGPUs and 
> limiting GTT there, then I would say that we should use
>     gtt_size = 3/4ths system memory
> for all devices instead of
>     gtt_size = max(3/4ths system memory, 3GiB)
> as it was before the revert, as it is problematic on systems with < 
> 3GiB of system memory.

Yeah, that's indeed not a good idea.

Regards,
Christian.

>
> - Joshie 🐸✨
>
>>
>>>
>>> - Joshie 🐸✨
>>>
>>>>
>>>>>
>>>>> This is problematic on APUs, especially with a small carveout
>>>>> which can be as low as a fixed 128MiB, as there would be very a 
>>>>> limited
>>>>> 3GiB available for video memory.
>>>>> This obviously does not meet the demands of modern applications.
>>>>>
>>>>> This patch makes it so the GTT size heuristic always uses 3/4ths of
>>>>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
>>>>> only on devices with dedicated video memory).
>>>>>
>>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size 
>>>>> according to
>>>>> system memory size only")
>>>>>
>>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, 
>>>>> uint, 0600);
>>>>>
>>>>>   /**
>>>>>    * DOC: gttsize (int)
>>>>> - * Restrict the size of GTT domain in MiB for testing. The 
>>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>>>> - * otherwise 3/4 RAM size).
>>>>> + * Restrict the size of GTT domain in MiB for testing. The 
>>>>> default is -1 (On APUs this is 3/4th
>>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, 
>>>>> whichever is bigger,
>>>>> + * with an upper bound of 3/4th of system memory.
>>>>>    */
>>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes 
>>>>> (-1 = auto)");
>>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> index 4d8f19ab1014..294f26f4f310 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device 
>>>>> *adev)
>>>>>                  struct sysinfo si;
>>>>>
>>>>>                  si_meminfo(&si);
>>>>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 
>>>>> 20),
>>>>> -                              adev->gmc.mc_vram_size),
>>>>> -                              ((uint64_t)si.totalram * 
>>>>> si.mem_unit * 3/4));
>>>>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
>>>>> +               /* If we have dedicated memory, limit our GTT size to
>>>>> +                * 3GiB or VRAM size, whichever is bigger
>>>>> +                */
>>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>>>> +                       gtt_size = 
>>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>>>> + adev->gmc.mc_vram_size),
>>>>> +                               gtt_size);
>>>>> +               }
>>>>>          }
>>>>>          else
>>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>>>> -- 
>>>>> 2.30.0
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0 
>>>>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 12:54         ` Christian König
@ 2021-01-06 13:02           ` Bas Nieuwenhuizen
  2021-01-06 13:05             ` Christian König
  2021-01-06 13:06           ` Joshua Ashton
  1 sibling, 1 reply; 16+ messages in thread
From: Bas Nieuwenhuizen @ 2021-01-06 13:02 UTC (permalink / raw)
  To: Christian König; +Cc: Alex Deucher, amd-gfx list, Joshua Ashton


[-- Attachment #1.1: Type: text/plain, Size: 7235 bytes --]

On Wed, Jan 6, 2021 at 1:54 PM Christian König <christian.koenig@amd.com>
wrote:

> Am 06.01.21 um 13:47 schrieb Joshua Ashton:
> >
> >
> > On 1/6/21 7:52 AM, Christian König wrote:
> >> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
> >>> On 1/5/21 10:10 PM, Alex Deucher wrote:
> >>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es>
> wrote:
> >>>>>
> >>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
> >>>>> according to system memory size only""), the GTT size was limited by
> >>>>> 3GiB or VRAM size.
> >>>>
> >>>> The commit in question was to fix a hang with certain tests on APUs.
> >>>> That should be tested again before we re-enable this.  If it is fixed,
> >>>> we should just revert the revert rather than special case dGPUs.
> >>>>
> >>>> Alex
> >>>>
> >>>
> >>> I think the commit before the revert (ba851eed895c) has some
> >>> fundamental problems:
> >>>
> >>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that
> >>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
> >>>
> >>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes
> >>> sense also and is a sensible limit to avoid silly situations with
> >>> overallocation and potential OOM.
> >>>
> >>> This patch solves both of those issues.
> >>
> >> No, Alex is right this approach was already tried and it causes
> >> problems.
> >>
> >> Additional to that why should this be an issue? Even when VRAM is
> >> very small on APUs we still use 3GiB of GTT.
> >>
> >> Regards,
> >> Christian.
> >
> > The problem is that 3GiB of GTT isn't enough for most modern games.
>
> You seem to misunderstand what the GTT size means here. This is the
> amount of memory an application can lock down in a single command
> submissions.
>
> It is still possible for the game to use all of system memory for
> textures etc... it can just happen that some buffers are temporary
> marked as inaccessible for the GPU.
>

For Vulkan we (both RADV and AMDVLK) use GTT as the total size. Usage in
modern games is essentially "bindless" so there is no way to track at a
per-submission level what memory needs to be resident. (and even with
tracking applications are allowed to use all the memory in a single draw
call, which would be unsplittable anyway ...)


> > My laptop has a 128MiB carveout which is not possible to be configured
> > in the BIOS so I am stuck with that size without extra kernel
> > parameters which shouldn't be necessary.
>
> Did you ran into problems without the parameter?
>
> >
> > If you dislike the approach of keeping the extra check for dGPUs and
> > limiting GTT there, then I would say that we should use
> >     gtt_size = 3/4ths system memory
> > for all devices instead of
> >     gtt_size = max(3/4ths system memory, 3GiB)
> > as it was before the revert, as it is problematic on systems with <
> > 3GiB of system memory.
>
> Yeah, that's indeed not a good idea.
>
> Regards,
> Christian.
>
> >
> > - Joshie 🐸✨
> >
> >>
> >>>
> >>> - Joshie 🐸✨
> >>>
> >>>>
> >>>>>
> >>>>> This is problematic on APUs, especially with a small carveout
> >>>>> which can be as low as a fixed 128MiB, as there would be very a
> >>>>> limited
> >>>>> 3GiB available for video memory.
> >>>>> This obviously does not meet the demands of modern applications.
> >>>>>
> >>>>> This patch makes it so the GTT size heuristic always uses 3/4ths of
> >>>>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
> >>>>> only on devices with dedicated video memory).
> >>>>>
> >>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size
> >>>>> according to
> >>>>> system memory size only")
> >>>>>
> >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> >>>>> ---
> >>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
> >>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
> >>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>> index 72efd579ec5e..a5a41e9272d6 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size,
> >>>>> uint, 0600);
> >>>>>
> >>>>>   /**
> >>>>>    * DOC: gttsize (int)
> >>>>> - * Restrict the size of GTT domain in MiB for testing. The
> >>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
> >>>>> - * otherwise 3/4 RAM size).
> >>>>> + * Restrict the size of GTT domain in MiB for testing. The
> >>>>> default is -1 (On APUs this is 3/4th
> >>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized,
> >>>>> whichever is bigger,
> >>>>> + * with an upper bound of 3/4th of system memory.
> >>>>>    */
> >>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes
> >>>>> (-1 = auto)");
> >>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>>>> index 4d8f19ab1014..294f26f4f310 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device
> >>>>> *adev)
> >>>>>                  struct sysinfo si;
> >>>>>
> >>>>>                  si_meminfo(&si);
> >>>>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB <<
> >>>>> 20),
> >>>>> -                              adev->gmc.mc_vram_size),
> >>>>> -                              ((uint64_t)si.totalram *
> >>>>> si.mem_unit * 3/4));
> >>>>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
> >>>>> +               /* If we have dedicated memory, limit our GTT size to
> >>>>> +                * 3GiB or VRAM size, whichever is bigger
> >>>>> +                */
> >>>>> +               if (!(adev->flags & AMD_IS_APU)) {
> >>>>> +                       gtt_size =
> >>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
> >>>>> + adev->gmc.mc_vram_size),
> >>>>> +                               gtt_size);
> >>>>> +               }
> >>>>>          }
> >>>>>          else
> >>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
> >>>>> --
> >>>>> 2.30.0
> >>>>>
> >>>>> _______________________________________________
> >>>>> amd-gfx mailing list
> >>>>> amd-gfx@lists.freedesktop.org
> >>>>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0
> >>>>>
> >>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 10963 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 13:02           ` Bas Nieuwenhuizen
@ 2021-01-06 13:05             ` Christian König
  2021-01-06 13:17               ` Joshua Ashton
  0 siblings, 1 reply; 16+ messages in thread
From: Christian König @ 2021-01-06 13:05 UTC (permalink / raw)
  To: Bas Nieuwenhuizen, Christian König
  Cc: Alex Deucher, Joshua Ashton, amd-gfx list


[-- Attachment #1.1: Type: text/plain, Size: 8711 bytes --]

Am 06.01.21 um 14:02 schrieb Bas Nieuwenhuizen:
>
>
> On Wed, Jan 6, 2021 at 1:54 PM Christian König 
> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>
>     Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>     >
>     >
>     > On 1/6/21 7:52 AM, Christian König wrote:
>     >> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>     >>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>     >>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton
>     <joshua@froggi.es <mailto:joshua@froggi.es>> wrote:
>     >>>>>
>     >>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>     >>>>> according to system memory size only""), the GTT size was
>     limited by
>     >>>>> 3GiB or VRAM size.
>     >>>>
>     >>>> The commit in question was to fix a hang with certain tests
>     on APUs.
>     >>>> That should be tested again before we re-enable this.  If it
>     is fixed,
>     >>>> we should just revert the revert rather than special case dGPUs.
>     >>>>
>     >>>> Alex
>     >>>>
>     >>>
>     >>> I think the commit before the revert (ba851eed895c) has some
>     >>> fundamental problems:
>     >>>
>     >>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if
>     that
>     >>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>     >>>
>     >>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs
>     makes
>     >>> sense also and is a sensible limit to avoid silly situations with
>     >>> overallocation and potential OOM.
>     >>>
>     >>> This patch solves both of those issues.
>     >>
>     >> No, Alex is right this approach was already tried and it causes
>     >> problems.
>     >>
>     >> Additional to that why should this be an issue? Even when VRAM is
>     >> very small on APUs we still use 3GiB of GTT.
>     >>
>     >> Regards,
>     >> Christian.
>     >
>     > The problem is that 3GiB of GTT isn't enough for most modern games.
>
>     You seem to misunderstand what the GTT size means here. This is the
>     amount of memory an application can lock down in a single command
>     submissions.
>
>     It is still possible for the game to use all of system memory for
>     textures etc... it can just happen that some buffers are temporary
>     marked as inaccessible for the GPU.
>
>
> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. Usage 
> in modern games is essentially "bindless" so there is no way to track 
> at a per-submission level what memory needs to be resident. (and even 
> with tracking applications are allowed to use all the memory in a 
> single draw call, which would be unsplittable anyway ...)

Yeah, that is a really good point.

The issue is that we need some limitation since 3/4 of system memory is 
way to much and the max texture size test in piglit can cause a system 
crash.

The alternative is a better OOM handling, so that an application which 
uses to much system memory through the driver stack has a more likely 
chance to get killed. Cause currently that is either X or Wayland :(

Christian.

>
>
>     > My laptop has a 128MiB carveout which is not possible to be
>     configured
>     > in the BIOS so I am stuck with that size without extra kernel
>     > parameters which shouldn't be necessary.
>
>     Did you ran into problems without the parameter?
>
>     >
>     > If you dislike the approach of keeping the extra check for dGPUs
>     and
>     > limiting GTT there, then I would say that we should use
>     >     gtt_size = 3/4ths system memory
>     > for all devices instead of
>     >     gtt_size = max(3/4ths system memory, 3GiB)
>     > as it was before the revert, as it is problematic on systems with <
>     > 3GiB of system memory.
>
>     Yeah, that's indeed not a good idea.
>
>     Regards,
>     Christian.
>
>     >
>     > - Joshie 🐸✨
>     >
>     >>
>     >>>
>     >>> - Joshie 🐸✨
>     >>>
>     >>>>
>     >>>>>
>     >>>>> This is problematic on APUs, especially with a small carveout
>     >>>>> which can be as low as a fixed 128MiB, as there would be very a
>     >>>>> limited
>     >>>>> 3GiB available for video memory.
>     >>>>> This obviously does not meet the demands of modern applications.
>     >>>>>
>     >>>>> This patch makes it so the GTT size heuristic always uses
>     3/4ths of
>     >>>>> the system memory size on APUs (limiting the size by
>     3GiB/VRAM size
>     >>>>> only on devices with dedicated video memory).
>     >>>>>
>     >>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size
>     >>>>> according to
>     >>>>> system memory size only")
>     >>>>>
>     >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es
>     <mailto:joshua@froggi.es>>
>     >>>>> ---
>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>     >>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>     >>>>>
>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>     >>>>> index 72efd579ec5e..a5a41e9272d6 100644
>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>     >>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize,
>     amdgpu_gart_size,
>     >>>>> uint, 0600);
>     >>>>>
>     >>>>>   /**
>     >>>>>    * DOC: gttsize (int)
>     >>>>> - * Restrict the size of GTT domain in MiB for testing. The
>     >>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>     >>>>> - * otherwise 3/4 RAM size).
>     >>>>> + * Restrict the size of GTT domain in MiB for testing. The
>     >>>>> default is -1 (On APUs this is 3/4th
>     >>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized,
>     >>>>> whichever is bigger,
>     >>>>> + * with an upper bound of 3/4th of system memory.
>     >>>>>    */
>     >>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in
>     megabytes
>     >>>>> (-1 = auto)");
>     >>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>     >>>>> index 4d8f19ab1014..294f26f4f310 100644
>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>     >>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device
>     >>>>> *adev)
>     >>>>>                  struct sysinfo si;
>     >>>>>
>     >>>>>                  si_meminfo(&si);
>     >>>>> -               gtt_size =
>     min(max((AMDGPU_DEFAULT_GTT_SIZE_MB <<
>     >>>>> 20),
>     >>>>> - adev->gmc.mc_vram_size),
>     >>>>> - ((uint64_t)si.totalram *
>     >>>>> si.mem_unit * 3/4));
>     >>>>> +               gtt_size = (uint64_t)si.totalram *
>     si.mem_unit * 3/4;
>     >>>>> +               /* If we have dedicated memory, limit our
>     GTT size to
>     >>>>> +                * 3GiB or VRAM size, whichever is bigger
>     >>>>> +                */
>     >>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>     >>>>> +                       gtt_size =
>     >>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>     >>>>> + adev->gmc.mc_vram_size),
>     >>>>> + gtt_size);
>     >>>>> +               }
>     >>>>>          }
>     >>>>>          else
>     >>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>     >>>>> --
>     >>>>> 2.30.0
>     >>>>>
>     >>>>> _______________________________________________
>     >>>>> amd-gfx mailing list
>     >>>>> amd-gfx@lists.freedesktop.org
>     <mailto:amd-gfx@lists.freedesktop.org>
>     >>>>>
>     https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0
>
>     >>>>>
>     >>
>
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 15653 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 12:54         ` Christian König
  2021-01-06 13:02           ` Bas Nieuwenhuizen
@ 2021-01-06 13:06           ` Joshua Ashton
  1 sibling, 0 replies; 16+ messages in thread
From: Joshua Ashton @ 2021-01-06 13:06 UTC (permalink / raw)
  To: Christian König, Alex Deucher; +Cc: amd-gfx list



On 1/6/21 12:54 PM, Christian König wrote:
> Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>>
>>
>> On 1/6/21 7:52 AM, Christian König wrote:
>>> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>>>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>>>>>
>>>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>>>>> according to system memory size only""), the GTT size was limited by
>>>>>> 3GiB or VRAM size.
>>>>>
>>>>> The commit in question was to fix a hang with certain tests on APUs.
>>>>> That should be tested again before we re-enable this.  If it is fixed,
>>>>> we should just revert the revert rather than special case dGPUs.
>>>>>
>>>>> Alex
>>>>>
>>>>
>>>> I think the commit before the revert (ba851eed895c) has some 
>>>> fundamental problems:
>>>>
>>>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
>>>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>>>
>>>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
>>>> sense also and is a sensible limit to avoid silly situations with 
>>>> overallocation and potential OOM.
>>>>
>>>> This patch solves both of those issues.
>>>
>>> No, Alex is right this approach was already tried and it causes 
>>> problems.
>>>
>>> Additional to that why should this be an issue? Even when VRAM is 
>>> very small on APUs we still use 3GiB of GTT.
>>>
>>> Regards,
>>> Christian.
>>
>> The problem is that 3GiB of GTT isn't enough for most modern games.
> 
> You seem to misunderstand what the GTT size means here. This is the 
> amount of memory an application can lock down in a single command 
> submissions.
> 
> It is still possible for the game to use all of system memory for 
> textures etc... it can just happen that some buffers are temporary 
> marked as inaccessible for the GPU.

In Vulkan, command buffers are explicit and the amount of memory the app 
uses is not trackable at a command buffer level due to bindless.

This means that we can't magically split command buffers like in GL if 
too much memory is being used by a single submission.

This means that the only two visible heaps available to AMD APUs in RADV 
right now are the carveout and GTT. As I understand it there is no other 
way to use more memory in APIs with explicit cmd buffering & bindless.

> 
>> My laptop has a 128MiB carveout which is not possible to be configured 
>> in the BIOS so I am stuck with that size without extra kernel 
>> parameters which shouldn't be necessary.
> 
> Did you ran into problems without the parameter?
> 
>>
>> If you dislike the approach of keeping the extra check for dGPUs and 
>> limiting GTT there, then I would say that we should use
>>     gtt_size = 3/4ths system memory
>> for all devices instead of
>>     gtt_size = max(3/4ths system memory, 3GiB)
>> as it was before the revert, as it is problematic on systems with < 
>> 3GiB of system memory.
> 
> Yeah, that's indeed not a good idea.
> 
> Regards,
> Christian.
> 
>>
>> - Joshie 🐸✨
>>
>>>
>>>>
>>>> - Joshie 🐸✨
>>>>
>>>>>
>>>>>>
>>>>>> This is problematic on APUs, especially with a small carveout
>>>>>> which can be as low as a fixed 128MiB, as there would be very a 
>>>>>> limited
>>>>>> 3GiB available for video memory.
>>>>>> This obviously does not meet the demands of modern applications.
>>>>>>
>>>>>> This patch makes it so the GTT size heuristic always uses 3/4ths of
>>>>>> the system memory size on APUs (limiting the size by 3GiB/VRAM size
>>>>>> only on devices with dedicated video memory).
>>>>>>
>>>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size 
>>>>>> according to
>>>>>> system memory size only")
>>>>>>
>>>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize, amdgpu_gart_size, 
>>>>>> uint, 0600);
>>>>>>
>>>>>>   /**
>>>>>>    * DOC: gttsize (int)
>>>>>> - * Restrict the size of GTT domain in MiB for testing. The 
>>>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>>>>> - * otherwise 3/4 RAM size).
>>>>>> + * Restrict the size of GTT domain in MiB for testing. The 
>>>>>> default is -1 (On APUs this is 3/4th
>>>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized, 
>>>>>> whichever is bigger,
>>>>>> + * with an upper bound of 3/4th of system memory.
>>>>>>    */
>>>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in megabytes 
>>>>>> (-1 = auto)");
>>>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> index 4d8f19ab1014..294f26f4f310 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device 
>>>>>> *adev)
>>>>>>                  struct sysinfo si;
>>>>>>
>>>>>>                  si_meminfo(&si);
>>>>>> -               gtt_size = min(max((AMDGPU_DEFAULT_GTT_SIZE_MB << 
>>>>>> 20),
>>>>>> -                              adev->gmc.mc_vram_size),
>>>>>> -                              ((uint64_t)si.totalram * 
>>>>>> si.mem_unit * 3/4));
>>>>>> +               gtt_size = (uint64_t)si.totalram * si.mem_unit * 3/4;
>>>>>> +               /* If we have dedicated memory, limit our GTT size to
>>>>>> +                * 3GiB or VRAM size, whichever is bigger
>>>>>> +                */
>>>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>>>>> +                       gtt_size = 
>>>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>>>>> + adev->gmc.mc_vram_size),
>>>>>> +                               gtt_size);
>>>>>> +               }
>>>>>>          }
>>>>>>          else
>>>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>>>>> -- 
>>>>>> 2.30.0
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0 
>>>>>>
>>>
> 

- Joshie 🐸✨
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 13:05             ` Christian König
@ 2021-01-06 13:17               ` Joshua Ashton
  2021-01-06 13:45                 ` Christian König
  0 siblings, 1 reply; 16+ messages in thread
From: Joshua Ashton @ 2021-01-06 13:17 UTC (permalink / raw)
  To: christian.koenig, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list



On 1/6/21 1:05 PM, Christian König wrote:
> Am 06.01.21 um 14:02 schrieb Bas Nieuwenhuizen:
>>
>>
>> On Wed, Jan 6, 2021 at 1:54 PM Christian König 
>> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>>
>>     Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>>     >
>>     >
>>     > On 1/6/21 7:52 AM, Christian König wrote:
>>     >> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>>     >>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>     >>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton
>>     <joshua@froggi.es <mailto:joshua@froggi.es>> wrote:
>>     >>>>>
>>     >>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>     >>>>> according to system memory size only""), the GTT size was
>>     limited by
>>     >>>>> 3GiB or VRAM size.
>>     >>>>
>>     >>>> The commit in question was to fix a hang with certain tests
>>     on APUs.
>>     >>>> That should be tested again before we re-enable this.  If it
>>     is fixed,
>>     >>>> we should just revert the revert rather than special case dGPUs.
>>     >>>>
>>     >>>> Alex
>>     >>>>
>>     >>>
>>     >>> I think the commit before the revert (ba851eed895c) has some
>>     >>> fundamental problems:
>>     >>>
>>     >>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if
>>     that
>>     >>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>     >>>
>>     >>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs
>>     makes
>>     >>> sense also and is a sensible limit to avoid silly situations with
>>     >>> overallocation and potential OOM.
>>     >>>
>>     >>> This patch solves both of those issues.
>>     >>
>>     >> No, Alex is right this approach was already tried and it causes
>>     >> problems.
>>     >>
>>     >> Additional to that why should this be an issue? Even when VRAM is
>>     >> very small on APUs we still use 3GiB of GTT.
>>     >>
>>     >> Regards,
>>     >> Christian.
>>     >
>>     > The problem is that 3GiB of GTT isn't enough for most modern games.
>>
>>     You seem to misunderstand what the GTT size means here. This is the
>>     amount of memory an application can lock down in a single command
>>     submissions.
>>
>>     It is still possible for the game to use all of system memory for
>>     textures etc... it can just happen that some buffers are temporary
>>     marked as inaccessible for the GPU.
>>
>>
>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. Usage 
>> in modern games is essentially "bindless" so there is no way to track 
>> at a per-submission level what memory needs to be resident. (and even 
>> with tracking applications are allowed to use all the memory in a 
>> single draw call, which would be unsplittable anyway ...)
> 
> Yeah, that is a really good point.
> 
> The issue is that we need some limitation since 3/4 of system memory is 
> way to much and the max texture size test in piglit can cause a system 
> crash.
> 
> The alternative is a better OOM handling, so that an application which 
> uses to much system memory through the driver stack has a more likely 
> chance to get killed. Cause currently that is either X or Wayland :(
> 
> Christian.

As I understand it, what is being exposed right now is essentially 
max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, before 
the revert what was being taken was just max(3GiB, 3/4ths).

If you had < 3GiB of system memory that seems like a bit of an issue 
that could easily leat to OOM to me?

Are you hitting on something smaller than 3/4ths right now? I remember 
the source commit mentioned they only had 1GiB of system memory 
available, so that could be possible if you had a carveout of < 786MiB...

- Joshie 🐸✨

> 
>>
>>
>>     > My laptop has a 128MiB carveout which is not possible to be
>>     configured
>>     > in the BIOS so I am stuck with that size without extra kernel
>>     > parameters which shouldn't be necessary.
>>
>>     Did you ran into problems without the parameter?
>>
>>     >
>>     > If you dislike the approach of keeping the extra check for dGPUs
>>     and
>>     > limiting GTT there, then I would say that we should use
>>     >     gtt_size = 3/4ths system memory
>>     > for all devices instead of
>>     >     gtt_size = max(3/4ths system memory, 3GiB)
>>     > as it was before the revert, as it is problematic on systems with <
>>     > 3GiB of system memory.
>>
>>     Yeah, that's indeed not a good idea.
>>
>>     Regards,
>>     Christian.
>>
>>     >
>>     > - Joshie 🐸✨
>>     >
>>     >>
>>     >>>
>>     >>> - Joshie 🐸✨
>>     >>>
>>     >>>>
>>     >>>>>
>>     >>>>> This is problematic on APUs, especially with a small carveout
>>     >>>>> which can be as low as a fixed 128MiB, as there would be very a
>>     >>>>> limited
>>     >>>>> 3GiB available for video memory.
>>     >>>>> This obviously does not meet the demands of modern applications.
>>     >>>>>
>>     >>>>> This patch makes it so the GTT size heuristic always uses
>>     3/4ths of
>>     >>>>> the system memory size on APUs (limiting the size by
>>     3GiB/VRAM size
>>     >>>>> only on devices with dedicated video memory).
>>     >>>>>
>>     >>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size
>>     >>>>> according to
>>     >>>>> system memory size only")
>>     >>>>>
>>     >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es
>>     <mailto:joshua@froggi.es>>
>>     >>>>> ---
>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>     >>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>     >>>>>
>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>     >>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>     >>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize,
>>     amdgpu_gart_size,
>>     >>>>> uint, 0600);
>>     >>>>>
>>     >>>>>   /**
>>     >>>>>    * DOC: gttsize (int)
>>     >>>>> - * Restrict the size of GTT domain in MiB for testing. The
>>     >>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>     >>>>> - * otherwise 3/4 RAM size).
>>     >>>>> + * Restrict the size of GTT domain in MiB for testing. The
>>     >>>>> default is -1 (On APUs this is 3/4th
>>     >>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM sized,
>>     >>>>> whichever is bigger,
>>     >>>>> + * with an upper bound of 3/4th of system memory.
>>     >>>>>    */
>>     >>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in
>>     megabytes
>>     >>>>> (-1 = auto)");
>>     >>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>     >>>>> index 4d8f19ab1014..294f26f4f310 100644
>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>     >>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct amdgpu_device
>>     >>>>> *adev)
>>     >>>>>                  struct sysinfo si;
>>     >>>>>
>>     >>>>>                  si_meminfo(&si);
>>     >>>>> -               gtt_size =
>>     min(max((AMDGPU_DEFAULT_GTT_SIZE_MB <<
>>     >>>>> 20),
>>     >>>>> - adev->gmc.mc_vram_size),
>>     >>>>> - ((uint64_t)si.totalram *
>>     >>>>> si.mem_unit * 3/4));
>>     >>>>> +               gtt_size = (uint64_t)si.totalram *
>>     si.mem_unit * 3/4;
>>     >>>>> +               /* If we have dedicated memory, limit our
>>     GTT size to
>>     >>>>> +                * 3GiB or VRAM size, whichever is bigger
>>     >>>>> +                */
>>     >>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>     >>>>> +                       gtt_size =
>>     >>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>     >>>>> + adev->gmc.mc_vram_size),
>>     >>>>> + gtt_size);
>>     >>>>> +               }
>>     >>>>>          }
>>     >>>>>          else
>>     >>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>     >>>>> --
>>     >>>>> 2.30.0
>>     >>>>>
>>     >>>>> _______________________________________________
>>     >>>>> amd-gfx mailing list
>>     >>>>> amd-gfx@lists.freedesktop.org
>>     <mailto:amd-gfx@lists.freedesktop.org>
>>     >>>>>
>>     https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0
>>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C890f3f3bb9144929d52308d8b2413a35%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455340521793984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ix5qMEHXC%2BeOly4OlgZ4mbFPIGz37g0JPawHfh412wE%3D&amp;reserved=0>
>>
>>     >>>>>
>>     >>
>>
>>     _______________________________________________
>>     amd-gfx mailing list
>>     amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>     <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 13:17               ` Joshua Ashton
@ 2021-01-06 13:45                 ` Christian König
  2021-01-06 14:18                   ` Joshua Ashton
  0 siblings, 1 reply; 16+ messages in thread
From: Christian König @ 2021-01-06 13:45 UTC (permalink / raw)
  To: Joshua Ashton, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list

Am 06.01.21 um 14:17 schrieb Joshua Ashton:
>
>
> On 1/6/21 1:05 PM, Christian König wrote:
>> Am 06.01.21 um 14:02 schrieb Bas Nieuwenhuizen:
>>>
>>>
>>> On Wed, Jan 6, 2021 at 1:54 PM Christian König 
>>> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>>>
>>>     Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>>>     >
>>>     >
>>>     > On 1/6/21 7:52 AM, Christian König wrote:
>>>     >> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>>>     >>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>>     >>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton
>>>     <joshua@froggi.es <mailto:joshua@froggi.es>> wrote:
>>>     >>>>>
>>>     >>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set 
>>> gtt size
>>>     >>>>> according to system memory size only""), the GTT size was
>>>     limited by
>>>     >>>>> 3GiB or VRAM size.
>>>     >>>>
>>>     >>>> The commit in question was to fix a hang with certain tests
>>>     on APUs.
>>>     >>>> That should be tested again before we re-enable this.  If it
>>>     is fixed,
>>>     >>>> we should just revert the revert rather than special case 
>>> dGPUs.
>>>     >>>>
>>>     >>>> Alex
>>>     >>>>
>>>     >>>
>>>     >>> I think the commit before the revert (ba851eed895c) has some
>>>     >>> fundamental problems:
>>>     >>>
>>>     >>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if
>>>     that
>>>     >>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>>     >>>
>>>     >>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs
>>>     makes
>>>     >>> sense also and is a sensible limit to avoid silly situations 
>>> with
>>>     >>> overallocation and potential OOM.
>>>     >>>
>>>     >>> This patch solves both of those issues.
>>>     >>
>>>     >> No, Alex is right this approach was already tried and it causes
>>>     >> problems.
>>>     >>
>>>     >> Additional to that why should this be an issue? Even when 
>>> VRAM is
>>>     >> very small on APUs we still use 3GiB of GTT.
>>>     >>
>>>     >> Regards,
>>>     >> Christian.
>>>     >
>>>     > The problem is that 3GiB of GTT isn't enough for most modern 
>>> games.
>>>
>>>     You seem to misunderstand what the GTT size means here. This is the
>>>     amount of memory an application can lock down in a single command
>>>     submissions.
>>>
>>>     It is still possible for the game to use all of system memory for
>>>     textures etc... it can just happen that some buffers are temporary
>>>     marked as inaccessible for the GPU.
>>>
>>>
>>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. 
>>> Usage in modern games is essentially "bindless" so there is no way 
>>> to track at a per-submission level what memory needs to be resident. 
>>> (and even with tracking applications are allowed to use all the 
>>> memory in a single draw call, which would be unsplittable anyway ...)
>>
>> Yeah, that is a really good point.
>>
>> The issue is that we need some limitation since 3/4 of system memory 
>> is way to much and the max texture size test in piglit can cause a 
>> system crash.
>>
>> The alternative is a better OOM handling, so that an application 
>> which uses to much system memory through the driver stack has a more 
>> likely chance to get killed. Cause currently that is either X or 
>> Wayland :(
>>
>> Christian.
>
> As I understand it, what is being exposed right now is essentially 
> max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, 
> before the revert what was being taken was just max(3GiB, 3/4ths).
>
> If you had < 3GiB of system memory that seems like a bit of an issue 
> that could easily leat to OOM to me?

Not really, as I said GTT is only the memory the GPU can lock at the 
same time. It is perfectly possible to have that larger than the 
available system memory.

In other words this is *not* to prevent using to much system memory, for 
this we have an additional limit inside TTM. But instead to have a 
reasonable limit for applications to not use to much memory at the same 
time.

>
> Are you hitting on something smaller than 3/4ths right now? I remember 
> the source commit mentioned they only had 1GiB of system memory 
> available, so that could be possible if you had a carveout of < 786MiB...

What do you mean with that? I don't have a test system at hand for this 
if that's what you are asking for.

Regards,
Christian.

>
> - Joshie 🐸✨
>
>>
>>>
>>>
>>>     > My laptop has a 128MiB carveout which is not possible to be
>>>     configured
>>>     > in the BIOS so I am stuck with that size without extra kernel
>>>     > parameters which shouldn't be necessary.
>>>
>>>     Did you ran into problems without the parameter?
>>>
>>>     >
>>>     > If you dislike the approach of keeping the extra check for dGPUs
>>>     and
>>>     > limiting GTT there, then I would say that we should use
>>>     >     gtt_size = 3/4ths system memory
>>>     > for all devices instead of
>>>     >     gtt_size = max(3/4ths system memory, 3GiB)
>>>     > as it was before the revert, as it is problematic on systems 
>>> with <
>>>     > 3GiB of system memory.
>>>
>>>     Yeah, that's indeed not a good idea.
>>>
>>>     Regards,
>>>     Christian.
>>>
>>>     >
>>>     > - Joshie 🐸✨
>>>     >
>>>     >>
>>>     >>>
>>>     >>> - Joshie 🐸✨
>>>     >>>
>>>     >>>>
>>>     >>>>>
>>>     >>>>> This is problematic on APUs, especially with a small carveout
>>>     >>>>> which can be as low as a fixed 128MiB, as there would be 
>>> very a
>>>     >>>>> limited
>>>     >>>>> 3GiB available for video memory.
>>>     >>>>> This obviously does not meet the demands of modern 
>>> applications.
>>>     >>>>>
>>>     >>>>> This patch makes it so the GTT size heuristic always uses
>>>     3/4ths of
>>>     >>>>> the system memory size on APUs (limiting the size by
>>>     3GiB/VRAM size
>>>     >>>>> only on devices with dedicated video memory).
>>>     >>>>>
>>>     >>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size
>>>     >>>>> according to
>>>     >>>>> system memory size only")
>>>     >>>>>
>>>     >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es
>>>     <mailto:joshua@froggi.es>>
>>>     >>>>> ---
>>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>     >>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>     >>>>>
>>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>     >>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>     >>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize,
>>>     amdgpu_gart_size,
>>>     >>>>> uint, 0600);
>>>     >>>>>
>>>     >>>>>   /**
>>>     >>>>>    * DOC: gttsize (int)
>>>     >>>>> - * Restrict the size of GTT domain in MiB for testing. The
>>>     >>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>>     >>>>> - * otherwise 3/4 RAM size).
>>>     >>>>> + * Restrict the size of GTT domain in MiB for testing. The
>>>     >>>>> default is -1 (On APUs this is 3/4th
>>>     >>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM 
>>> sized,
>>>     >>>>> whichever is bigger,
>>>     >>>>> + * with an upper bound of 3/4th of system memory.
>>>     >>>>>    */
>>>     >>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in
>>>     megabytes
>>>     >>>>> (-1 = auto)");
>>>     >>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>     >>>>> index 4d8f19ab1014..294f26f4f310 100644
>>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>     >>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct 
>>> amdgpu_device
>>>     >>>>> *adev)
>>>     >>>>>                  struct sysinfo si;
>>>     >>>>>
>>>     >>>>>                  si_meminfo(&si);
>>>     >>>>> -               gtt_size =
>>>     min(max((AMDGPU_DEFAULT_GTT_SIZE_MB <<
>>>     >>>>> 20),
>>>     >>>>> - adev->gmc.mc_vram_size),
>>>     >>>>> - ((uint64_t)si.totalram *
>>>     >>>>> si.mem_unit * 3/4));
>>>     >>>>> +               gtt_size = (uint64_t)si.totalram *
>>>     si.mem_unit * 3/4;
>>>     >>>>> +               /* If we have dedicated memory, limit our
>>>     GTT size to
>>>     >>>>> +                * 3GiB or VRAM size, whichever is bigger
>>>     >>>>> +                */
>>>     >>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>>     >>>>> +                       gtt_size =
>>>     >>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>>     >>>>> + adev->gmc.mc_vram_size),
>>>     >>>>> + gtt_size);
>>>     >>>>> +               }
>>>     >>>>>          }
>>>     >>>>>          else
>>>     >>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>>     >>>>> --
>>>     >>>>> 2.30.0
>>>     >>>>>
>>>     >>>>> _______________________________________________
>>>     >>>>> amd-gfx mailing list
>>>     >>>>> amd-gfx@lists.freedesktop.org
>>>     <mailto:amd-gfx@lists.freedesktop.org>
>>>     >>>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520008031%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=V4FWrLLGDFGC2ZsrDNDmqGqsMT72Va%2F854f9jxPtqLU%3D&amp;reserved=0
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0>
>>>
>>>     >>>>>
>>>     >>
>>>
>>>     _______________________________________________
>>>     amd-gfx mailing list
>>>     amd-gfx@lists.freedesktop.org 
>>> <mailto:amd-gfx@lists.freedesktop.org>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0 
>>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 13:45                 ` Christian König
@ 2021-01-06 14:18                   ` Joshua Ashton
  2021-01-06 14:59                     ` Christian König
  0 siblings, 1 reply; 16+ messages in thread
From: Joshua Ashton @ 2021-01-06 14:18 UTC (permalink / raw)
  To: Christian König, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list



On 1/6/21 1:45 PM, Christian König wrote:
> Am 06.01.21 um 14:17 schrieb Joshua Ashton:
>>
>>
>> On 1/6/21 1:05 PM, Christian König wrote:
>>> Am 06.01.21 um 14:02 schrieb Bas Nieuwenhuizen:
>>>>
>>>>
>>>> On Wed, Jan 6, 2021 at 1:54 PM Christian König 
>>>> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>>>>
>>>>     Am 06.01.21 um 13:47 schrieb Joshua Ashton:
>>>>     >
>>>>     >
>>>>     > On 1/6/21 7:52 AM, Christian König wrote:
>>>>     >> Am 05.01.21 um 23:31 schrieb Joshua Ashton:
>>>>     >>> On 1/5/21 10:10 PM, Alex Deucher wrote:
>>>>     >>>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton
>>>>     <joshua@froggi.es <mailto:joshua@froggi.es>> wrote:
>>>>     >>>>>
>>>>     >>>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set 
>>>> gtt size
>>>>     >>>>> according to system memory size only""), the GTT size was
>>>>     limited by
>>>>     >>>>> 3GiB or VRAM size.
>>>>     >>>>
>>>>     >>>> The commit in question was to fix a hang with certain tests
>>>>     on APUs.
>>>>     >>>> That should be tested again before we re-enable this.  If it
>>>>     is fixed,
>>>>     >>>> we should just revert the revert rather than special case 
>>>> dGPUs.
>>>>     >>>>
>>>>     >>>> Alex
>>>>     >>>>
>>>>     >>>
>>>>     >>> I think the commit before the revert (ba851eed895c) has some
>>>>     >>> fundamental problems:
>>>>     >>>
>>>>     >>> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if
>>>>     that
>>>>     >>> wouldn't fit into say, 1GiB or 2GiB of available RAM.
>>>>     >>>
>>>>     >>> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs
>>>>     makes
>>>>     >>> sense also and is a sensible limit to avoid silly situations 
>>>> with
>>>>     >>> overallocation and potential OOM.
>>>>     >>>
>>>>     >>> This patch solves both of those issues.
>>>>     >>
>>>>     >> No, Alex is right this approach was already tried and it causes
>>>>     >> problems.
>>>>     >>
>>>>     >> Additional to that why should this be an issue? Even when 
>>>> VRAM is
>>>>     >> very small on APUs we still use 3GiB of GTT.
>>>>     >>
>>>>     >> Regards,
>>>>     >> Christian.
>>>>     >
>>>>     > The problem is that 3GiB of GTT isn't enough for most modern 
>>>> games.
>>>>
>>>>     You seem to misunderstand what the GTT size means here. This is the
>>>>     amount of memory an application can lock down in a single command
>>>>     submissions.
>>>>
>>>>     It is still possible for the game to use all of system memory for
>>>>     textures etc... it can just happen that some buffers are temporary
>>>>     marked as inaccessible for the GPU.
>>>>
>>>>
>>>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. 
>>>> Usage in modern games is essentially "bindless" so there is no way 
>>>> to track at a per-submission level what memory needs to be resident. 
>>>> (and even with tracking applications are allowed to use all the 
>>>> memory in a single draw call, which would be unsplittable anyway ...)
>>>
>>> Yeah, that is a really good point.
>>>
>>> The issue is that we need some limitation since 3/4 of system memory 
>>> is way to much and the max texture size test in piglit can cause a 
>>> system crash.
>>>
>>> The alternative is a better OOM handling, so that an application 
>>> which uses to much system memory through the driver stack has a more 
>>> likely chance to get killed. Cause currently that is either X or 
>>> Wayland :(
>>>
>>> Christian.
>>
>> As I understand it, what is being exposed right now is essentially 
>> max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, 
>> before the revert what was being taken was just max(3GiB, 3/4ths).
>>
>> If you had < 3GiB of system memory that seems like a bit of an issue 
>> that could easily leat to OOM to me?
> 
> Not really, as I said GTT is only the memory the GPU can lock at the 
> same time. It is perfectly possible to have that larger than the 
> available system memory.
> 
> In other words this is *not* to prevent using to much system memory, for 
> this we have an additional limit inside TTM. But instead to have a 
> reasonable limit for applications to not use to much memory at the same 
> time.
> 

Worth noting that this GTT size here also affects the memory reporting 
and budgeting for applications. If the user has 1GiB of total system 
memory and 3GiB set here, then 3GiB will be the budget and size exposed 
to applications too...

(On APUs,) we really don't want to expose more GTT than system memory. 
Apps will eat into it and end up swapping or running into OOM or 
swapping *very* quickly. (I imagine this is likely what was being run 
into before the revert.)

Alternatively, in RADV and other user space drivers like AMDVLK, we 
could limit this to the system memory size or 3/4ths ourselves. Although 
that's kinda gross and I don't think that's the correct path...

>>
>> Are you hitting on something smaller than 3/4ths right now? I remember 
>> the source commit mentioned they only had 1GiB of system memory 
>> available, so that could be possible if you had a carveout of < 786MiB...
> 
> What do you mean with that? I don't have a test system at hand for this 
> if that's what you are asking for.

This was mainly a question to whoever did the revert. The question to 
find out some extra info about what they are using at the time.

- Joshie 🐸✨

> 
> Regards,
> Christian.
> 
>>
>> - Joshie 🐸✨
>>
>>>
>>>>
>>>>
>>>>     > My laptop has a 128MiB carveout which is not possible to be
>>>>     configured
>>>>     > in the BIOS so I am stuck with that size without extra kernel
>>>>     > parameters which shouldn't be necessary.
>>>>
>>>>     Did you ran into problems without the parameter?
>>>>
>>>>     >
>>>>     > If you dislike the approach of keeping the extra check for dGPUs
>>>>     and
>>>>     > limiting GTT there, then I would say that we should use
>>>>     >     gtt_size = 3/4ths system memory
>>>>     > for all devices instead of
>>>>     >     gtt_size = max(3/4ths system memory, 3GiB)
>>>>     > as it was before the revert, as it is problematic on systems 
>>>> with <
>>>>     > 3GiB of system memory.
>>>>
>>>>     Yeah, that's indeed not a good idea.
>>>>
>>>>     Regards,
>>>>     Christian.
>>>>
>>>>     >
>>>>     > - Joshie 🐸✨
>>>>     >
>>>>     >>
>>>>     >>>
>>>>     >>> - Joshie 🐸✨
>>>>     >>>
>>>>     >>>>
>>>>     >>>>>
>>>>     >>>>> This is problematic on APUs, especially with a small carveout
>>>>     >>>>> which can be as low as a fixed 128MiB, as there would be 
>>>> very a
>>>>     >>>>> limited
>>>>     >>>>> 3GiB available for video memory.
>>>>     >>>>> This obviously does not meet the demands of modern 
>>>> applications.
>>>>     >>>>>
>>>>     >>>>> This patch makes it so the GTT size heuristic always uses
>>>>     3/4ths of
>>>>     >>>>> the system memory size on APUs (limiting the size by
>>>>     3GiB/VRAM size
>>>>     >>>>> only on devices with dedicated video memory).
>>>>     >>>>>
>>>>     >>>>> Fixes: 24562523688b ("Revert drm/amd/amdgpu: set gtt size
>>>>     >>>>> according to
>>>>     >>>>> system memory size only")
>>>>     >>>>>
>>>>     >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es
>>>>     <mailto:joshua@froggi.es>>
>>>>     >>>>> ---
>>>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 +++--
>>>>     >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++++---
>>>>     >>>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>>>     >>>>>
>>>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>     >>>>> index 72efd579ec5e..a5a41e9272d6 100644
>>>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>     >>>>> @@ -192,8 +192,9 @@ module_param_named(gartsize,
>>>>     amdgpu_gart_size,
>>>>     >>>>> uint, 0600);
>>>>     >>>>>
>>>>     >>>>>   /**
>>>>     >>>>>    * DOC: gttsize (int)
>>>>     >>>>> - * Restrict the size of GTT domain in MiB for testing. The
>>>>     >>>>> default is -1 (It's VRAM size if 3GB < VRAM < 3/4 RAM,
>>>>     >>>>> - * otherwise 3/4 RAM size).
>>>>     >>>>> + * Restrict the size of GTT domain in MiB for testing. The
>>>>     >>>>> default is -1 (On APUs this is 3/4th
>>>>     >>>>> + * of the system memory; on dGPUs this is 3GiB or VRAM 
>>>> sized,
>>>>     >>>>> whichever is bigger,
>>>>     >>>>> + * with an upper bound of 3/4th of system memory.
>>>>     >>>>>    */
>>>>     >>>>>   MODULE_PARM_DESC(gttsize, "Size of the GTT domain in
>>>>     megabytes
>>>>     >>>>> (-1 = auto)");
>>>>     >>>>>   module_param_named(gttsize, amdgpu_gtt_size, int, 0600);
>>>>     >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>     >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>     >>>>> index 4d8f19ab1014..294f26f4f310 100644
>>>>     >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>     >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>     >>>>> @@ -1865,9 +1865,15 @@ int amdgpu_ttm_init(struct 
>>>> amdgpu_device
>>>>     >>>>> *adev)
>>>>     >>>>>                  struct sysinfo si;
>>>>     >>>>>
>>>>     >>>>>                  si_meminfo(&si);
>>>>     >>>>> -               gtt_size =
>>>>     min(max((AMDGPU_DEFAULT_GTT_SIZE_MB <<
>>>>     >>>>> 20),
>>>>     >>>>> - adev->gmc.mc_vram_size),
>>>>     >>>>> - ((uint64_t)si.totalram *
>>>>     >>>>> si.mem_unit * 3/4));
>>>>     >>>>> +               gtt_size = (uint64_t)si.totalram *
>>>>     si.mem_unit * 3/4;
>>>>     >>>>> +               /* If we have dedicated memory, limit our
>>>>     GTT size to
>>>>     >>>>> +                * 3GiB or VRAM size, whichever is bigger
>>>>     >>>>> +                */
>>>>     >>>>> +               if (!(adev->flags & AMD_IS_APU)) {
>>>>     >>>>> +                       gtt_size =
>>>>     >>>>> min(max(AMDGPU_DEFAULT_GTT_SIZE_MB << 20,
>>>>     >>>>> + adev->gmc.mc_vram_size),
>>>>     >>>>> + gtt_size);
>>>>     >>>>> +               }
>>>>     >>>>>          }
>>>>     >>>>>          else
>>>>     >>>>>                  gtt_size = (uint64_t)amdgpu_gtt_size << 20;
>>>>     >>>>> --
>>>>     >>>>> 2.30.0
>>>>     >>>>>
>>>>     >>>>> _______________________________________________
>>>>     >>>>> amd-gfx mailing list
>>>>     >>>>> amd-gfx@lists.freedesktop.org
>>>>     <mailto:amd-gfx@lists.freedesktop.org>
>>>>     >>>>>
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520008031%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=V4FWrLLGDFGC2ZsrDNDmqGqsMT72Va%2F854f9jxPtqLU%3D&amp;reserved=0 
>>>>
>>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0> 
>>>>
>>>>
>>>>     >>>>>
>>>>     >>
>>>>
>>>>     _______________________________________________
>>>>     amd-gfx mailing list
>>>>     amd-gfx@lists.freedesktop.org 
>>>> <mailto:amd-gfx@lists.freedesktop.org>
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0 
>>>>
>>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0> 
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C90674764db6d48dd746708d8b2456bdc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637455358520018028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8qIBp%2Bvrsato4fv3B%2BLQmbAmCDlRE4qGPi9iN8J2Nf8%3D&amp;reserved=0 
>>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 14:18                   ` Joshua Ashton
@ 2021-01-06 14:59                     ` Christian König
  2021-01-06 17:04                       ` Joshua Ashton
  0 siblings, 1 reply; 16+ messages in thread
From: Christian König @ 2021-01-06 14:59 UTC (permalink / raw)
  To: Joshua Ashton, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list

Am 06.01.21 um 15:18 schrieb Joshua Ashton:
> [SNIP]
>>>>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. 
>>>>> Usage in modern games is essentially "bindless" so there is no way 
>>>>> to track at a per-submission level what memory needs to be 
>>>>> resident. (and even with tracking applications are allowed to use 
>>>>> all the memory in a single draw call, which would be unsplittable 
>>>>> anyway ...)
>>>>
>>>> Yeah, that is a really good point.
>>>>
>>>> The issue is that we need some limitation since 3/4 of system 
>>>> memory is way to much and the max texture size test in piglit can 
>>>> cause a system crash.
>>>>
>>>> The alternative is a better OOM handling, so that an application 
>>>> which uses to much system memory through the driver stack has a 
>>>> more likely chance to get killed. Cause currently that is either X 
>>>> or Wayland :(
>>>>
>>>> Christian.
>>>
>>> As I understand it, what is being exposed right now is essentially 
>>> max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, 
>>> before the revert what was being taken was just max(3GiB, 3/4ths).
>>>
>>> If you had < 3GiB of system memory that seems like a bit of an issue 
>>> that could easily leat to OOM to me?
>>
>> Not really, as I said GTT is only the memory the GPU can lock at the 
>> same time. It is perfectly possible to have that larger than the 
>> available system memory.
>>
>> In other words this is *not* to prevent using to much system memory, 
>> for this we have an additional limit inside TTM. But instead to have 
>> a reasonable limit for applications to not use to much memory at the 
>> same time.
>>
>
> Worth noting that this GTT size here also affects the memory reporting 
> and budgeting for applications. If the user has 1GiB of total system 
> memory and 3GiB set here, then 3GiB will be the budget and size 
> exposed to applications too...

Yeah, that's indeed problematic.

>
> (On APUs,) we really don't want to expose more GTT than system memory. 
> Apps will eat into it and end up swapping or running into OOM or 
> swapping *very* quickly. (I imagine this is likely what was being run 
> into before the revert.)

No, the issue is that some applications try to allocate textures way 
above some reasonable limit.

> Alternatively, in RADV and other user space drivers like AMDVLK, we 
> could limit this to the system memory size or 3/4ths ourselves. 
> Although that's kinda gross and I don't think that's the correct path...

Ok, let me explain from the other side: We have this limitation because 
otherwise some tests like the maximum texture size test for OpenGL 
crashes the system. And this is independent of your system configuration.

We could of course add another limit for the texture size in 
OpenGL/RADV/AMDVLK, but I agree that this is rather awkward.

>>>
>>> Are you hitting on something smaller than 3/4ths right now? I 
>>> remember the source commit mentioned they only had 1GiB of system 
>>> memory available, so that could be possible if you had a carveout of 
>>> < 786MiB...
>>
>> What do you mean with that? I don't have a test system at hand for 
>> this if that's what you are asking for.
>
> This was mainly a question to whoever did the revert. The question to 
> find out some extra info about what they are using at the time.

You don't need a specific system configuration for this, just try to run 
the max texture size test in piglit.

Regards,
Christian.

>
> - Joshie 🐸✨

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 14:59                     ` Christian König
@ 2021-01-06 17:04                       ` Joshua Ashton
  2021-01-07 16:10                         ` Christian König
  0 siblings, 1 reply; 16+ messages in thread
From: Joshua Ashton @ 2021-01-06 17:04 UTC (permalink / raw)
  To: Christian König, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list



On 1/6/21 2:59 PM, Christian König wrote:
> Am 06.01.21 um 15:18 schrieb Joshua Ashton:
>> [SNIP]
>>>>>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. 
>>>>>> Usage in modern games is essentially "bindless" so there is no way 
>>>>>> to track at a per-submission level what memory needs to be 
>>>>>> resident. (and even with tracking applications are allowed to use 
>>>>>> all the memory in a single draw call, which would be unsplittable 
>>>>>> anyway ...)
>>>>>
>>>>> Yeah, that is a really good point.
>>>>>
>>>>> The issue is that we need some limitation since 3/4 of system 
>>>>> memory is way to much and the max texture size test in piglit can 
>>>>> cause a system crash.
>>>>>
>>>>> The alternative is a better OOM handling, so that an application 
>>>>> which uses to much system memory through the driver stack has a 
>>>>> more likely chance to get killed. Cause currently that is either X 
>>>>> or Wayland :(
>>>>>
>>>>> Christian.
>>>>
>>>> As I understand it, what is being exposed right now is essentially 
>>>> max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, 
>>>> before the revert what was being taken was just max(3GiB, 3/4ths).
>>>>
>>>> If you had < 3GiB of system memory that seems like a bit of an issue 
>>>> that could easily leat to OOM to me?
>>>
>>> Not really, as I said GTT is only the memory the GPU can lock at the 
>>> same time. It is perfectly possible to have that larger than the 
>>> available system memory.
>>>
>>> In other words this is *not* to prevent using to much system memory, 
>>> for this we have an additional limit inside TTM. But instead to have 
>>> a reasonable limit for applications to not use to much memory at the 
>>> same time.
>>>
>>
>> Worth noting that this GTT size here also affects the memory reporting 
>> and budgeting for applications. If the user has 1GiB of total system 
>> memory and 3GiB set here, then 3GiB will be the budget and size 
>> exposed to applications too...
> 
> Yeah, that's indeed problematic.
> 
>>
>> (On APUs,) we really don't want to expose more GTT than system memory. 
>> Apps will eat into it and end up swapping or running into OOM or 
>> swapping *very* quickly. (I imagine this is likely what was being run 
>> into before the revert.)
> 
> No, the issue is that some applications try to allocate textures way 
> above some reasonable limit.
> 
>> Alternatively, in RADV and other user space drivers like AMDVLK, we 
>> could limit this to the system memory size or 3/4ths ourselves. 
>> Although that's kinda gross and I don't think that's the correct path...
> 
> Ok, let me explain from the other side: We have this limitation because 
> otherwise some tests like the maximum texture size test for OpenGL 
> crashes the system. And this is independent of your system configuration.
> 
> We could of course add another limit for the texture size in 
> OpenGL/RADV/AMDVLK, but I agree that this is rather awkward.
> 
>>>>
>>>> Are you hitting on something smaller than 3/4ths right now? I 
>>>> remember the source commit mentioned they only had 1GiB of system 
>>>> memory available, so that could be possible if you had a carveout of 
>>>> < 786MiB...
>>>
>>> What do you mean with that? I don't have a test system at hand for 
>>> this if that's what you are asking for.
>>
>> This was mainly a question to whoever did the revert. The question to 
>> find out some extra info about what they are using at the time.
> 
> You don't need a specific system configuration for this, just try to run 
> the max texture size test in piglit.
> 
> Regards,
> Christian.

I see... I have not managed to reproduce a hang as described in the 
revert commit, but I have had a soft crash and delay with the OOM killer 
ending X.org after a little bit when GTT > system memory.

I tested with max-texture-size on both Renoir and Picasso the following 
conditions:
16GiB RAM + 12 GiB GTT -> test works fine
16GiB RAM + 64 GiB GTT -> OOM killer kills X.org after a little bit of 
waiting (piglit died with it)
2 GiB RAM + 1.5GiB GTT -> test works fine

I also tested on my Radeon VII and it worked fine regardless of the GTT 
size there, although that card has more than enough video memory any way 
for nothing to be an issue there 🐸.
Limiting my system memory to 2GiB, the card's memory and visible memory 
to 1GiB and the GTT to 1.75GiB, the test works fine.

The only time I ever had problems with a crash or pesudo-hang (waiting 
for OOM killer but the system was locked up) was whenever GTT was > 
system memory (ie. in the reverted commit)

If I edited my commit to universally use 3/4ths of the system memory for 
GTT for all hardware, would that be considered to be merged?

Thanks!
- Joshie 🐸✨

> 
>>
>> - Joshie 🐸✨
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-06 17:04                       ` Joshua Ashton
@ 2021-01-07 16:10                         ` Christian König
  0 siblings, 0 replies; 16+ messages in thread
From: Christian König @ 2021-01-07 16:10 UTC (permalink / raw)
  To: Joshua Ashton, Bas Nieuwenhuizen; +Cc: Alex Deucher, amd-gfx list

Am 06.01.21 um 18:04 schrieb Joshua Ashton:
>
>
> On 1/6/21 2:59 PM, Christian König wrote:
>> Am 06.01.21 um 15:18 schrieb Joshua Ashton:
>>> [SNIP]
>>>>>>> For Vulkan we (both RADV and AMDVLK) use GTT as the total size. 
>>>>>>> Usage in modern games is essentially "bindless" so there is no 
>>>>>>> way to track at a per-submission level what memory needs to be 
>>>>>>> resident. (and even with tracking applications are allowed to 
>>>>>>> use all the memory in a single draw call, which would be 
>>>>>>> unsplittable anyway ...)
>>>>>>
>>>>>> Yeah, that is a really good point.
>>>>>>
>>>>>> The issue is that we need some limitation since 3/4 of system 
>>>>>> memory is way to much and the max texture size test in piglit can 
>>>>>> cause a system crash.
>>>>>>
>>>>>> The alternative is a better OOM handling, so that an application 
>>>>>> which uses to much system memory through the driver stack has a 
>>>>>> more likely chance to get killed. Cause currently that is either 
>>>>>> X or Wayland :(
>>>>>>
>>>>>> Christian.
>>>>>
>>>>> As I understand it, what is being exposed right now is essentially 
>>>>> max(vram size, 3GiB) limited by 3/4ths of the memory. Previously, 
>>>>> before the revert what was being taken was just max(3GiB, 3/4ths).
>>>>>
>>>>> If you had < 3GiB of system memory that seems like a bit of an 
>>>>> issue that could easily leat to OOM to me?
>>>>
>>>> Not really, as I said GTT is only the memory the GPU can lock at 
>>>> the same time. It is perfectly possible to have that larger than 
>>>> the available system memory.
>>>>
>>>> In other words this is *not* to prevent using to much system 
>>>> memory, for this we have an additional limit inside TTM. But 
>>>> instead to have a reasonable limit for applications to not use to 
>>>> much memory at the same time.
>>>>
>>>
>>> Worth noting that this GTT size here also affects the memory 
>>> reporting and budgeting for applications. If the user has 1GiB of 
>>> total system memory and 3GiB set here, then 3GiB will be the budget 
>>> and size exposed to applications too...
>>
>> Yeah, that's indeed problematic.
>>
>>>
>>> (On APUs,) we really don't want to expose more GTT than system 
>>> memory. Apps will eat into it and end up swapping or running into 
>>> OOM or swapping *very* quickly. (I imagine this is likely what was 
>>> being run into before the revert.)
>>
>> No, the issue is that some applications try to allocate textures way 
>> above some reasonable limit.
>>
>>> Alternatively, in RADV and other user space drivers like AMDVLK, we 
>>> could limit this to the system memory size or 3/4ths ourselves. 
>>> Although that's kinda gross and I don't think that's the correct 
>>> path...
>>
>> Ok, let me explain from the other side: We have this limitation 
>> because otherwise some tests like the maximum texture size test for 
>> OpenGL crashes the system. And this is independent of your system 
>> configuration.
>>
>> We could of course add another limit for the texture size in 
>> OpenGL/RADV/AMDVLK, but I agree that this is rather awkward.
>>
>>>>>
>>>>> Are you hitting on something smaller than 3/4ths right now? I 
>>>>> remember the source commit mentioned they only had 1GiB of system 
>>>>> memory available, so that could be possible if you had a carveout 
>>>>> of < 786MiB...
>>>>
>>>> What do you mean with that? I don't have a test system at hand for 
>>>> this if that's what you are asking for.
>>>
>>> This was mainly a question to whoever did the revert. The question 
>>> to find out some extra info about what they are using at the time.
>>
>> You don't need a specific system configuration for this, just try to 
>> run the max texture size test in piglit.
>>
>> Regards,
>> Christian.
>
> I see... I have not managed to reproduce a hang as described in the 
> revert commit, but I have had a soft crash and delay with the OOM 
> killer ending X.org after a little bit when GTT > system memory.
>
> I tested with max-texture-size on both Renoir and Picasso the 
> following conditions:
> 16GiB RAM + 12 GiB GTT -> test works fine
> 16GiB RAM + 64 GiB GTT -> OOM killer kills X.org after a little bit of 
> waiting (piglit died with it)
> 2 GiB RAM + 1.5GiB GTT -> test works fine
>
> I also tested on my Radeon VII and it worked fine regardless of the 
> GTT size there, although that card has more than enough video memory 
> any way for nothing to be an issue there 🐸.
> Limiting my system memory to 2GiB, the card's memory and visible 
> memory to 1GiB and the GTT to 1.75GiB, the test works fine.
>
> The only time I ever had problems with a crash or pesudo-hang (waiting 
> for OOM killer but the system was locked up) was whenever GTT was > 
> system memory (ie. in the reverted commit)
>
> If I edited my commit to universally use 3/4ths of the system memory 
> for GTT for all hardware, would that be considered to be merged?

Well maybe 1/2 and only on APUs. And you need to find somebody with 
another Raven to test that. Maybe Nirmoy has time for this.

Regards,
Christian.

>
> Thanks!
> - Joshie 🐸✨
>
>>
>>>
>>> - Joshie 🐸✨
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/amdgpu: don't limit gtt size on apus
  2021-01-05 22:31   ` Joshua Ashton
  2021-01-06  7:52     ` Christian König
@ 2021-01-08  9:27     ` Paul Menzel
  1 sibling, 0 replies; 16+ messages in thread
From: Paul Menzel @ 2021-01-08  9:27 UTC (permalink / raw)
  To: Joshua Ashton, Alex Deucher; +Cc: Christian Koenig, amd-gfx

Dear Linux folks,


Am 05.01.21 um 23:31 schrieb Joshua Ashton:
> On 1/5/21 10:10 PM, Alex Deucher wrote:
>> On Tue, Jan 5, 2021 at 5:05 PM Joshua Ashton <joshua@froggi.es> wrote:
>>>
>>> Since commit 24562523688b ("Revert "drm/amd/amdgpu: set gtt size
>>> according to system memory size only""), the GTT size was limited by
>>> 3GiB or VRAM size.
>>
>> The commit in question was to fix a hang with certain tests on APUs.
>> That should be tested again before we re-enable this.  If it is fixed,
>> we should just revert the revert rather than special case dGPUs.

> I think the commit before the revert (ba851eed895c) has some fundamental 
> problems:

For the record, I can’t find commit ba851eed895c in my git repository. 
It seems to be commit 5f97fc0e03 [1]. (I wonder how that happened, but 
it was in 2017, so hopefully the processes improved.)

> It was always specifying max(3GiB, 3/4ths RAM) of GTT, even if that 
> wouldn't fit into say, 1GiB or 2GiB of available RAM.
> 
> Limiting GTT to min(max(3GiB, VRAM), 3/4ths RAM) size on dGPUs makes 
> sense also and is a sensible limit to avoid silly situations with 
> overallocation and potential OOM.
> 
> This patch solves both of those issues.


Kind regards,

Paul


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5f97fc0e032594212459f63e9c6229cd79ccb697
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-01-08  9:27 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 22:03 [PATCH] drm/amdgpu: don't limit gtt size on apus Joshua Ashton
2021-01-05 22:10 ` Alex Deucher
2021-01-05 22:31   ` Joshua Ashton
2021-01-06  7:52     ` Christian König
2021-01-06 12:47       ` Joshua Ashton
2021-01-06 12:54         ` Christian König
2021-01-06 13:02           ` Bas Nieuwenhuizen
2021-01-06 13:05             ` Christian König
2021-01-06 13:17               ` Joshua Ashton
2021-01-06 13:45                 ` Christian König
2021-01-06 14:18                   ` Joshua Ashton
2021-01-06 14:59                     ` Christian König
2021-01-06 17:04                       ` Joshua Ashton
2021-01-07 16:10                         ` Christian König
2021-01-06 13:06           ` Joshua Ashton
2021-01-08  9:27     ` Paul Menzel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.