On Fri, Nov 18, 2011 at 03:30:03PM +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. 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)?

Error is in ttm_bo_handle_move_mem move_notify is call before we do the move but the move might fail.

For destroy issue is when destroying a gtt buffer, it will just be unbind, no call to ttm_bo_handle_move_mem which is the only function calling back move_notify. (see ttm_bo_release_list).

I will fix this 2 corner case. I just wanted to make things symetrical. By hooking up the virtual address space update with bind/unbind

Note that at one point in the future the dream i have is no more reserve, iommu page fault which is coming up (PASID process address space id, with ATS address translation service and PRI page request interface) would allow such things. Only synchronization will be needed when moving object from system ram to vram or vice versa.

Cheers, Jerome

On Fri, Nov 18, 2011 at 04:06:05PM +0100, Thomas Hellstrom wrote:

On 11/18/2011 03:56 PM, Jerome Glisse wrote:

...

On Fri, Nov 18, 2011 at 03:30:03PM +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. 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)?

Error is in ttm_bo_handle_move_mem move_notify is call before we do the move but the move might fail.

But even if the move fails in 1b) isn't it safe to just leave the virtual GPU map unbound, since GPU page tables will *always* be set up when placement is complete in 1c?

Call order is (assuming we are moving to system memory): 1 ttm_tt_populate 2 ttm_tt_bind 3 move_notify_callback 4 either driver move callback or ttm helper, both can fail on fail go to out_err which does : ttm_tt_unbind & destroy so all page are gone now Thing is move_notify have been call and it believes that the bo is no in system ram with all the pages that are now free and can be reuse by the kernel for other purpose.

Anyway i will add a callback to move notify there too. Patch is on the way.

...

For destroy issue is when destroying a gtt buffer, it will just be unbind, no call to ttm_bo_handle_move_mem which is the only function calling back move_notify. (see ttm_bo_release_list).

Yes, here a fix is needed.

...

I will fix this 2 corner case. I just wanted to make things symetrical. By hooking up the virtual address space update with bind/unbind

Note that at one point in the future the dream i have is no more reserve, iommu page fault which is coming up (PASID process address space id, with ATS address translation service and PRI page request interface) would allow such things. Only synchronization will be needed when moving object from system ram to vram or vice versa.

Indeed, but then TTM will probably be overkill, and something simpler can be implemented.

Cheers, Jerome

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

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

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

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

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

On Sat, Nov 19, 2011 at 12:00 PM, Thomas Hellstrom thellstrom@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

On Sat, Nov 19, 2011 at 2:46 PM, Thomas Hellstrom thellstrom@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@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. 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).

I agree on making ttm api clear an explicit about what is expected and i think move notify is pretty clear on what it's.

Cheers, Jerome

On Sat, Nov 19, 2011 at 4:01 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

On 11/19/2011 09:37 PM, Jerome Glisse wrote:

...

On Sat, Nov 19, 2011 at 2:46 PM, Thomas Hellstromthellstrom@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@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.

So we have one page table for each opener of the drm file. This page table is in a bo, it's validated in vram when the virtual address page is in use. There could be several active page table at the same time (GPU working on different work load all possibly in different virtual address space). The page table also cover vram (everything is a page wether it's in system memory or vram). So in the end the pagetable can be quite big like 1M or bigger. Each page table can have the same bo at different virtual address (so no sharing btw page table).

So if we were to build/update page table at each cs ioctl we are talking about moving around possibly several M of memory. Also if we want to go the update way only that means we need to track for each page table what was the last position of each bo we want to use, that sounds like a big loop.

With move notify we update all page table in which the bo is mapped into. Assumption is that bo don't move often and when it does it will soon be use. Also that way we don't have to track anything. We know that at all point in time all the page table have an up to date entry for all the bo.

...

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).

Ok there is 2 different thing for me, system placement in ttm is when the bo will never be use by the gpu (true for both radeon and nouveau). TTM_PL_TT is when a bo is bind to a gart and backed by system page. We know that the page will there as long as the bo is in use. PL_TT means the GPU page table point to system memory.

So for me TTM_PL_SYSTEM == bo might have page but they might dissappear at anytime, TTM_PL_TT bo has page and they are there for good until the bo is no longer in use.

So for me anytime bo placement change we need to call move notify (wether it's swapin, bo move or other ...) when placement become SYSTEM i assume that the bo is unbound and not in use and i update all page table for this bo to invalid entry.

Now, i don't get the common use case for gpu without gart. For me dma is kind of a gart and should have its placement like TTM_PL_TT

Cheers, Jerome

On Sat, Nov 19, 2011 at 5:37 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

On 11/19/2011 10:22 PM, Jerome Glisse wrote:

...

On Sat, Nov 19, 2011 at 4:01 PM, Thomas Hellstromthellstrom@vmware.com  wrote:

...

On 11/19/2011 09:37 PM, Jerome Glisse wrote:

...

On Sat, Nov 19, 2011 at 2:46 PM, Thomas Hellstromthellstrom@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@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.

So we have one page table for each opener of the drm file. This page table is in a bo, it's validated in vram when the virtual address page is in use. There could be several active page table at the same time (GPU working on different work load all possibly in different virtual address space). The page table also cover vram (everything is a page wether it's in system memory or vram). So in the end the pagetable can be quite big like 1M or bigger. Each page table can have the same bo at different virtual address (so no sharing btw page table).

