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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
