[PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Nicholas Kazlauskas]

The dumb_create API isn't intended for high performance rendering
and it's more useful for userspace (ie. IGT) to have them precleared.

The bonus here is that we also won't needlessly leak whatever was
previously in VRAM, but it also probably wasn't sensitive if it was
going through this API.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index fcaaac30e84b..a58072bbc9b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
 	domain = amdgpu_bo_get_preferred_pin_domain(adev,
 				amdgpu_display_supported_domains(adev));
 	r = amdgpu_gem_object_create(adev, args->size, 0, domain,
-				     AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
+				     AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
+				     AMDGPU_GEM_CREATE_VRAM_CLEARED,
 				     ttm_bo_type_device, NULL, &gobj);
 	if (r)
 		return -ENOMEM;
-- 
2.17.1

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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Alex Deucher]

On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
<nicholas.kazlauskas@amd.com> wrote:
>
> The dumb_create API isn't intended for high performance rendering
> and it's more useful for userspace (ie. IGT) to have them precleared.
>
> The bonus here is that we also won't needlessly leak whatever was
> previously in VRAM, but it also probably wasn't sensitive if it was
> going through this API.
>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index fcaaac30e84b..a58072bbc9b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
>         domain = amdgpu_bo_get_preferred_pin_domain(adev,
>                                 amdgpu_display_supported_domains(adev));
>         r = amdgpu_gem_object_create(adev, args->size, 0, domain,
> -                                    AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
> +                                    AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>                                      ttm_bo_type_device, NULL, &gobj);
>         if (r)
>                 return -ENOMEM;
> --
> 2.17.1
>
> _______________________________________________
> 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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Christian König]

Am 08.03.19 um 17:47 schrieb Alex Deucher:
> On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
> <nicholas.kazlauskas@amd.com> wrote:
>> The dumb_create API isn't intended for high performance rendering
>> and it's more useful for userspace (ie. IGT) to have them precleared.
>>
>> The bonus here is that we also won't needlessly leak whatever was
>> previously in VRAM, but it also probably wasn't sensitive if it was
>> going through this API.
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

NAK, IIRC we used to have this bit here before.

In general I agree that this would be a good idea, but the problem is 
the that first dump buffer is sometimes created before the engine 
(usually SDMA) to clear VRAM is initialized.

So you can run into a nice crash on suspend/resume.

Christian.

>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index fcaaac30e84b..a58072bbc9b8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
>>          domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>                                  amdgpu_display_supported_domains(adev));
>>          r = amdgpu_gem_object_create(adev, args->size, 0, domain,
>> -                                    AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
>> +                                    AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>>                                       ttm_bo_type_device, NULL, &gobj);
>>          if (r)
>>                  return -ENOMEM;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> 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

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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Kazlauskas, Nicholas]

On 3/11/19 12:58 PM, Christian König wrote:
> Am 08.03.19 um 17:47 schrieb Alex Deucher:
>> On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
>> <nicholas.kazlauskas@amd.com> wrote:
>>> The dumb_create API isn't intended for high performance rendering
>>> and it's more useful for userspace (ie. IGT) to have them precleared.
>>>
>>> The bonus here is that we also won't needlessly leak whatever was
>>> previously in VRAM, but it also probably wasn't sensitive if it was
>>> going through this API.
>>>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> NAK, IIRC we used to have this bit here before.
> 
> In general I agree that this would be a good idea, but the problem is 
> the that first dump buffer is sometimes created before the engine 
> (usually SDMA) to clear VRAM is initialized.
> 
> So you can run into a nice crash on suspend/resume.
> 
> Christian.

FWIW I never hit it myself during any suspend/resume tests I did but 
I'll take your word for it.

How about doing a map/memset/unmap immediately after buffer creation 
here? It'd be much slower, but it's also what we'd be doing in userspace 
anyway.

Nicholas Kazlauskas


> 
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index fcaaac30e84b..a58072bbc9b8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file 
>>> *file_priv,
>>>          domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>>                                  
>>> amdgpu_display_supported_domains(adev));
>>>          r = amdgpu_gem_object_create(adev, args->size, 0, domain,
>>> -                                    
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
>>> +                                    
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>>>                                       ttm_bo_type_device, NULL, &gobj);
>>>          if (r)
>>>                  return -ENOMEM;
>>> -- 
>>> 2.17.1
>>>
>>> _______________________________________________
>>> 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
> 

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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Koenig, Christian]

