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