[PATCH 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[Michel Dänzer]

On 29/06/17 05:23 PM, Christian König wrote:
> Am 29.06.2017 um 04:35 schrieb Michel Dänzer:
>> On 29/06/17 08:26 AM, John Brooks wrote:
>>> On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:
>>>>>
>>>>> Instead of the flag being set in stone at BO creation, set the flag
>>>>> when a
>>>>> page fault occurs so that it goes somewhere CPU-visible, and clear
>>>>> it when
>>>>> the BO is requested by the GPU.
>>>>>
>>>>> However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
>>>>> invisible VRAM, where they may promptly generate another page
>>>>> fault. When
>>>>> BOs are constantly moved back and forth like this, it is highly
>>>>> detrimental
>>>>> to performance. Only clear the flag on CS if:
>>>>>
>>>>> - The BO wasn't page faulted for a certain amount of time
>>>>> (currently 10
>>>>>    seconds), and
>>>>> - its last page fault didn't occur too soon (currently 500ms) after
>>>>> its
>>>>>    last CS request, or vice versa.
>>>>>
>>>>> Setting the flag in amdgpu_fault_reserve_notify() also means that
>>>>> we can
>>>>> remove the loop to restrict lpfn to the end of visible VRAM, because
>>>>> amdgpu_ttm_placement_init() will do it for us.
>>>> I'm fine with the general approach, but I'm still absolutely not
>>>> keen about
>>>> clearing the flag when userspace has originally specified it.
>> Is there any specific concern you have about that?
> 
> Yeah, quite a bunch actually. We want to use this flag for P2P buffer
> sharing in the future as well and I don't intent to add another one like
> CPU_ACCESS_REALLY_REQUIRED or something like this.

Won't a BO need to be pinned while it's being shared with another device?


>>>> Please add a new flag something like
>>>> AMDGPU_GEM_CREATE_CPU_ACCESS_HINT or
>>>> something like this.
>>> Is it the fact that we clear a flag that userspace expects not to
>>> have changed
>>> if it queries it later? I think that's the only effect of this that's
>>> directly
>>> visible to userspace code.
>> I don't see any way for userspace to query that.
>>
>>
>>> As for a new "hint" flag, I assume this new flag would be an
>>> alternative to the
>>> existing CPU_ACCESS_REQUIRED flag, and we'd change Mesa et al to use
>>> it in
>>> situations where the kernel *should* place a BO somewhere
>>> CPU-accessible, but
>>> is permitted to move it elsewhere. Is that correct?
>> That seems silly. The userspace flag could never be more than a hint.
>> Unfortunately we named it to suggest differently, but we have to live
>> with that.
> 
> No, just the other way around. The CPU_ACCESS_REQUIRED flag was
> introduced to note that it is MANDATORY to always have CPU access to the
> buffer.
> 
> It's just mesa which uses the flag as a hint to we could get CPU access.

Can you describe a specific scenario where userspace would actually need
the strict semantics? Otherwise I'm afraid what you're demanding boils
down to userspace churn for no benefit.


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