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

[Christian König]

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:
>>> Am 28.06.2017 um 04:33 schrieb John Brooks:
>>>> When the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is given by userspace,
>>>> it should only be treated as a hint to initially place a BO somewhere CPU
>>>> accessible, rather than having a permanent effect on BO placement.
>>>>
>>>> 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.

>>> 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.

Regards,
Christian.

> If we do need a new hint flag internally in the driver, we should simply
> translate AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED to the new flag in
> amdgpu_gem_create_ioctl, and not expose the new flag to userspace.
>
>
> But other than the question in my followup to the cover letter, this
> patch looks good to me as is.