[PATCH] drm/amdgpu: consider user preference when pinning for SG display

[Christian König]

Am 22.05.2018 um 09:22 schrieb Michel Dänzer:
> On 2018-05-19 05:59 PM, Christian König wrote:
>> Am 18.05.2018 um 21:16 schrieb Alex Deucher:
>>> On Fri, May 18, 2018 at 2:22 PM, Samuel Li <samuel.li at amd.com> wrote:
>>>> On 2018-05-18 04:21 AM, Michel Dänzer wrote:
>>>>> On 2018-05-17 06:55 PM, Alex Deucher wrote:
>>>>>> If the pin domain is set to GTT | VRAM, look at the preferred domains
>>>>>> for the bo and respect that if it's been set explicitly.
>>>>>>
>>>>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 9 +++++++--
>>>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 6a9e46ae7f0a..16192f17653e 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -704,9 +704,14 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>>>>> *bo, u32 domain,
>>>>>>         * See function amdgpu_display_supported_domains()
>>>>>>         */
>>>>>>        if (domain == (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>> AMDGPU_GEM_DOMAIN_GTT)) {
>>>>>> -            domain = AMDGPU_GEM_DOMAIN_VRAM;
>>>>>> -            if (adev->gmc.real_vram_size <= AMDGPU_SG_THRESHOLD)
>>>>>> +            if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>> +                    domain = AMDGPU_GEM_DOMAIN_VRAM; /* if user
>>>>>> really wants vram, respect it */
>>>>>> +            else if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GTT)
>>>>>> +                    domain = AMDGPU_GEM_DOMAIN_GTT; /* if user
>>>>>> really wants gtt, respect it */
>>>>> I'd spell VRAM and GTT in capital letters in the comments.
>>>>>
>>>>>
>>>>>> +            else if (adev->gmc.real_vram_size <= AMDGPU_SG_THRESHOLD)
>>>>>>                        domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>>> +            else
>>>>>> +                    domain = AMDGPU_GEM_DOMAIN_VRAM;
>>>>>>        }
>>>>> Is everything in place to deal with any issues that might occur when
>>>>> flipping between buffers in VRAM and GTT?
>>>>>
>>>> Besides this question, I am also wondering how to deal with the first
>>>> display buffer created in kernel at amdgpufb_create_pinned_object()?
>>> amdgpufb_create_pinned_object() calls
>>> amdgpu_display_supported_domains() for the initial domain and
>>> amdgpu_gem_object_create() uses that for the preferred domain so it
>>> won't change the behavior there.
>> That was actually something I was not sure about when I initially
>> created the patch.
>>
>> See the FB emulation expects some pointer into a linear PCI resource
>> (VRAM), I'm not sure if that can actually deal with something kmapped
>> into the vmap (GTT).
>>
>> Since this is seldom used and only with low resolution
> It's not that low resolution these days (normally the smallest native
> resolution of all connected displays, i.e. generally >= 1920x1080), i.e.
> on the order of ~10 MB of VRAM that can't be used for anything else.

Hui, what? Isn't the fb freed after X takes over?

>> it might be better to do the conservative approach and use VRAM here.
> Or somebody could try forcing GTT for this, and see what happens. :) If
> it works, I'd say GTT is actually preferable for this.

GTT is certainly preferable. Could just be that some common fb code 
calls io*map() on this and wonders why that fails horrible :)

I think as long as we don't see a crash during startup with boot screens 
everything should be fine.

Christian.

>> And we do a full mode set when switching away from FB emulation AFAIK,
>> so the move from VRAM to GTT shouldn't be a problem.
> Agreed.
>
>