[PATCH] drm/ttm: pass buffer object for bind/unbind callback

[Jerome Glisse]

On Fri, Nov 18, 2011 at 04:06:05PM +0100, Thomas Hellstrom wrote:
> On 11/18/2011 03:56 PM, Jerome Glisse wrote:
> >On Fri, Nov 18, 2011 at 03:30:03PM +0100, Thomas Hellstrom wrote:
> >>On 11/18/2011 02:15 PM, Ben Skeggs wrote:
> >>>On Fri, 2011-11-18 at 08:57 +0100, Thomas Hellstrom wrote:
> >>>>Jerome,
> >>>>
> >>>>I don't like this change for the following reasons
> >>>-snip-
> >>>
> >>>>>One can take advantage of move notify callback but, there are
> >>>>>corner case where bind/unbind might be call without move notify
> >>>>>being call (in error path mostly). So to make sure that each
> >>>>>virtual address space have a consistent view of wether a buffer
> >>>>>object is backed or not by system page it's better to pass the
> >>>>>buffer object to bind/unbind.
> >>>As I discussed previously with Jerome on IRC, I believe the move_notify
> >>>hook is sufficient.  I fixed a couple of issues back with it back when I
> >>>implemented support for this in nouveau.
> >>>
> >>>Jerome did point out two issues however, which I believe can be solved
> >>>easily enough.
> >>>
> >>>The first was that the error path after move_notify() has been called
> >>>doesn't reverse the move_notify() too, leaving incorrect page table
> >>>entries.  This, I think, could easily be solved by swapping bo->mem and
> >>>mem, and re-calling move_notify() to have the driver reverse whatever it
> >>>did.
> >>>
> >>>The second issue is that apparently move_notify() doesn't get called in
> >>>the destroy path.  Nouveau's GEM layer takes care of this for our
> >>>userspace bos, and we don't use any kernel bos that are mapped into a
> >>>channel's address space so we don't hit it.  This can probably be solved
> >>>easily enough too I expect.
> >>>
> >>>Ben.
> >>>
> >>OK. I understand. Surely if a move_notify is missing somewhere, that
> >>can easily be fixed.
> >>
> >>It might be good if we can outline how the virtual tables are set
> >>up. In my world, the following would happen:
> >>
> >>1) Pre command submission:
> >>
> >>a) reserve bo
> >>b) call ttm_bo_validate to set placement. move_notify will tear down
> >>any existing GPU page tables for the bo.
> >>c) Set up GPU page tables.
> >>d) Command submission
> >>e) unreserve_bo().
> >>
> >>
> >>2) When eviction happens:
> >>a) reserve bo
> >>b) move_notify is called to tear down page tables
> >>c) change placement
> >>d) Unreserve bo.
> >>
> >>Let's say an error occurs in 1d) Why would you need to undo 1c?
> >>
> >>Similarly if an error occurs in 2c) Why would you  need to undo 2b)?
> >>
> >Error is in ttm_bo_handle_move_mem move_notify is call before we do the
> >move but the move might fail.
> But even if the move fails in 1b) isn't it safe to just leave the
> virtual GPU map unbound, since GPU page tables will *always* be set
> up when placement is complete in 1c?

Call order is (assuming we are moving to system memory):
1 ttm_tt_populate
2 ttm_tt_bind
3 move_notify_callback
4 either driver move callback or ttm helper, both can fail
  on fail go to out_err which does :
ttm_tt_unbind & destroy so all page are gone now
Thing is move_notify have been call and it believes that the bo is no
in system ram with all the pages that are now free and can be reuse by
the kernel for other purpose.

Anyway i will add a callback to move notify there too. Patch is on the
way.

> >For destroy issue is when destroying a gtt buffer, it will just be
> >unbind, no call to ttm_bo_handle_move_mem which is the only function
> >calling back move_notify. (see ttm_bo_release_list).
> 
> Yes, here a fix is needed.
> 
> >I will fix this 2 corner case. I just wanted to make things symetrical.
> >By hooking up the virtual address space update with bind/unbind
> >
> >Note that at one point in the future the dream i have is no more
> >reserve, iommu page fault which is coming up (PASID process address
> >space id, with ATS address translation service and PRI page request
> >interface) would allow such things. Only synchronization will be
> >needed when moving object from system ram to vram or vice versa.
> 
> Indeed, but then TTM will probably be overkill, and something
> simpler can be implemented.

Cheers,
Jerome