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