On Fri, Nov 18, 2011 at 6:25 PM, Jerome Glisse <j.glisse@xxxxxxxxx> wrote: > 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 > Never mind, sent v3 with proper fix. swaped out memory is system placement and no gpu will make use of such placement so for validate to be usefull placement must be gart in which case move buffer is call and move notify will be too. radeon doesn't need change to it's move notify callback because it doesn't use the mem parameter (for the moment). Cheers, Jerome _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
