From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Skeggs Subject: Re: [PATCH] drm/ttm: pass buffer object for bind/unbind callback Date: Sun, 20 Nov 2011 00:53:05 +1000 Message-ID: <1321714385.21551.6.camel@nisroch> References: <1321568442-8215-1-git-send-email-j.glisse@gmail.com> <4EC60FEC.3010703@vmware.com> <1321622128.13669.4.camel@nisroch> <4EC66BEB.1090900@vmware.com> <1321637208.18323.7.camel@nisroch> <4EC6E0DA.4060907@vmware.com> <1321662373.18323.15.camel@nisroch> <4EC77FDF.5030206@vmware.com> Reply-To: skeggsb@gmail.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-gx0-f177.google.com (mail-gx0-f177.google.com [209.85.161.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 94C9C9E745 for ; Sat, 19 Nov 2011 06:51:03 -0800 (PST) Received: by ggki1 with SMTP id i1so135664ggk.36 for ; Sat, 19 Nov 2011 06:51:03 -0800 (PST) In-Reply-To: <4EC77FDF.5030206@vmware.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Thomas Hellstrom Cc: Jerome Glisse , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Sat, 2011-11-19 at 11:07 +0100, Thomas Hellstrom wrote: > On 11/19/2011 01:26 AM, Ben Skeggs wrote: > > 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. > > > > I think you misunderstand me a little. > The swapout issue should be handled by calling a move_notify() to kill > the virtual GPU mappings. > > However, when moving data that has page tables pointing to it, one > should (IMHO) do: > > 1) Kill the old page tables > 2) Move the object > 3) Set up new page tables on demand. > > This is done by TTM for CPU page tables and I think that should be done > also for GPU page tables. move_notify() should be responsible for 1), > The driver execbuf for 3), and 3) needs only to be done for the > particular ring / fifo which is executing commands that touch the buffer. > > This leaves a clear and well-defined requirement on TTM: > Notify the driver when it needs to kill its GPU page tables. Ok. This I don't really object to really. I read the previous mail as that GPU mappings should only exist as long as the buffer is fenced, which would be a ridiculous amount of overhead. > > With the latest patch from jerome: > Notify the driver when it needs to rebuild it page tables. Also on error > paths, but not for swapins because no driver will probably set up GPU > page tables to SYSTEM_MEMORY. > > This is what I mean with fragile, and I much rather see the other approach. > > Ben, I didn't fully understand why you didn't want to use that approach. > Did you see a significant overhead with it. Now I'm more clear on what you meant, no, it wouldn't be a lot of overhead. However, move_notify() was never intended for the use you're proposing now, or the new ttm_mem_reg would never have been being passed in as a parameter... Really though, I also don't see why move_notify() can't just be called in all the situations a placement change happens (moves/deletion/swapout/swapin, whatever) and let the driver do whatever it needs to as a result. That seems just as well specified as changing move_notify() to mean "remove all gpu mappings". I totally agree that there's currently a couple of cases we miss here, which should indeed be fixed. However, I'm fine if there's a strong preference for your proposal. Thanks, Ben. > > Thanks, > /Thomas > > > > > > > > > >