On 2017-06-29 09:56 PM, John Brooks wrote: > On Thu, Jun 29, 2017 at 11:35:53AM +0900, Michel Dänzer wrote: >> 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? >> >> >>>> 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. > I was looking at amdgpu_gem_metadata_ioctl(). It looked like it was possible to > query a BO's flags through that method. I don't know if it actually matters; it > was just a stab in the dark for any possibly tangible effect on userspace that > might arise from the kernel changing the flag. > > John And it looks like I am not correct about this as I misread it. It exposes bo->metadata_flags, not bo->flags. John >> >>> 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. >> >> 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. >> >> >> -- >> Earthling Michel Dänzer | http://www.amd.com >> Libre software enthusiast | Mesa and X developer > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