So if we were to build/update page table at each cs ioctl we are talking about moving around possibly several M of memory. Also if we want to go the update way only that means we need to track for each page table what was the last position of each bo we want to use, that sounds like a big loop.

With move notify we update all page table in which the bo is mapped into. Assumption is that bo don't move often and when it does it will soon be use. Also that way we don't have to track anything. We know that at all point in time all the page table have an up to date entry for all the bo.

As mentioned previously, and in the discussion with Ben, the page tables would not need to be rebuilt on each CS. They would be rebuilt only on the first CS following a move_notify that caused a page table invalidation.

move_notify: if (is_incompatible(new_mem_type)) {  bo->page_tables_invalid = true;  invalidate_page_tables(bo); }

command_submission: if (bo->page_tables_invalid) {   set_up_page_tables(bo);   bo->page_tables_invalid = false; }

Why is it different from updating page table in move notify ? I don't see any bonus here, all the information we need are already available in 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).

Ok there is 2 different thing for me, system placement in ttm is when the bo will never be use by the gpu (true for both radeon and nouveau). TTM_PL_TT is when a bo is bind to a gart and backed by system page. We know that the page will there as long as the bo is in use. PL_TT means the GPU page table point to system memory.

Memory types in TTM are completely orthogonal to allowed GPU usage. The GPU may access a bo if it's reserved, fenced or pinned, regardless of its placement.

A TT memory type is a *single* GPU aperture that may be mapped from the aperture side by the CPU (AGP). It may also be used by a single unmappable aperture that wants to use TTM's range management and eviction (vmwgfx GMR). The driver can define more than one such memory type (psb), but a bo can only be placed in one of those at a time, so this approach is unsuitable for multiple apertures pointing to the same pages.

radeon virtual memory have a special address space, the system address space, it's managed by ttm through a TTM_TT (exact same code as current one). All the other address space are not managed by ttm but we require a bo to be bound to ttm_tt to be use, thought we can relax that. That's the reason why i consider system placement as different.

Hardware with multiple page tables (one per context or file descriptor) would probably want to use SYSTEM memory directly (unichrome SGDMA, old vmwgfx GMR), unless the per-context page tables can only point to pages that already reside in a bigger GPU-wide translation table. (IIRC i965 might be an example of the latter, but I'm unsure).

Cheers, Jerome

On Sun, Nov 20, 2011 at 4:30 AM, Thomas Hellstrom thellstrom@vmware.com wrote:

On 11/19/2011 11:54 PM, Jerome Glisse wrote:

As mentioned previously, and in the discussion with Ben, the page tables would not need to be rebuilt on each CS. They would be rebuilt only on the first CS following a move_notify that caused a page table invalidation.

move_notify: if (is_incompatible(new_mem_type)) {  bo->page_tables_invalid = true;  invalidate_page_tables(bo); }

command_submission: if (bo->page_tables_invalid) {   set_up_page_tables(bo);   bo->page_tables_invalid = false; }

Why is it different from updating page table in move notify ? I don't see any bonus here, all the information we need are already available in move_notify.

I've iterated the pros of this approach at least two times before, but for completeness let's do it again:

8<----------------------------------------------------------------------------------------------------

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.

8<-----------------------------------------------------------------------------------------------------

And some extra items like partially populated TTMs that were mentioned elsewhere.

If done in move_notify i don't see why 1 would be different or 2. I agree that in some case 3 is true. Given when move notify is call the ttm_tt is always fully populated at that point (only exception is in destroy path but it's a special on its own). If driver populate in move_notify is doesn't change anything from ttm pov.

Memory types in TTM are completely orthogonal to allowed GPU usage. The GPU may access a bo if it's reserved, fenced or pinned, regardless of its placement.

A TT memory type is a *single* GPU aperture that may be mapped from the aperture side by the CPU (AGP). It may also be used by a single unmappable aperture that wants to use TTM's range management and eviction (vmwgfx GMR). The driver can define more than one such memory type (psb), but a bo can only be placed in one of those at a time, so this approach is unsuitable for multiple apertures pointing to the same pages.

radeon virtual memory have a special address space, the system address space, it's managed by ttm through a TTM_TT (exact same code as current one). All the other address space are not managed by ttm but we require a bo to be bound to ttm_tt to be use, thought we can relax that. That's the reason why i consider system placement as different.

Yes for Radeon system memory may be different, and that's fine. But as also previously mentioned we're trying to design a generic interface here, in which we need to consider GPU- mappable system memory.

I think the pros of this interface design compared to populating in bo_move are pretty well established, so can you please explain why you keep arguing against it? What is it that I have missed?

/Thomas

It's just i absolutely see no diff in doing it in move_notify (point 1 and 2 doesn't change from my pov). I fail to see the pro that's all.

Cheers, Jerome