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

[Daniel Vetter]

On Thu, Jun 29, 2017 at 06:58:05PM +0900, Michel Dänzer wrote:
> 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?

That's an artifact of the current kernel implementation, I think we could
do better (but for current use-cases where we share a bunch of scanouts
and maybe a few pixmaps it's pointless). I wouldn't bet uapi on this never
changing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch