[PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Christian König]

Am 26.07.19 um 16:53 schrieb Michel Dänzer:
> On 2019-07-26 1:55 p.m., Christian König wrote:
>> Am 26.07.19 um 10:54 schrieb Michel Dänzer:
>>> On 2019-07-26 9:11 a.m., Christian König wrote:
>>>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>>>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>>>>> amdgpu_bo_do_create into standalone helper so it can be reused
>>>>> in other functions.
>>>>>
>>>>> v4:
>>>>> Switch to return bool.
>>>>>
>>>>> v5: Fix typos.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>>>>> +++++++++++++++++-------------
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>>>>     2 files changed, 37 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 989b7b5..8702062 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>>>>> amdgpu_device *adev,
>>>>>         return false;
>>>>>     }
>>>>>     +bool amdgpu_bo_support_uswc(u64 bo_flags)
>>>>> +{
>>>>> +
>>>>> +#ifdef CONFIG_X86_32
>>>>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>>>>> +     */
>>>>> +    return false;
>>>>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>>>>> +    /* Don't try to enable write-combining when it can't work, or
>>>>> things
>>>>> +     * may be slow
>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>>>>> +     */
>>>>> +
>>>>> +#ifndef CONFIG_COMPILE_TEST
>>>>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>>>>> performance \
>>>>> +     thanks to write-combining
>>>>> +#endif
>>>>> +
>>>>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>>>>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>>>>> for "
>>>>> +                  "better performance thanks to write-combining\n");
>>>> I don't think this message belongs here.
>>>>
>>>> [...]
>>>>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>>>>> [...]
>>>>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>>>>             bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>> Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC
>>>> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
>>>> the warning.
>>> That would require duplicating the CONFIG_X86_PAT related logic here as
>>> well, which is a bit ugly.
>> Actually I would say we should drop this extra check and always emit a
>> message that USWC is disabled for this platform.
>>
>> We now need it for more than just better performance and it should be
>> explicitly noted that this is not available in the logs.
> A log message which doesn't explain why it's disabled / how to enable it
> would probably cause us user support pain.

Mhm, sounds like you didn't got what I wanted to say.

No log message was fine as long as USWC was only a performance 
optimization, but now it becomes mandatory for correct operation in some 
settings.

In other words in very low VRAM configurations it can be that we can't 
enable higher resolution because the kernel is not compiled with the 
necessary flags for USWC support.

Printing that when the driver loads sounds like the best place to me.

Regards,
Christian.