Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index a0992f0b8afd..0c2233ee6029 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, *pl = gbo->placement; }
-static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo, - bool evict, - struct ttm_resource *new_mem) +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo) { struct ttm_buffer_object *bo = &gbo->bo; struct drm_device *dev = bo->base.dev; @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo, struct ttm_operation_ctx *ctx, struct ttm_resource *new_mem) { - int ret; - - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); - ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); - if (ret) { - swap(*new_mem, gbo->bo.mem); - drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem); - swap(*new_mem, gbo->bo.mem); - } - return ret; + drm_gem_vram_bo_driver_move_notify(gbo); + return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); }
/* @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
gbo = drm_gem_vram_of_bo(bo);
- drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); + drm_gem_vram_bo_driver_move_notify(gbo); }
static int bo_driver_move(struct ttm_buffer_object *bo,
Remove the unused evict parameter and drop swapping bo->mem.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/qxl/qxl_ttm.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index b7f77eb685cb..47afe95d04a1 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -121,7 +121,6 @@ static struct ttm_tt *qxl_ttm_tt_create(struct ttm_buffer_object *bo, }
static void qxl_bo_move_notify(struct ttm_buffer_object *bo, - bool evict, struct ttm_resource *new_mem) { struct qxl_bo *qbo; @@ -144,29 +143,22 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *old_mem = &bo->mem; int ret;
- qxl_bo_move_notify(bo, evict, new_mem); + qxl_bo_move_notify(bo, new_mem);
ret = ttm_bo_wait_ctx(bo, ctx); if (ret) - goto out; + return ret;
if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) { ttm_bo_move_null(bo, new_mem); return 0; } - ret = ttm_bo_move_memcpy(bo, ctx, new_mem); -out: - if (ret) { - swap(*new_mem, bo->mem); - qxl_bo_move_notify(bo, false, new_mem); - swap(*new_mem, bo->mem); - } - return ret; + return ttm_bo_move_memcpy(bo, ctx, new_mem); }
static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) { - qxl_bo_move_notify(bo, false, NULL); + qxl_bo_move_notify(bo, NULL); }
static struct ttm_device_funcs qxl_bo_driver = {
Hi
Am 11.02.21 um 14:16 schrieb Christian König:
Remove the unused evict parameter and drop swapping bo->mem.
Could you provide the same patch for vram-helpers as well?
Best regards Thomas
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/qxl/qxl_ttm.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index b7f77eb685cb..47afe95d04a1 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -121,7 +121,6 @@ static struct ttm_tt *qxl_ttm_tt_create(struct ttm_buffer_object *bo, }
static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
bool evict, struct ttm_resource *new_mem)@@ -144,29 +143,22 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *old_mem = &bo->mem; int ret;
- qxl_bo_move_notify(bo, evict, new_mem);
qxl_bo_move_notify(bo, new_mem);
ret = ttm_bo_wait_ctx(bo, ctx); if (ret)
goto out;
return ret;if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) { ttm_bo_move_null(bo, new_mem); return 0; }
- ret = ttm_bo_move_memcpy(bo, ctx, new_mem);
-out:
- if (ret) {
swap(*new_mem, bo->mem);qxl_bo_move_notify(bo, false, new_mem);swap(*new_mem, bo->mem);- }
- return ret;
return ttm_bo_move_memcpy(bo, ctx, new_mem); }
static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) {
- qxl_bo_move_notify(bo, false, NULL);
qxl_bo_move_notify(bo, NULL); }
static struct ttm_device_funcs qxl_bo_driver = {
Just another leftover from a TTM cleanup.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/nouveau/nouveau_bo.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index ca2a8ae1938e..9bb8cee3df40 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -861,9 +861,8 @@ nouveau_bo_move_init(struct nouveau_drm *drm) NV_INFO(drm, "MM: using %s for buffer copies\n", name); }
-static void -nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, - struct ttm_resource *new_reg) +static void nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, + struct ttm_resource *new_reg) { struct nouveau_mem *mem = new_reg ? nouveau_mem(new_reg) : NULL; struct nouveau_bo *nvbo = nouveau_bo(bo); @@ -949,7 +948,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, return ret; }
- nouveau_bo_move_ntfy(bo, evict, new_reg); + nouveau_bo_move_ntfy(bo, new_reg); ret = ttm_bo_wait_ctx(bo, ctx); if (ret) goto out_ntfy; @@ -1014,9 +1013,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, } out_ntfy: if (ret) { - swap(*new_reg, bo->mem); - nouveau_bo_move_ntfy(bo, false, new_reg); - swap(*new_reg, bo->mem); + nouveau_bo_move_ntfy(bo, &bo->mem); } return ret; } @@ -1290,7 +1287,7 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl static void nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) { - nouveau_bo_move_ntfy(bo, false, NULL); + nouveau_bo_move_ntfy(bo, NULL); }
struct ttm_device_funcs nouveau_bo_driver = {
Instead of swapping bo->mem just give old and new as parameters.
Also drop unused parameters and code.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 9 +++++--- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 26 +++++++--------------- 3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index e65b00f8336d..21a72fa7b9fd 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -775,7 +775,8 @@ extern void vmw_resource_unreserve(struct vmw_resource *res, struct vmw_buffer_object *new_backup, unsigned long new_backup_offset); extern void vmw_query_move_notify(struct ttm_buffer_object *bo, - struct ttm_resource *mem); + struct ttm_resource *old_mem, + struct ttm_resource *new_mem); extern int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob); extern void vmw_resource_evict_all(struct vmw_private *dev_priv); extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c3a724e37104..35f02958ee2c 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -847,13 +847,15 @@ int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob) * vmw_query_move_notify - Read back cached query states * * @bo: The TTM buffer object about to move. - * @mem: The memory region @bo is moving to. + * @old_mem: The memory region @bo is moving from. + * @new_mem: The memory region @bo is moving to. * * Called before the query MOB is swapped out to read back cached query * states from the device. */ void vmw_query_move_notify(struct ttm_buffer_object *bo, - struct ttm_resource *mem) + struct ttm_resource *old_mem, + struct ttm_resource *new_mem) { struct vmw_buffer_object *dx_query_mob; struct ttm_device *bdev = bo->bdev; @@ -871,7 +873,8 @@ void vmw_query_move_notify(struct ttm_buffer_object *bo, }
/* If BO is being moved from MOB to system memory */ - if (mem->mem_type == TTM_PL_SYSTEM && bo->mem.mem_type == VMW_PL_MOB) { + if (new_mem->mem_type == TTM_PL_SYSTEM && + old_mem->mem_type == VMW_PL_MOB) { struct vmw_fence_obj *fence;
(void) vmw_query_readback_all(dx_query_mob); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index 63f10c865061..38801462134e 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -692,20 +692,19 @@ static int vmw_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource * * * @bo: The TTM buffer object about to move. * @evict: Unused - * @mem: The struct ttm_resource indicating to what memory + * @old_mem: The old memory where we move from + * @new_mem: The struct ttm_resource indicating to what memory * region the move is taking place. * * Calls move_notify for all subsystems needing it. * (currently only resources). */ static void vmw_move_notify(struct ttm_buffer_object *bo, - bool evict, - struct ttm_resource *mem) + struct ttm_resource *old_mem, + struct ttm_resource *new_mem) { - if (!mem) - return; - vmw_bo_move_notify(bo, mem); - vmw_query_move_notify(bo, mem); + vmw_bo_move_notify(bo, new_mem); + vmw_query_move_notify(bo, old_mem, new_mem); }
@@ -736,7 +735,7 @@ static int vmw_move(struct ttm_buffer_object *bo, return ret; }
- vmw_move_notify(bo, evict, new_mem); + vmw_move_notify(bo, &bo->mem, new_mem);
if (old_man->use_tt && new_man->use_tt) { if (bo->mem.mem_type == TTM_PL_SYSTEM) { @@ -758,18 +757,10 @@ static int vmw_move(struct ttm_buffer_object *bo, } return 0; fail: - swap(*new_mem, bo->mem); - vmw_move_notify(bo, false, new_mem); - swap(*new_mem, bo->mem); + vmw_move_notify(bo, new_mem, &bo->mem); return ret; }
-static void -vmw_delete_mem_notify(struct ttm_buffer_object *bo) -{ - vmw_move_notify(bo, false, NULL); -} - struct ttm_device_funcs vmw_bo_driver = { .ttm_tt_create = &vmw_ttm_tt_create, .ttm_tt_populate = &vmw_ttm_populate, @@ -779,7 +770,6 @@ struct ttm_device_funcs vmw_bo_driver = { .evict_flags = vmw_evict_flags, .move = vmw_move, .verify_access = vmw_verify_access, - .delete_mem_notify = vmw_delete_mem_notify, .swap_notify = vmw_swap_notify, .io_mem_reserve = &vmw_ttm_io_mem_reserve, };
On Thu, Feb 11, 2021 at 8:17 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Instead of swapping bo->mem just give old and new as parameters.
Also drop unused parameters and code.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 9 +++++--- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 26 +++++++--------------- 3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index e65b00f8336d..21a72fa7b9fd 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -775,7 +775,8 @@ extern void vmw_resource_unreserve(struct vmw_resource *res, struct vmw_buffer_object *new_backup, unsigned long new_backup_offset); extern void vmw_query_move_notify(struct ttm_buffer_object *bo,
struct ttm_resource *mem);
struct ttm_resource *old_mem,struct ttm_resource *new_mem);extern int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob); extern void vmw_resource_evict_all(struct vmw_private *dev_priv); extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c3a724e37104..35f02958ee2c 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -847,13 +847,15 @@ int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob)
- vmw_query_move_notify - Read back cached query states
- @bo: The TTM buffer object about to move.
- @mem: The memory region @bo is moving to.
- @old_mem: The memory region @bo is moving from.
*/
- @new_mem: The memory region @bo is moving to.
- Called before the query MOB is swapped out to read back cached query
- states from the device.
void vmw_query_move_notify(struct ttm_buffer_object *bo,
struct ttm_resource *mem)
struct ttm_resource *old_mem,struct ttm_resource *new_mem){ struct vmw_buffer_object *dx_query_mob; struct ttm_device *bdev = bo->bdev; @@ -871,7 +873,8 @@ void vmw_query_move_notify(struct ttm_buffer_object *bo, }
/* If BO is being moved from MOB to system memory */
if (mem->mem_type == TTM_PL_SYSTEM && bo->mem.mem_type == VMW_PL_MOB) {
if (new_mem->mem_type == TTM_PL_SYSTEM &&old_mem->mem_type == VMW_PL_MOB) { struct vmw_fence_obj *fence; (void) vmw_query_readback_all(dx_query_mob);diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index 63f10c865061..38801462134e 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -692,20 +692,19 @@ static int vmw_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *
- @bo: The TTM buffer object about to move.
- @evict: Unused
Evict is removed now. Can be dropped from the documentation.
Alex
- @mem: The struct ttm_resource indicating to what memory
- @old_mem: The old memory where we move from
*/
- @new_mem: The struct ttm_resource indicating to what memory
region the move is taking place.- Calls move_notify for all subsystems needing it.
- (currently only resources).
static void vmw_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_resource *mem)
struct ttm_resource *old_mem,struct ttm_resource *new_mem){
if (!mem)return;vmw_bo_move_notify(bo, mem);vmw_query_move_notify(bo, mem);
vmw_bo_move_notify(bo, new_mem);vmw_query_move_notify(bo, old_mem, new_mem);}
@@ -736,7 +735,7 @@ static int vmw_move(struct ttm_buffer_object *bo, return ret; }
vmw_move_notify(bo, evict, new_mem);
vmw_move_notify(bo, &bo->mem, new_mem); if (old_man->use_tt && new_man->use_tt) { if (bo->mem.mem_type == TTM_PL_SYSTEM) {@@ -758,18 +757,10 @@ static int vmw_move(struct ttm_buffer_object *bo, } return 0; fail:
swap(*new_mem, bo->mem);vmw_move_notify(bo, false, new_mem);swap(*new_mem, bo->mem);
vmw_move_notify(bo, new_mem, &bo->mem); return ret;}
-static void -vmw_delete_mem_notify(struct ttm_buffer_object *bo) -{
vmw_move_notify(bo, false, NULL);-}
struct ttm_device_funcs vmw_bo_driver = { .ttm_tt_create = &vmw_ttm_tt_create, .ttm_tt_populate = &vmw_ttm_populate, @@ -779,7 +770,6 @@ struct ttm_device_funcs vmw_bo_driver = { .evict_flags = vmw_evict_flags, .move = vmw_move, .verify_access = vmw_verify_access,
.delete_mem_notify = vmw_delete_mem_notify, .swap_notify = vmw_swap_notify, .io_mem_reserve = &vmw_ttm_io_mem_reserve,};
2.25.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
Am 11.02.21 um 14:16 schrieb Christian König:
Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup.
Yes this was introduced in a recent cleanup effort. Can you explain what the code intends to do? It seems as if it tries to "re-unmap the BO" if the move_memcpy fails.
If the move_memcpy fails now, it seems like we can live without reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)
Best regards Thomas
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index a0992f0b8afd..0c2233ee6029 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, *pl = gbo->placement; }
-static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
bool evict,struct ttm_resource *new_mem)+static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo) { struct ttm_buffer_object *bo = &gbo->bo; struct drm_device *dev = bo->base.dev; @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo, struct ttm_operation_ctx *ctx, struct ttm_resource *new_mem) {
- int ret;
- drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
- ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
- if (ret) {
swap(*new_mem, gbo->bo.mem);drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);swap(*new_mem, gbo->bo.mem);- }
- return ret;
drm_gem_vram_bo_driver_move_notify(gbo);
return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); }
/*
@@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
gbo = drm_gem_vram_of_bo(bo);
- drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
drm_gem_vram_bo_driver_move_notify(gbo); }
static int bo_driver_move(struct ttm_buffer_object *bo,
Am 11.02.21 um 15:52 schrieb Thomas Zimmermann:
Hi
Am 11.02.21 um 14:16 schrieb Christian König:
Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup.
Yes this was introduced in a recent cleanup effort. Can you explain what the code intends to do? It seems as if it tries to "re-unmap the BO" if the move_memcpy fails.
If the move_memcpy fails now, it seems like we can live without reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)
I think so, but I'm not 100% sure either.
The swap() -> notify -> swap() was just how TTM did it and that was moved into the drivers.
I'm now just trying to remove all the hard write accesses to bo->mem from drivers and stumbled over this here.
Thanks for the comments, Christian.
Best regards Thomas
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index a0992f0b8afd..0c2233ee6029 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, *pl = gbo->placement; } -static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo, - bool evict, - struct ttm_resource *new_mem) +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo) { struct ttm_buffer_object *bo = &gbo->bo; struct drm_device *dev = bo->base.dev; @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo, struct ttm_operation_ctx *ctx, struct ttm_resource *new_mem) { - int ret;
- drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); - ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); - if (ret) { - swap(*new_mem, gbo->bo.mem); - drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem); - swap(*new_mem, gbo->bo.mem); - } - return ret; + drm_gem_vram_bo_driver_move_notify(gbo); + return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); } /* @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) gbo = drm_gem_vram_of_bo(bo); - drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); + drm_gem_vram_bo_driver_move_notify(gbo); } static int bo_driver_move(struct ttm_buffer_object *bo,
Hi
Am 11.02.21 um 16:05 schrieb Christian König:
Am 11.02.21 um 15:52 schrieb Thomas Zimmermann:
Hi
Am 11.02.21 um 14:16 schrieb Christian König:
Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup.
Yes this was introduced in a recent cleanup effort. Can you explain what the code intends to do? It seems as if it tries to "re-unmap the BO" if the move_memcpy fails.
If the move_memcpy fails now, it seems like we can live without reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)
I think so, but I'm not 100% sure either.
The swap() -> notify -> swap() was just how TTM did it and that was moved into the drivers.
I'm now just trying to remove all the hard write accesses to bo->mem from drivers and stumbled over this here.
We already have a vmap count of zero; so unmapping the BO pages is fine at any time. The next caller of vmap will simple instantiate a new mapping.
Let me give this patch a test run tomorrow, but it seems correct.
Best regards Thomas
Thanks for the comments, Christian.
Best regards Thomas
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index a0992f0b8afd..0c2233ee6029 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, *pl = gbo->placement; } -static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo, - bool evict, - struct ttm_resource *new_mem) +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo) { struct ttm_buffer_object *bo = &gbo->bo; struct drm_device *dev = bo->base.dev; @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo, struct ttm_operation_ctx *ctx, struct ttm_resource *new_mem) { - int ret;
- drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); - ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); - if (ret) { - swap(*new_mem, gbo->bo.mem); - drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem); - swap(*new_mem, gbo->bo.mem); - } - return ret; + drm_gem_vram_bo_driver_move_notify(gbo); + return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); } /* @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) gbo = drm_gem_vram_of_bo(bo); - drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); + drm_gem_vram_bo_driver_move_notify(gbo); } static int bo_driver_move(struct ttm_buffer_object *bo,
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
Am 11.02.21 um 14:16 schrieb Christian König:
Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup.
Signed-off-by: Christian König christian.koenig@amd.com
Appears to work.
Reviewed-by: Thomas Zimmermann tzimmermann@suse.de Tested-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index a0992f0b8afd..0c2233ee6029 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, *pl = gbo->placement; }
-static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
bool evict,struct ttm_resource *new_mem)+static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo) { struct ttm_buffer_object *bo = &gbo->bo; struct drm_device *dev = bo->base.dev; @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo, struct ttm_operation_ctx *ctx, struct ttm_resource *new_mem) {
- int ret;
- drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
- ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
- if (ret) {
swap(*new_mem, gbo->bo.mem);drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);swap(*new_mem, gbo->bo.mem);- }
- return ret;
drm_gem_vram_bo_driver_move_notify(gbo);
return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem); }
/*
@@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
gbo = drm_gem_vram_of_bo(bo);
- drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
drm_gem_vram_bo_driver_move_notify(gbo); }
static int bo_driver_move(struct ttm_buffer_object *bo,
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Thomas Zimmermann