On Fri, 2011-11-18 at 23:48 +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? I don't see how it's fragile, there's only the one error path after that point that needs to undo it. And, what *is* the expected use then? I see it as "tell the driver the buffer is moving so it can do whatever is necessary as a result". Swapouts are a missed case though, indeed. > > 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. This alone doesn't solve the swapouts issue either unless you're also wanting us to unmap once a buffer becomes unfenced, which would lead to loads of completely unnecessary map/unmaps. Ben. > > Thanks, > Thomas > > > We allocate the virtual address when a client first gets a reference to > > a bo (ie. creates new buffer, or opens an existing handle). If the > > buffer happens to be in VRAM or TT memory at this point, it's also > > mapped. > > > > And as buffers move around, we get called by TTM through move_notify() > > of course, and do any mapping/unmapping that's necessary as a result. > > > > Ben. > > > > > >> d) Command submission > >> e) unreserve_bo(). > >> > >> > >> 2) When eviction happens: > >> a) reserve bo > >> b) move_notify is called to tear down page tables > >> c) change placement > >> d) Unreserve bo. > >> > >> Let's say an error occurs in 1d) Why would you need to undo 1c? > >> > >> Similarly if an error occurs in 2c) Why would you need to undo 2b)? > >> > >> Thanks, > >> Thomas > >> > >> > >> > >> > >> > >> > >> > >> > >> > > > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
