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