Am 11.03.19 um 18:04 schrieb Kazlauskas, Nicholas:
> On 3/11/19 12:58 PM, Christian König wrote:
>> Am 08.03.19 um 17:47 schrieb Alex Deucher:
>>> On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
>>> <nicholas.kazlauskas@amd.com> wrote:
>>>> The dumb_create API isn't intended for high performance rendering
>>>> and it's more useful for userspace (ie. IGT) to have them precleared.
>>>>
>>>> The bonus here is that we also won't needlessly leak whatever was
>>>> previously in VRAM, but it also probably wasn't sensitive if it was
>>>> going through this API.
>>>>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> NAK, IIRC we used to have this bit here before.
>>
>> In general I agree that this would be a good idea, but the problem is
>> the that first dump buffer is sometimes created before the engine
>> (usually SDMA) to clear VRAM is initialized.
>>
>> So you can run into a nice crash on suspend/resume.
>>
>> Christian.
> FWIW I never hit it myself during any suspend/resume tests I did but
> I'll take your word for it.
>
> How about doing a map/memset/unmap immediately after buffer creation
> here? It'd be much slower, but it's also what we'd be doing in userspace
> anyway.

And history repeat itself :) Yeah, that was also suggested before and 
didn't worked either for some reason.

But that was ~2 years ago and I really don't remember the details.

What could work is maybe to add a check if the engine is available if 
the flag is given and ignore it when it isn't.

Christian.

>
> Nicholas Kazlauskas
>
>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index fcaaac30e84b..a58072bbc9b8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file
>>>> *file_priv,
>>>>           domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>>>                                   
>>>> amdgpu_display_supported_domains(adev));
>>>>           r = amdgpu_gem_object_create(adev, args->size, 0, domain,
>>>> -
>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
>>>> +
>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>>>>                                        ttm_bo_type_device, NULL, &gobj);
>>>>           if (r)
>>>>                   return -ENOMEM;
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> 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

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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Kazlauskas, Nicholas]

On 3/11/19 1:31 PM, Koenig, Christian wrote:
> Am 11.03.19 um 18:04 schrieb Kazlauskas, Nicholas:
>> On 3/11/19 12:58 PM, Christian König wrote:
>>> Am 08.03.19 um 17:47 schrieb Alex Deucher:
>>>> On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
>>>> <nicholas.kazlauskas@amd.com> wrote:
>>>>> The dumb_create API isn't intended for high performance rendering
>>>>> and it's more useful for userspace (ie. IGT) to have them precleared.
>>>>>
>>>>> The bonus here is that we also won't needlessly leak whatever was
>>>>> previously in VRAM, but it also probably wasn't sensitive if it was
>>>>> going through this API.
>>>>>
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>> NAK, IIRC we used to have this bit here before.
>>>
>>> In general I agree that this would be a good idea, but the problem is
>>> the that first dump buffer is sometimes created before the engine
>>> (usually SDMA) to clear VRAM is initialized.
>>>
>>> So you can run into a nice crash on suspend/resume.
>>>
>>> Christian.
>> FWIW I never hit it myself during any suspend/resume tests I did but
>> I'll take your word for it.
>>
>> How about doing a map/memset/unmap immediately after buffer creation
>> here? It'd be much slower, but it's also what we'd be doing in userspace
>> anyway.
> 
> And history repeat itself :) Yeah, that was also suggested before and
> didn't worked either for some reason.
> 
> But that was ~2 years ago and I really don't remember the details.
> 
> What could work is maybe to add a check if the engine is available if
> the flag is given and ignore it when it isn't.
> 
> Christian.

The function amdgpu_fill_buffer is used to clear memory, and it checks

if (!adev->mman.buffer_funcs_enabled)

at the start. And buffer_funcs_enabled is only set after SDMA has been 
initialized following resume. In the case where the SDMA engine is off 
it'll just throw that DRM_ERROR and return -EINVAL.

