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

[Michel Dänzer]

On 2019-07-26 6:02 p.m., Christian König wrote:
> 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.

With an APU which supports scatter/gather scanout, sure.

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

Works for me, but it still needs to explain why it's disabled / how to
enable it... Something like "enable PAT" or "use a 64-bit kernel".


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer