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

[Jerome Glisse]

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