So if we still want to continue with buffer creation in the case where 
the engine is off then this flag could also be checked in 
dumb_buffer_create and cleared in the case where buffer_funcs_enabled = 
false.

How does that sound to you?

Nicholas Kazlauskas

> 
>>
>> Nicholas Kazlauskas
>>
>>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index fcaaac30e84b..a58072bbc9b8 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file
>>>>> *file_priv,
>>>>>            domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>>>>                                    
>>>>> amdgpu_display_supported_domains(adev));
>>>>>            r = amdgpu_gem_object_create(adev, args->size, 0, domain,
>>>>> -
>>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
>>>>> +
>>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>>> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>>>>>                                         ttm_bo_type_device, NULL, &gobj);
>>>>>            if (r)
>>>>>                    return -ENOMEM;
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> 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
> 
> _______________________________________________
> 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

Re: [PATCH] drm/amdgpu: Clear VRAM for DRM dumb_create buffers

[Christian König]

Am 11.03.19 um 18:53 schrieb Kazlauskas, Nicholas:
> On 3/11/19 1:31 PM, Koenig, Christian wrote:
>> Am 11.03.19 um 18:04 schrieb Kazlauskas, Nicholas:
>>> On 3/11/19 12:58 PM, Christian König wrote:
>>>> Am 08.03.19 um 17:47 schrieb Alex Deucher:
>>>>> On Fri, Mar 8, 2019 at 10:38 AM Nicholas Kazlauskas
>>>>> <nicholas.kazlauskas@amd.com> wrote:
>>>>>> The dumb_create API isn't intended for high performance rendering
>>>>>> and it's more useful for userspace (ie. IGT) to have them precleared.
>>>>>>
>>>>>> The bonus here is that we also won't needlessly leak whatever was
>>>>>> previously in VRAM, but it also probably wasn't sensitive if it was
>>>>>> going through this API.
>>>>>>
>>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>>> NAK, IIRC we used to have this bit here before.
>>>>
>>>> In general I agree that this would be a good idea, but the problem is
>>>> the that first dump buffer is sometimes created before the engine
>>>> (usually SDMA) to clear VRAM is initialized.
>>>>
>>>> So you can run into a nice crash on suspend/resume.
>>>>
>>>> Christian.
>>> FWIW I never hit it myself during any suspend/resume tests I did but
>>> I'll take your word for it.
>>>
>>> How about doing a map/memset/unmap immediately after buffer creation
>>> here? It'd be much slower, but it's also what we'd be doing in userspace
>>> anyway.
>> And history repeat itself :) Yeah, that was also suggested before and
>> didn't worked either for some reason.
>>
>> But that was ~2 years ago and I really don't remember the details.
>>
>> What could work is maybe to add a check if the engine is available if
>> the flag is given and ignore it when it isn't.
>>
>> Christian.
> The function amdgpu_fill_buffer is used to clear memory, and it checks
>
> if (!adev->mman.buffer_funcs_enabled)
>
> at the start. And buffer_funcs_enabled is only set after SDMA has been
> initialized following resume. In the case where the SDMA engine is off
> it'll just throw that DRM_ERROR and return -EINVAL.
>
> So if we still want to continue with buffer creation in the case where
> the engine is off then this flag could also be checked in
> dumb_buffer_create and cleared in the case where buffer_funcs_enabled =
> false.
>
> How does that sound to you?

Yeah, that sounds like a plan to me.

Christian.

>
> Nicholas Kazlauskas
>
>>> Nicholas Kazlauskas
>>>
>>>
>>>>>> ---
>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index fcaaac30e84b..a58072bbc9b8 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file
>>>>>> *file_priv,
>>>>>>             domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>>>>>                                     
>>>>>> amdgpu_display_supported_domains(adev));
>>>>>>             r = amdgpu_gem_object_create(adev, args->size, 0, domain,
>>>>>> -
>>>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
>>>>>> +
>>>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>>>> +                                    AMDGPU_GEM_CREATE_VRAM_CLEARED,
>>>>>>                                          ttm_bo_type_device, NULL, &gobj);
>>>>>>             if (r)
>>>>>>                     return -ENOMEM;
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>> _______________________________________________
>> 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

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