[PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Alex Deucher]

This adds allocation latency, but aligns better with user
expectations.  The latency should improve with the drm buddy
clearing patches that Arun has been working on.

In addition this fixes the high CPU spikes seen when doing
wipe on release.

v2: always set AMDGPU_GEM_CREATE_VRAM_CLEARED (Christian)

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com> (v1)
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Cc: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 1f149c9e2177..8794661b0bfa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -349,6 +349,9 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	/* always clear VRAM */
+	flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
+
 	/* create a gem object to contain this object in */
 	if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
 	    AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
-- 
2.46.0

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Paneer Selvam, Arunpravin]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

this will fix performance issues,
Acked-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com 
<mailto:Arunpravin.PaneerSelvam@amd.com>>

On 8/29/2024 10:56 PM, Alex Deucher wrote:
> This adds allocation latency, but aligns better with user
> expectations.  The latency should improve with the drm buddy
> clearing patches that Arun has been working on.
>
> In addition this fixes the high CPU spikes seen when doing
> wipe on release.
>
> v2: always set AMDGPU_GEM_CREATE_VRAM_CLEARED (Christian)
>
> Closes:https://gitlab.freedesktop.org/drm/amd/-/issues/3528
> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> Reviewed-by: Michel Dänzer<mdaenzer@redhat.com>  (v1)
> Signed-off-by: Alex Deucher<alexander.deucher@amd.com>
> Cc: Arunpravin Paneer Selvam<Arunpravin.PaneerSelvam@amd.com>
> Cc: Christian König<christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 1f149c9e2177..8794661b0bfa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -349,6 +349,9 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   		return -EINVAL;
>   	}
>   
> +	/* always clear VRAM */
> +	flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
> +
>   	/* create a gem object to contain this object in */
>   	if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
>   	    AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {

[-- Attachment #2: Type: text/html, Size: 2506 bytes --]

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Marek Olšák]

Can you also bump the DRM version, so that userspace knows when to
skip its own clear?

Also, clearing with SDMA takes up to 33 times more time (= is up to
97% slower) than clearing with compute.

Marek

On Thu, Aug 29, 2024 at 2:23 PM Paneer Selvam, Arunpravin
<arunpravin.paneerselvam@amd.com> wrote:
>
> this will fix performance issues,
> Acked-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>
> On 8/29/2024 10:56 PM, Alex Deucher wrote:
>
> This adds allocation latency, but aligns better with user
> expectations.  The latency should improve with the drm buddy
> clearing patches that Arun has been working on.
>
> In addition this fixes the high CPU spikes seen when doing
> wipe on release.
>
> v2: always set AMDGPU_GEM_CREATE_VRAM_CLEARED (Christian)
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> Reviewed-by: Michel Dänzer <mdaenzer@redhat.com> (v1)
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 1f149c9e2177..8794661b0bfa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -349,6 +349,9 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   return -EINVAL;
>   }
>
> + /* always clear VRAM */
> + flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
> +
>   /* create a gem object to contain this object in */
>   if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
>      AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
>
>

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Alex Deucher]

On Fri, Sep 6, 2024 at 10:18 AM Marek Olšák <maraeo@gmail.com> wrote:
>
> Can you also bump the DRM version, so that userspace knows when to
> skip its own clear?

Sure, although going forward, it might be better to migrate to a
generic flags query in the INFO ioctl so we can just check for various
feature bits so we can backport functionality to older kernels more
easily.

Alex

