[PATCH 3/4] drm/amdgpu: Fix acquiring VM on large-BAR systems

[Felix Kühling]

Am 2018-03-24 um 04:34 AM schrieb Christian König:
> Am 23.03.2018 um 20:53 schrieb Felix Kuehling:
>> On 2018-03-23 03:35 PM, Christian König wrote:
>>> Am 23.03.2018 um 20:30 schrieb Felix Kuehling:
>>>> On large-BAR systems the VM page tables for compute are accessed by
>>>> the CPU. Always allow CPU access to the page directory so that it can
>>>> be used later by the CPU when a VM is converted to a compute VM.
>>> NAK, that means we initial place the PD in the visible are for GFX
>>> which is not desired (not much of an issue for Vega/Raven, but bad for
>>> older generations).
>> I'm not setting AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED. Doesn't that mean
>> the buffer can be placed in invisible VRAM initially?
>
> The difference is the preference. When AMDGPU_GEM_CREATE_NO_CPU_ACCESS
> is set we try to place it initially in invisible VRAM.

Where is that done? I'm looking at amdgpu_ttm_placement_from_domain. It
only looks at AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED. It does not
consider the AMDGPU_GEM_CREATE_NO_CPU_ACCESS flag. So creating the BO
without AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED should have the intended
effect of placing the BO into invisible VRAM initially.

As far as I can tell, my patch does not change the initial placement. It
only allows the BO to be kmapped later.

>
> Since VM page directories have a low chance of being evicted it will
> just stick in visible VRAM when it was placed initially there.
>
>>> Better to clear the flag later on and set the
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED and then re-validate.
>> The AMDGPU_GEM_CREATE_NO_CPU_ACCESS seems to mean "this BO must never be
>> CPU mapped". Clearing that flag later kind of defeats the purpose of
>> making such a declaration at allocation time. If I know that the buffer
>> may need CPU access at some point in the future, I figured I shouldn't
>> set the flag in the first place.
>
> Correct, but that only counts for userspace because userspace can't
> modify the flags :)
>
> If I remember correctly we also enforce it in the mmap IOCTL that the
> flag isn't set, but that doesn't really matter for the kernel.
>
> At least the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is cleared and
> set all the time in the kernel (but not for VM page tables).

Right, I see it being set while pinning. But page tables don't usually
get pinned.

>
>> FWIW, I didn't see any example of the AMDGPU_GEM_CREATE_NO_CPU_ACCESS
>> flag ever being removed from an existing BO.
>
> Yeah, but that should be harmless as far as I can see.
>
> If you want to avoid the move of the page directory on large BAR
> systems when switching a VM from GFX to compute we can also initially
> create the BO in the system domain.
>
> Thinking about that a bit more that is probably a good idea anyway
> because it avoids using VRAM by just opening the device.

Sure, I'd do that as a separate change because it actually changes the
initial placement, whereas my patch did not (as far as I can tell).

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Regards,
>>    Felix
>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 90ff79e..bc3557b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -2406,8 +2406,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>>>> struct amdgpu_vm *vm,
>>>>        if (vm->use_cpu_for_update)
>>>>            flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>>>>        else
>>>> -        flags |= (AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>>>> -                AMDGPU_GEM_CREATE_SHADOW);
>>>> +        flags |= AMDGPU_GEM_CREATE_SHADOW;
>>>>          size = amdgpu_vm_bo_size(adev, adev->vm_manager.root_level);
>>>>        r = amdgpu_bo_create(adev, size, align, true,
>>>> AMDGPU_GEM_DOMAIN_VRAM,
>> _______________________________________________
>> 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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180325/14448d92/attachment.sig>