Hi Christian,
I've been trying to move towards the idea of not having TTM manage the global TT, I'm still not sure what the result would look like so I've been randomly trying out a direction or two,
There are some patches in : https://github.com/airlied/linux/commits/ttm-half-baked-ideas
a) it splits use_tt into two flags, one for system memory backing and one for binding, and tries to use them correctly. b) adds cpu_pin/unpin hooks for the amdgpu/radeon drivers to use for userptr.
I sort of envision being able to set the use_tt flag to false for drivers who don't want TTM to maintain their global TT, but I'm still not certain how that would look, I'd welcome any input there.
Thanks, Dave.
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Regards, Christian.
Am 15.09.20 um 07:30 schrieb Dave Airlie:
Hi Christian,
I've been trying to move towards the idea of not having TTM manage the global TT, I'm still not sure what the result would look like so I've been randomly trying out a direction or two,
There are some patches in : https://github.com/airlied/linux/commits/ttm-half-baked-ideas
a) it splits use_tt into two flags, one for system memory backing and one for binding, and tries to use them correctly. b) adds cpu_pin/unpin hooks for the amdgpu/radeon drivers to use for userptr.
I sort of envision being able to set the use_tt flag to false for drivers who don't want TTM to maintain their global TT, but I'm still not certain how that would look, I'd welcome any input there.
Thanks, Dave.
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
Dave.
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
* Driver calls ttm_bo_validate to put memory where it is cpu-mappable and gpu-bindable * On successful validate, driver sets up GPU bindings itself.
* move_notify only invalidates GPU bindings and should really return a void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
If the driver has no special cpu-mapping requirements, it should be perfectly legal for it to not provide any bind() or unbind() functionality.
/Thomas
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 16 Sep 2020 at 16:44, Thomas Hellström (Intel) thomas_os@shipmail.org wrote:
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
- Driver calls ttm_bo_validate to put memory where it is cpu-mappable
and gpu-bindable
On successful validate, driver sets up GPU bindings itself.
move_notify only invalidates GPU bindings and should really return a void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
If the driver has no special cpu-mapping requirements, it should be perfectly legal for it to not provide any bind() or unbind() functionality.
I think that is close to where we want to end up, it's just transitioning through a few intermediate stages to get to it.
I think I can likely put the binds into the driver move callback instead of the move_notify once I reorg things a bit more, and then maybe we could split the move out to happen post validate.
I'm just worried about intermediate state here, so if we validate something into VRAM we still have access to the CPU side backing store while it's moved in, and vice-versa.
Dave.
Am 16.09.20 um 08:56 schrieb Dave Airlie:
On Wed, 16 Sep 2020 at 16:44, Thomas Hellström (Intel) thomas_os@shipmail.org wrote:
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com...
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
- Driver calls ttm_bo_validate to put memory where it is cpu-mappable
and gpu-bindable
On successful validate, driver sets up GPU bindings itself.
move_notify only invalidates GPU bindings and should really return a void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
If the driver has no special cpu-mapping requirements, it should be perfectly legal for it to not provide any bind() or unbind() functionality.
I think that is close to where we want to end up, it's just transitioning through a few intermediate stages to get to it.
I think I can likely put the binds into the driver move callback instead of the move_notify once I reorg things a bit more, and then maybe we could split the move out to happen post validate.
I'm just worried about intermediate state here, so if we validate something into VRAM we still have access to the CPU side backing store while it's moved in, and vice-versa.
Yes exactly.
For the intermediate step I think the best would be to manually bind the TT object to the GART after calling ttm_bo_validate() like Thomas suggested.
Unbinding can then happen at two locations: 1. In move_notify() for the case GTT->SYSTEM. 2. When the TT object is destroyed.
Both of those should be inside the driver and not TTM.
Regards, Christian.
Dave.
Am 16.09.20 um 08:44 schrieb Thomas Hellström (Intel):
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com...
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
And exactly that's what we need to change. See TTM or more precisely the eviction handling should only manage where buffers are located and notify that driver that something needs to move.
Managing the whole binding/unbinding and actually moving the buffer around inside TTM was a bad idea to begin with.
In other words we have domains A, B, C.... and manage which BOs are inside those domains and can be evicted when necessary.
If the need arise to evict something the driver gets a notification which BO was picked for eviction and acts accordingly.
This means that the eviction_valuable, evict_flag, move_notify, binding, unbinding etc... callbacks should be removed in the long term.
Regards, Christian.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
- Driver calls ttm_bo_validate to put memory where it is cpu-mappable
and gpu-bindable
On successful validate, driver sets up GPU bindings itself.
move_notify only invalidates GPU bindings and should really return a
void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
If the driver has no special cpu-mapping requirements, it should be perfectly legal for it to not provide any bind() or unbind() functionality.
/Thomas
dri-devel mailing list dri-devel@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
On Wed, 16 Sep 2020 at 16:58, Christian König christian.koenig@amd.com wrote:
Am 16.09.20 um 08:44 schrieb Thomas Hellström (Intel):
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com...
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
And exactly that's what we need to change. See TTM or more precisely the eviction handling should only manage where buffers are located and notify that driver that something needs to move.
Managing the whole binding/unbinding and actually moving the buffer around inside TTM was a bad idea to begin with.
In other words we have domains A, B, C.... and manage which BOs are inside those domains and can be evicted when necessary.
If the need arise to evict something the driver gets a notification which BO was picked for eviction and acts accordingly.
This means that the eviction_valuable, evict_flag, move_notify, binding, unbinding etc... callbacks should be removed in the long term.
I've pushed a few more WIP direction changes into my branch now, I think the fun bit I have to face next is splitting the move_accel_cleanup and pipeline move cleanups.
but I'll keep in mind the goal of trying to get to a cleaner finished solution.
Dave.
On Wed, Sep 16, 2020 at 8:50 AM Thomas Hellström (Intel) thomas_os@shipmail.org wrote:
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
- Driver calls ttm_bo_validate to put memory where it is cpu-mappable
and gpu-bindable
On successful validate, driver sets up GPU bindings itself.
move_notify only invalidates GPU bindings and should really return a void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
pre-coffee and all, but for mmap I think we can fix that by extracting a few more of the building blocks from ttm_bo_vm, so that the driver's fault handler becomes roughly: 1. bo_reserve 2. bo_validate to make sure it's in the right spot, driver would control that. E.g. on i915 pwrite we first try the gtt without evicting, and then fall back to other mappings, maybe some driver wants to have that kind of fine-grained control too. 3. top half of ttm fault handler, up to but excluding ttm_tt_populate 4. again back in driver code, filling out all the tt itself (so keeping the tt structures probably good idea) 5. bottom half of ttm fault handler with all the pte insert code and prefaulting and all that 6. bo_unreserve
If the driver has no special cpu-mapping requirements, it should be perfectly legal for it to not provide any bind() or unbind() functionality.
In general I think separating the tt stuff from the placement stuff sounds like a very good idea. That should also allow us to share some vm/vma handling for per-process tt and all that, since doing that right (and from reading too many drivers amdgpu's code seems like a very nice template with the dma_resv in the vm for overall tracking, allowing O(1) execbuf and fun things like that) isn't that simple. And not all drivers that want these vm handling helpers might want ttm (thinking especially about soc stuff here). -Daniel
/Thomas
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 9/16/20 9:22 AM, Daniel Vetter wrote:
On Wed, Sep 16, 2020 at 8:50 AM Thomas Hellström (Intel) thomas_os@shipmail.org wrote:
On 9/16/20 6:28 AM, Dave Airlie wrote:
On Wed, 16 Sep 2020 at 14:19, Dave Airlie airlied@gmail.com wrote:
On Wed, 16 Sep 2020 at 00:12, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Hi Dave,
I think we should just completely nuke ttm_tt_bind() and ttm_tt_unbind() and all of that.
Drivers can to this from their move_notify() callback now instead.
Good plan, I've put a bunch of rework into the same branch,
https://github.com/airlied/linux/commits/ttm-half-baked-ideas
but I've fried my brain a bit, I'm having trouble reconciling move notify and unbinding in the right places, I feel like I'm circling around the answer but haven't hit it yet.
drm/ttm: add unbind to move notify paths.
In that tree is incorrect and I think where things fall apart, since if we are moving TTM to VRAM that will unbind the TTM object from the GTT at move notify time before the move has executed.
I'm feeling a move_complete_notify might be an idea, but I'm wondering if it's a bad idea.
Dave.
I don't know if this complicates things more, but move_notify was originally only thought to be an invalidation callback, and was never intended to drive any other actions in the driver than to invalidate various GPU bindings.
The idea was that TTM should really never set up any GPU bindings, but just provide memory where it was gpu-bindable and make sure it was CPU-mappable where needed. The "exception" was mappable AGP-type gpu-bindings, for the simple reason that they were needed to provide CPU-mappings on systems where you couldn't map the pages directly. But since we set up a GPU map on these systems anyway, many (most) drivers just made use of that, but others took the step further insisting on using move_notify() to set up GPU bindings, which was never intended and adds error paths in the TTM move code that are pretty hard to follow.
So if we're changing things here, I'd vote for the following:
- Driver calls ttm_bo_validate to put memory where it is cpu-mappable
and gpu-bindable
On successful validate, driver sets up GPU bindings itself.
move_notify only invalidates GPU bindings and should really return a void.
So that bind() and unbind() stuff is really only needed for cpu-map through aperture. If we ditch that, then we need to re-define the task of TTM to provide memory in a cpu-mappable location and figure how drivers that require cpu-map-through-aperture should handle this, since they can't use the TTM fault handler for that memory anymore. The same holds for drivers that want to manage their translation table themselves, and needs some cpu-mapping operations to go through the aperture rather than to the pages directly.
pre-coffee and all, but for mmap I think we can fix that by extracting a few more of the building blocks from ttm_bo_vm, so that the driver's fault handler becomes roughly:
- bo_reserve
- bo_validate to make sure it's in the right spot, driver would
control that. E.g. on i915 pwrite we first try the gtt without evicting, and then fall back to other mappings, maybe some driver wants to have that kind of fine-grained control too. 3. top half of ttm fault handler, up to but excluding ttm_tt_populate 4. again back in driver code, filling out all the tt itself (so keeping the tt structures probably good idea) 5. bottom half of ttm fault handler with all the pte insert code and prefaulting and all that 6. bo_unreserve
Yes, something like that should probably work just fine.
/Thomas
dri-devel@lists.freedesktop.org
-
Christian König -
Christian König -
Daniel Vetter -
Dave Airlie -
Thomas Hellström (Intel)