On 01/25/2012 10:41 AM, Ben Skeggs wrote:
My main concern is that we blindly and unnecessarily set up GPU bindings and
end up with unnecessary code in TTM, and furthermore that we communicate
that bad practice to future driver writers.
This "unnecessary code" is like 5 lines of cleanup if something fails,
hardly anything to be jumping up and down about :)
It's just not TTM's business, unless the GPU maps are mappable by the
CPU as well.
Also, What if the mapping setup in move_notify() fails?
Thomas, what do you suggest to move forward with this? Both of these
bugs are serious regressions that make nouveau unusable with the current
3.3-rc series. Ben.
My number one choice would of course be to have the drivers set up their
private GPU mappings after a
successful validate call, as I originally suggested and you agreed to.
If that's not possible (I realize it's late in the release series), I'll
ack this patch if you and Jerome agree not to block
attempts to move in that direction for future kernel releases.
I can't say I'm entirely happy with the plan honestly. To me, it still
seems more efficient to handle this when a move happens (comparatively
rare) and "map new backing storage into every vm that has a reference"
than to (on every single buffer of every single "exec" call) go "is this
buffer mapped into this channel's vm? yes, ok; no, lets go map it".
I'm not even sure how exactly I plan on storing this mapping efficiently
yet.. Scanning the BO's linked list of VMs it's mapped into for "if
(this_vma == chan->vma)" doesn't exactly sound performant.
As previously suggested, in the simplest case a bo could have a 'needs
remap' flag
that is set on gpu map teardown on move_notify(), and when this flag is
detected in validate,
go ahead and set up all needed maps and clear that flag.
This is the simplest case and more or less equivalent to the current
solution, except
maps aren't set up unless needed by at least one channel and there is a
clear way
to handle errors when GPU maps are set up.
A simple and straightforward fix that leaves the path open (if so
desired) to
handle finer channel granularity.
Or am I missing something?
/Thomas
Thanks,
Ben.
Thanks,
Thomas
Signed-off-by: Ben Skeggs<bskeggs@xxxxxxxxxx>
Cc: Jerome Glisse<j.glisse@xxxxxxxxx>
---
drivers/gpu/drm/nouveau/nouveau_bo.c | 4 ++++
drivers/gpu/drm/ttm/ttm_bo.c | 17 +++++++++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 724b41a..ec54364 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -812,6 +812,10 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
struct nouveau_bo *nvbo = nouveau_bo(bo);
struct nouveau_vma *vma;
+ /* ttm can now (stupidly) pass the driver bos it didn't create... */
+ if (bo->destroy != nouveau_bo_del_ttm)
+ return;
+
list_for_each_entry(vma,&nvbo->vma_list, head) {
if (new_mem&& new_mem->mem_type == TTM_PL_VRAM) {
nouveau_vm_map(vma, new_mem->mm_node);
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 2f0eab6..7c3a57d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -404,6 +404,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
}
}
+ if (bdev->driver->move_notify)
+ bdev->driver->move_notify(bo, mem);
+
if (!(old_man->flags& TTM_MEMTYPE_FLAG_FIXED)&&
!(new_man->flags& TTM_MEMTYPE_FLAG_FIXED))
ret = ttm_bo_move_ttm(bo, evict, no_wait_reserve, no_wait_gpu, mem);
@@ -413,11 +416,17 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
else
ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, mem);
- if (ret)
- goto out_err;
+ if (ret) {
+ if (bdev->driver->move_notify) {
+ struct ttm_mem_reg tmp_mem = *mem;
+ *mem = bo->mem;
+ bo->mem = tmp_mem;
+ bdev->driver->move_notify(bo, mem);
+ bo->mem = *mem;
+ }
- if (bdev->driver->move_notify)
- bdev->driver->move_notify(bo, mem);
+ goto out_err;
+ }
moved:
if (bo->evicted) {
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
