[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 at 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 at amd.com>
>>>> Reviewed-by: Alex Deucher <alexander.deucher at 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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>