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

[Jerome Glisse]

On Fri, Nov 18, 2011 at 06:14:02PM -0500, Jerome Glisse wrote:
> On Fri, Nov 18, 2011 at 11:48:58PM +0100, Thomas Hellstrom wrote:
> > On 11/18/2011 06:26 PM, Ben Skeggs wrote:
> > >On Fri, 2011-11-18 at 15:30 +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.
> > >Nouveau doesn't do this exactly.  I wanted to avoid having to check if a
> > >bo was bound to a particular address space on every command submission.
> > 
> > It perhaps might be a good idea to revisit this?
> > I think using move_notify to set up a new placement before the data
> > has actually been moved is very fragile and not the intended use.
> > That would also save you from having to handle error paths. Also how
> > do you handle swapouts?
> > 
> > A quick check in c) that the virtual map hasn't been torn down by a
> > move_notify and, in that case, rebuild it would probably be next to
> > free. The virtual GPU mapping would then be valid only after
> > validation and while the bo is fenced or pinned.
> > 
> > Thanks,
> > Thomas
> > 
> 
> Will respin for swapout and to move move_notify once bo move is
> effective. This shouldn't change neither nouvau or radeon behavior.
> 
> Cheers,
> Jerome

So now that i think to, issue is with swapin we would need to know the
associated bo in ttm_tt_swapin. I believe bo move mem might never be
call on swapin. Actually i am can of worried that we have bug ther.
bo_validate will just check for bo->ttm to be null to trigger any
ttm_tt activity but bo->ttm won't be destroy on swapout. So on validate
from a swaped out buffer we never bring back the page. Or am i missing
something ?

Cheers,
Jerome