>
> Also, clearing with SDMA takes up to 33 times more time (= is up to
> 97% slower) than clearing with compute.
>
> Marek
>
> On Thu, Aug 29, 2024 at 2:23 PM Paneer Selvam, Arunpravin
> <arunpravin.paneerselvam@amd.com> wrote:
> >
> > this will fix performance issues,
> > Acked-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> >
> > On 8/29/2024 10:56 PM, Alex Deucher wrote:
> >
> > This adds allocation latency, but aligns better with user
> > expectations.  The latency should improve with the drm buddy
> > clearing patches that Arun has been working on.
> >
> > In addition this fixes the high CPU spikes seen when doing
> > wipe on release.
> >
> > v2: always set AMDGPU_GEM_CREATE_VRAM_CLEARED (Christian)
> >
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
> > Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> > Reviewed-by: Michel Dänzer <mdaenzer@redhat.com> (v1)
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > index 1f149c9e2177..8794661b0bfa 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -349,6 +349,9 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
> >   return -EINVAL;
> >   }
> >
> > + /* always clear VRAM */
> > + flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
> > +
> >   /* create a gem object to contain this object in */
> >   if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
> >      AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
> >
> >

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Marek Olšák]

On Fri, Sep 6, 2024 at 1:53 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Fri, Sep 6, 2024 at 10:18 AM Marek Olšák <maraeo@gmail.com> wrote:
> >
> > Can you also bump the DRM version, so that userspace knows when to
> > skip its own clear?
>
> Sure, although going forward, it might be better to migrate to a
> generic flags query in the INFO ioctl so we can just check for various
> feature bits so we can backport functionality to older kernels more
> easily.

How it's exposed is up to you.

Marek

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Paneer Selvam, Arunpravin]

Thanks, I will try clearing with compute and check the performance.

Regards,
Arun.

On 9/6/2024 7:37 PM, Marek Olšák wrote:
> Can you also bump the DRM version, so that userspace knows when to
> skip its own clear?
>
> Also, clearing with SDMA takes up to 33 times more time (= is up to
> 97% slower) than clearing with compute.
>
> Marek
>
> On Thu, Aug 29, 2024 at 2:23 PM Paneer Selvam, Arunpravin
> <arunpravin.paneerselvam@amd.com> wrote:
>> this will fix performance issues,
>> Acked-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>
>> On 8/29/2024 10:56 PM, Alex Deucher wrote:
>>
>> This adds allocation latency, but aligns better with user
>> expectations.  The latency should improve with the drm buddy
>> clearing patches that Arun has been working on.
>>
>> In addition this fixes the high CPU spikes seen when doing
>> wipe on release.
>>
>> v2: always set AMDGPU_GEM_CREATE_VRAM_CLEARED (Christian)
>>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>> Reviewed-by: Michel Dänzer <mdaenzer@redhat.com> (v1)
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 1f149c9e2177..8794661b0bfa 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -349,6 +349,9 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>    return -EINVAL;
>>    }
>>
>> + /* always clear VRAM */
>> + flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
>> +
>>    /* create a gem object to contain this object in */
>>    if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
>>       AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
>>
>>

[PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Alex Deucher]

This adds allocation latency, but aligns better with user
expectations.  The latency should improve with the drm buddy
clearing patches that Arun has been working on.

In addition this fixes the high CPU spikes seen when doing
wipe on release.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Cc: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 1f149c9e2177..16af465f1c01 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -349,6 +349,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	/* always clear VRAM */
+	if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
+		flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
+
 	/* create a gem object to contain this object in */
 	if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
 	    AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
-- 
2.46.0

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Christian König]

Am 23.08.24 um 15:36 schrieb Alex Deucher:
> This adds allocation latency, but aligns better with user
> expectations.  The latency should improve with the drm buddy
> clearing patches that Arun has been working on.

The problem is still that we can't most likely will run into issues with 
some of the bootup time constrains we have.

>
> In addition this fixes the high CPU spikes seen when doing
> wipe on release.

Mhm, interesting.

>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3528
> Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 1f149c9e2177..16af465f1c01 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -349,6 +349,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   		return -EINVAL;
>   	}
>   
> +	/* always clear VRAM */
> +	if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)

I think we should drop that if. When only GTT is allocated the flag is 
ignored and it can be the we switch the domain later on.

Christian.

> +		flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
> +
>   	/* create a gem object to contain this object in */
>   	if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
>   	    AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {

[PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Alex Deucher]

This adds allocation latency, but aligns better with user
expectations.  The latency should improve with the drm buddy
clearing patches that Arun has been working on.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ebb3f87ef4f6..768840e8937d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -347,6 +347,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	/* always clear VRAM */
+	if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
+		flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
+
 	/* create a gem object to contain this object in */
 	if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
 	    AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
-- 
2.45.2

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Alex Deucher]

