[PATCH] drm/ttm: pass buffer object for bind/unbind callback

[Thomas Hellstrom]

On 11/19/2011 09:37 PM, Jerome Glisse wrote:
> On Sat, Nov 19, 2011 at 2:46 PM, Thomas Hellstrom<thellstrom at vmware.com>  wrote:
>    
>> On 11/19/2011 07:11 PM, Jerome Glisse wrote:
>>
>> On Sat, Nov 19, 2011 at 12:00 PM, Thomas Hellstrom
>> <thellstrom at vmware.com>  wrote:
>>
>>
>> On 11/19/2011 03:53 PM, Ben Skeggs wrote:
>>
>>
>> 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.
>>
>>
>>
>> I agree. What I meant was that the page tables wouldn't be considered valid
>> when the fence had signaled. However that was actually incorrect because
>> they would actually be valid until the next move_notify(). The intention was
>> never to tear down the mappings once the fence had signaled; that would have
>> been pretty stupid.
>>
>>
>>
>>
>>
>> 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...
>>
>>
>>
>> I suppose you're right. Still I think this is the right way to go. Since it
>> has the following advantages:
>>
>> 1) TTM doesn't need to care about the driver re-populating its GPU page
>> tables.
>> Since swapin is handled from the tt layer not the bo layer, this makes it a
>> bit easier on us.
>> 2) Transition to page-faulted GPU virtual maps is straightforward and
>> consistent. A non-page-faulting driver sets up the maps at CS time, A
>> pagefaulting driver can set them up directly from an irq handler without
>> reserving, since the bo is properly fenced or pinned when the pagefault
>> happens.
>> 3) A non-page-faulting driver knows at CS time exactly which
>> page-table-entries really do need populating, and can do this more
>> efficiently.
>>
>> So unless there are strong objections I suggest we should go this way.
>>
>> Even if this changes the semantics of the TTM<->  driver interface compared
>> to how Nouveau currently does things, it means that Jerome's current patch
>> is basically correct and doesn't any longer have any assumptions about
>> SYSTEM memory never being used for virtual GPU maps.
>>
>> Thanks,
>> Thomas.
>>
>>
>> I think it's better to let driver choose how it will handle its
>> virtual page table. For radeon  i update in move_notify in order to
>> minimize the number of update. I don't want to track if an object have
>> been updated or not against each page table. Of course this add
>> possibly useless overhead to move notify as we might update page table
>> to many time (bo from vram->gtt->system)
>>
>> I think if move notify is properly call once for each effective move
>> driver can do their own dance behind curtain.
>>
>> Cheers,
>> Jerome
>>
>>
>> Jerome,
>>
>> It's not really what the driver does that is the problem, it's what the
>> driver expects from the driver<->  TTM interface.
>>
>> That's why I'd really like a maintainable interface design before coding
>> patches that makes the interface a set of various workarounds.
>>
>> Enforcing this will also force driver writers to think twice before
>> implementing things in their own way adding another load of ad hoc callbacks
>> to TTM, without a clear specification but with the only purpose to fit a
>> specific driver implementation, so in a way it's our responsibility to
>> future driver writers to try to agree on a simple interface that works well
>> and allows drivers to do an efficient implementation, and that adds a little
>> maintenance burden on TTM.
>>
>> If a future driver writer looks at the Radeon code and replicates it,
>> because he thinks it's state of the art, and then finds out his code breaks
>> because he can't use SYSTEM memory for GPU page tables, or use his own
>> private LRU, or the code breaks with partially populated TTMs and finally
>> finds it's quite inefficient too because it unnecessarily populates page
>> tables, we've failed.
>>
>> This is really the point I'd like to make, but as a side note, I'm not
>> asking you to track each bo against each page table. What I'm asking you is
>> to not populate the page tables in bo_move() but in execbuf / pushbuf. The
>> possibility to track a bo against each page table comes as a nice benefit.
>>
>>
>> /Thomas
>>
>>      
> I don't see the issue with updating page table in move_notify. That's
> what i do for radeon virtual memory, doing it in command buffer ioctl
> is kind of too costly.

Could you describe why it is too costly? I previously asked Ben the same 
thing and he said it didn't come with a substantial performance penalty. 
Why would it be more than checking a single bool saying whether page 
tables had previously been killed or not? This would really help me 
understand why you want to do it from move_notify.

> Note that nouveau also update the page table in
> move_notify. So i think it's the right solution. TTM interface for
> move notify should just state that it will be call whenever a buffer
> change placement. Then we can add a comment on system buffer which can
> see there page disappear in thin air at any time. Note that when
> placement is system we mark all page table as invalid and point them
> to default entry (which is what nouveau is doing too).
>    

Well I've previously listed a number of disadvantages with this, which I 
think are still valid, but I fail to see any advantages? Just proposed 
workarounds for the disadvantages? (FWIW vmwgfx used to have GPU page 
tables pointing to system memory, and I'd say it would be a pretty 
common use-case for drivers that don't have a mappable GART).

/Thomas