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