On Fri, Jul 26, 2024 at 9:50 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> This adds allocation latency, but aligns better with user
> expectations.  The latency should improve with the drm buddy
> clearing patches that Arun has been working on.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ebb3f87ef4f6..768840e8937d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -347,6 +347,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>                 return -EINVAL;
>         }
>
> +       /* always clear VRAM */
> +       if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
> +               flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;

Alternatively, we can set AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE here,
although for cases like reboots, we probably want clear on create.

Alex


> +
>         /* create a gem object to contain this object in */
>         if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
>             AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) {
> --
> 2.45.2
>

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Michel Dänzer]

On 2024-07-26 17:25, Alex Deucher wrote:
> On Fri, Jul 26, 2024 at 9:50 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>>
>> This adds allocation latency, but aligns better with user
>> expectations.  The latency should improve with the drm buddy
>> clearing patches that Arun has been working on.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index ebb3f87ef4f6..768840e8937d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -347,6 +347,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>                 return -EINVAL;
>>         }
>>
>> +       /* always clear VRAM */
>> +       if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
>> +               flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
> 
> Alternatively, we can set AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE here,
> although for cases like reboots, we probably want clear on create.

I agree. Thanks for doing this!

Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>


BTW, maybe this allows reducing the use of AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE, or even eliminating it altogether? Does this leave any way for user space to access uncleared VRAM?


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Christian König]

Am 29.07.24 um 12:42 schrieb Michel Dänzer:
> On 2024-07-26 17:25, Alex Deucher wrote:
>> On Fri, Jul 26, 2024 at 9:50 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>>> This adds allocation latency, but aligns better with user
>>> expectations.  The latency should improve with the drm buddy
>>> clearing patches that Arun has been working on.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index ebb3f87ef4f6..768840e8937d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -347,6 +347,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>                  return -EINVAL;
>>>          }
>>>
>>> +       /* always clear VRAM */
>>> +       if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
>>> +               flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
>> Alternatively, we can set AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE here,
>> although for cases like reboots, we probably want clear on create.
> I agree. Thanks for doing this!

I strongly disagree. We have use cases were this will immediately become 
a problem.

Especially we won't be able to hold latency requirements on boot for 
certain projects any more.

Regards,
Christian.

>
> Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
>
>
> BTW, maybe this allows reducing the use of AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE, or even eliminating it altogether? Does this leave any way for user space to access uncleared VRAM?
>
>

Re: [PATCH] drm/amdgpu: always allocate cleared VRAM for GEM allocations

[Michel Dänzer]

On 2024-07-29 13:14, Christian König wrote:
> Am 29.07.24 um 12:42 schrieb Michel Dänzer:
>> On 2024-07-26 17:25, Alex Deucher wrote:
>>> On Fri, Jul 26, 2024 at 9:50 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>>>> This adds allocation latency, but aligns better with user
>>>> expectations.  The latency should improve with the drm buddy
>>>> clearing patches that Arun has been working on.
>>>>
>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index ebb3f87ef4f6..768840e8937d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -347,6 +347,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>                  return -EINVAL;
>>>>          }
>>>>
>>>> +       /* always clear VRAM */
>>>> +       if (args->in.domains & AMDGPU_GEM_DOMAIN_VRAM)
>>>> +               flags |= AMDGPU_GEM_CREATE_VRAM_CLEARED;
>>> Alternatively, we can set AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE here,
>>> although for cases like reboots, we probably want clear on create.
>> I agree. Thanks for doing this!
> 
> I strongly disagree. We have use cases were this will immediately become a problem.
> 
> Especially we won't be able to hold latency requirements on boot for certain projects any more.

Then it was only possible to achieve them by essentially cheating, leaking uncleared VRAM contents to user space.

If that's acceptable for those use cases, you could maybe add some kind of opt-in mechanism for them.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer