* [PATCH 0/3] ttm: change move notify + revised multi hop patch
@ 2020-10-21 4:40 Dave Airlie
2020-10-21 4:40 ` [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify Dave Airlie
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Dave Airlie @ 2020-10-21 4:40 UTC (permalink / raw)
To: dri-devel; +Cc: christian.koenig
This series is an update of what I posted yesterday, with two other cleanups.
1 just cleans up the remaining move_notify callback back to delete_mem_callback,
this should allow futher cleanups in the drivers.
2. trivial cleanup
3. the move hop patch. This removes the hack of storing the hop placement
in new_mem and puts it in a a new hop ttm_place parameter.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify 2020-10-21 4:40 [PATCH 0/3] ttm: change move notify + revised multi hop patch Dave Airlie @ 2020-10-21 4:40 ` Dave Airlie 2020-10-21 9:58 ` Christian König 2020-10-21 4:40 ` [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter Dave Airlie 2020-10-21 4:40 ` [PATCH 3/3] drm/ttm: avoid multihop moves in drivers Dave Airlie 2 siblings, 1 reply; 13+ messages in thread From: Dave Airlie @ 2020-10-21 4:40 UTC (permalink / raw) To: dri-devel; +Cc: christian.koenig From: Dave Airlie <airlied@redhat.com> The move notify callback is only used in one place, this should be removed in the future, but for now just rename it to the use case which is to notify the driver that the GPU memory is to be deleted. Drivers can be cleaned up after this separately. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++++++- drivers/gpu/drm/drm_gem_vram_helper.c | 8 +++----- drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +++++++- drivers/gpu/drm/qxl/qxl_ttm.c | 7 ++++++- drivers/gpu/drm/radeon/radeon_ttm.c | 8 +++++++- drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 8 +++++++- include/drm/ttm/ttm_bo_driver.h | 10 ++-------- 8 files changed, 41 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 87e10a212b8a..62f9194b1dd1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1730,6 +1730,12 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, return ret; } +static void +amdgpu_bo_delete_mem_notify(struct ttm_buffer_object *bo) +{ + amdgpu_bo_move_notify(bo, false, NULL); +} + static struct ttm_bo_driver amdgpu_bo_driver = { .ttm_tt_create = &amdgpu_ttm_tt_create, .ttm_tt_populate = &amdgpu_ttm_tt_populate, @@ -1739,7 +1745,7 @@ static struct ttm_bo_driver amdgpu_bo_driver = { .evict_flags = &amdgpu_evict_flags, .move = &amdgpu_bo_move, .verify_access = &amdgpu_verify_access, - .move_notify = &amdgpu_bo_move_notify, + .delete_mem_notify = &amdgpu_bo_delete_mem_notify, .release_notify = &amdgpu_bo_release_notify, .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, .io_mem_pfn = amdgpu_ttm_io_mem_pfn, diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 19087b22bdbb..9da823eb0edd 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -949,9 +949,7 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo, drm_gem_vram_bo_driver_evict_flags(gbo, placement); } -static void bo_driver_move_notify(struct ttm_buffer_object *bo, - bool evict, - struct ttm_resource *new_mem) +static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) { struct drm_gem_vram_object *gbo; @@ -961,7 +959,7 @@ static void bo_driver_move_notify(struct ttm_buffer_object *bo, gbo = drm_gem_vram_of_bo(bo); - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); + drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); } static int bo_driver_move(struct ttm_buffer_object *bo, @@ -1002,7 +1000,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .move = bo_driver_move, - .move_notify = bo_driver_move_notify, + .delete_mem_notify = bo_driver_delete_mem_notify, .io_mem_reserve = bo_driver_io_mem_reserve, }; diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 70b6f3b1ae85..acff82afe260 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1401,6 +1401,12 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl dma_resv_add_shared_fence(resv, &fence->base); } +static void +nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) +{ + nouveau_bo_move_ntfy(bo, false, NULL); +} + struct ttm_bo_driver nouveau_bo_driver = { .ttm_tt_create = &nouveau_ttm_tt_create, .ttm_tt_populate = &nouveau_ttm_tt_populate, @@ -1408,7 +1414,7 @@ struct ttm_bo_driver nouveau_bo_driver = { .ttm_tt_destroy = &nouveau_ttm_tt_destroy, .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = nouveau_bo_evict_flags, - .move_notify = nouveau_bo_move_ntfy, + .delete_mem_notify = nouveau_bo_delete_mem_notify, .move = nouveau_bo_move, .verify_access = nouveau_bo_verify_access, .io_mem_reserve = &nouveau_ttm_io_mem_reserve, diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index 1cc3c14bc684..b52a4563b47b 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -166,6 +166,11 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, return ret; } +static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) +{ + qxl_bo_move_notify(bo, false, NULL); +} + static struct ttm_bo_driver qxl_bo_driver = { .ttm_tt_create = &qxl_ttm_tt_create, .ttm_tt_destroy = &qxl_ttm_backend_destroy, @@ -173,7 +178,7 @@ static struct ttm_bo_driver qxl_bo_driver = { .evict_flags = &qxl_evict_flags, .move = &qxl_bo_move, .io_mem_reserve = &qxl_ttm_io_mem_reserve, - .move_notify = &qxl_bo_move_notify, + .delete_mem_notify = &qxl_bo_delete_mem_notify, }; static int qxl_ttm_init_mem_type(struct qxl_device *qdev, diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index cd454e5c802f..321c09d20c6c 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -824,6 +824,12 @@ bool radeon_ttm_tt_is_readonly(struct radeon_device *rdev, return !!(gtt->userflags & RADEON_GEM_USERPTR_READONLY); } +static void +radeon_bo_delete_mem_notify(struct ttm_buffer_object *bo) +{ + radeon_bo_move_notify(bo, false, NULL); +} + static struct ttm_bo_driver radeon_bo_driver = { .ttm_tt_create = &radeon_ttm_tt_create, .ttm_tt_populate = &radeon_ttm_tt_populate, @@ -833,7 +839,7 @@ static struct ttm_bo_driver radeon_bo_driver = { .evict_flags = &radeon_evict_flags, .move = &radeon_bo_move, .verify_access = &radeon_verify_access, - .move_notify = &radeon_bo_move_notify, + .delete_mem_notify = &radeon_bo_delete_mem_notify, .io_mem_reserve = &radeon_ttm_io_mem_reserve, }; diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 2b578012cdef..e2afab3d97ee 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -284,8 +284,8 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) { - if (bo->bdev->driver->move_notify) - bo->bdev->driver->move_notify(bo, false, NULL); + if (bo->bdev->driver->delete_mem_notify) + bo->bdev->driver->delete_mem_notify(bo); ttm_bo_tt_destroy(bo); ttm_resource_free(bo, &bo->mem); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index de25cf016be2..88be48ad0344 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -771,6 +771,12 @@ static int vmw_move(struct ttm_buffer_object *bo, return ret; } +static void +vmw_delete_mem_notify(struct ttm_buffer_object *bo) +{ + vmw_move_notify(bo, false, NULL); +} + struct ttm_bo_driver vmw_bo_driver = { .ttm_tt_create = &vmw_ttm_tt_create, .ttm_tt_populate = &vmw_ttm_populate, @@ -780,7 +786,7 @@ struct ttm_bo_driver vmw_bo_driver = { .evict_flags = vmw_evict_flags, .move = vmw_move, .verify_access = vmw_verify_access, - .move_notify = vmw_move_notify, + .delete_mem_notify = vmw_delete_mem_notify, .swap_notify = vmw_swap_notify, .io_mem_reserve = &vmw_ttm_io_mem_reserve, }; diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 72f106b335e9..29f6a1d1c853 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -156,15 +156,9 @@ struct ttm_bo_driver { struct file *filp); /** - * Hook to notify driver about a driver move so it - * can do tiling things and book-keeping. - * - * @evict: whether this move is evicting the buffer from the graphics - * address space + * Hook to notify driver about a resource delete. */ - void (*move_notify)(struct ttm_buffer_object *bo, - bool evict, - struct ttm_resource *new_mem); + void (*delete_mem_notify)(struct ttm_buffer_object *bo); /** * notify the driver that we're about to swap out this bo -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify 2020-10-21 4:40 ` [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify Dave Airlie @ 2020-10-21 9:58 ` Christian König 2020-10-28 9:16 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Christian König @ 2020-10-21 9:58 UTC (permalink / raw) To: Dave Airlie, dri-devel Am 21.10.20 um 06:40 schrieb Dave Airlie: > From: Dave Airlie <airlied@redhat.com> > > The move notify callback is only used in one place, this should > be removed in the future, but for now just rename it to the use > case which is to notify the driver that the GPU memory is to be > deleted. Probably the right thing to do is to call the move callback with move(from, NULL) in this case as well. And then driver can call the necessary function to throw away the backing store pipelined. > > Drivers can be cleaned up after this separately. > > Signed-off-by: Dave Airlie <airlied@redhat.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++++++- > drivers/gpu/drm/drm_gem_vram_helper.c | 8 +++----- > drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +++++++- > drivers/gpu/drm/qxl/qxl_ttm.c | 7 ++++++- > drivers/gpu/drm/radeon/radeon_ttm.c | 8 +++++++- > drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 8 +++++++- > include/drm/ttm/ttm_bo_driver.h | 10 ++-------- > 8 files changed, 41 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 87e10a212b8a..62f9194b1dd1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1730,6 +1730,12 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, > return ret; > } > > +static void > +amdgpu_bo_delete_mem_notify(struct ttm_buffer_object *bo) > +{ > + amdgpu_bo_move_notify(bo, false, NULL); > +} > + > static struct ttm_bo_driver amdgpu_bo_driver = { > .ttm_tt_create = &amdgpu_ttm_tt_create, > .ttm_tt_populate = &amdgpu_ttm_tt_populate, > @@ -1739,7 +1745,7 @@ static struct ttm_bo_driver amdgpu_bo_driver = { > .evict_flags = &amdgpu_evict_flags, > .move = &amdgpu_bo_move, > .verify_access = &amdgpu_verify_access, > - .move_notify = &amdgpu_bo_move_notify, > + .delete_mem_notify = &amdgpu_bo_delete_mem_notify, > .release_notify = &amdgpu_bo_release_notify, > .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, > .io_mem_pfn = amdgpu_ttm_io_mem_pfn, > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index 19087b22bdbb..9da823eb0edd 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -949,9 +949,7 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo, > drm_gem_vram_bo_driver_evict_flags(gbo, placement); > } > > -static void bo_driver_move_notify(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_resource *new_mem) > +static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) > { > struct drm_gem_vram_object *gbo; > > @@ -961,7 +959,7 @@ static void bo_driver_move_notify(struct ttm_buffer_object *bo, > > gbo = drm_gem_vram_of_bo(bo); > > - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); > + drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); > } > > static int bo_driver_move(struct ttm_buffer_object *bo, > @@ -1002,7 +1000,7 @@ static struct ttm_bo_driver bo_driver = { > .eviction_valuable = ttm_bo_eviction_valuable, > .evict_flags = bo_driver_evict_flags, > .move = bo_driver_move, > - .move_notify = bo_driver_move_notify, > + .delete_mem_notify = bo_driver_delete_mem_notify, > .io_mem_reserve = bo_driver_io_mem_reserve, > }; > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 70b6f3b1ae85..acff82afe260 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -1401,6 +1401,12 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl > dma_resv_add_shared_fence(resv, &fence->base); > } > > +static void > +nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) > +{ > + nouveau_bo_move_ntfy(bo, false, NULL); > +} > + > struct ttm_bo_driver nouveau_bo_driver = { > .ttm_tt_create = &nouveau_ttm_tt_create, > .ttm_tt_populate = &nouveau_ttm_tt_populate, > @@ -1408,7 +1414,7 @@ struct ttm_bo_driver nouveau_bo_driver = { > .ttm_tt_destroy = &nouveau_ttm_tt_destroy, > .eviction_valuable = ttm_bo_eviction_valuable, > .evict_flags = nouveau_bo_evict_flags, > - .move_notify = nouveau_bo_move_ntfy, > + .delete_mem_notify = nouveau_bo_delete_mem_notify, > .move = nouveau_bo_move, > .verify_access = nouveau_bo_verify_access, > .io_mem_reserve = &nouveau_ttm_io_mem_reserve, > diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > index 1cc3c14bc684..b52a4563b47b 100644 > --- a/drivers/gpu/drm/qxl/qxl_ttm.c > +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > @@ -166,6 +166,11 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, > return ret; > } > > +static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) > +{ > + qxl_bo_move_notify(bo, false, NULL); > +} > + > static struct ttm_bo_driver qxl_bo_driver = { > .ttm_tt_create = &qxl_ttm_tt_create, > .ttm_tt_destroy = &qxl_ttm_backend_destroy, > @@ -173,7 +178,7 @@ static struct ttm_bo_driver qxl_bo_driver = { > .evict_flags = &qxl_evict_flags, > .move = &qxl_bo_move, > .io_mem_reserve = &qxl_ttm_io_mem_reserve, > - .move_notify = &qxl_bo_move_notify, > + .delete_mem_notify = &qxl_bo_delete_mem_notify, > }; > > static int qxl_ttm_init_mem_type(struct qxl_device *qdev, > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index cd454e5c802f..321c09d20c6c 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -824,6 +824,12 @@ bool radeon_ttm_tt_is_readonly(struct radeon_device *rdev, > return !!(gtt->userflags & RADEON_GEM_USERPTR_READONLY); > } > > +static void > +radeon_bo_delete_mem_notify(struct ttm_buffer_object *bo) > +{ > + radeon_bo_move_notify(bo, false, NULL); > +} > + > static struct ttm_bo_driver radeon_bo_driver = { > .ttm_tt_create = &radeon_ttm_tt_create, > .ttm_tt_populate = &radeon_ttm_tt_populate, > @@ -833,7 +839,7 @@ static struct ttm_bo_driver radeon_bo_driver = { > .evict_flags = &radeon_evict_flags, > .move = &radeon_bo_move, > .verify_access = &radeon_verify_access, > - .move_notify = &radeon_bo_move_notify, > + .delete_mem_notify = &radeon_bo_delete_mem_notify, > .io_mem_reserve = &radeon_ttm_io_mem_reserve, > }; > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 2b578012cdef..e2afab3d97ee 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -284,8 +284,8 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > > static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > { > - if (bo->bdev->driver->move_notify) > - bo->bdev->driver->move_notify(bo, false, NULL); > + if (bo->bdev->driver->delete_mem_notify) > + bo->bdev->driver->delete_mem_notify(bo); > > ttm_bo_tt_destroy(bo); > ttm_resource_free(bo, &bo->mem); > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > index de25cf016be2..88be48ad0344 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > @@ -771,6 +771,12 @@ static int vmw_move(struct ttm_buffer_object *bo, > return ret; > } > > +static void > +vmw_delete_mem_notify(struct ttm_buffer_object *bo) > +{ > + vmw_move_notify(bo, false, NULL); > +} > + > struct ttm_bo_driver vmw_bo_driver = { > .ttm_tt_create = &vmw_ttm_tt_create, > .ttm_tt_populate = &vmw_ttm_populate, > @@ -780,7 +786,7 @@ struct ttm_bo_driver vmw_bo_driver = { > .evict_flags = vmw_evict_flags, > .move = vmw_move, > .verify_access = vmw_verify_access, > - .move_notify = vmw_move_notify, > + .delete_mem_notify = vmw_delete_mem_notify, > .swap_notify = vmw_swap_notify, > .io_mem_reserve = &vmw_ttm_io_mem_reserve, > }; > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index 72f106b335e9..29f6a1d1c853 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -156,15 +156,9 @@ struct ttm_bo_driver { > struct file *filp); > > /** > - * Hook to notify driver about a driver move so it > - * can do tiling things and book-keeping. > - * > - * @evict: whether this move is evicting the buffer from the graphics > - * address space > + * Hook to notify driver about a resource delete. > */ > - void (*move_notify)(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_resource *new_mem); > + void (*delete_mem_notify)(struct ttm_buffer_object *bo); > > /** > * notify the driver that we're about to swap out this bo _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify 2020-10-21 9:58 ` Christian König @ 2020-10-28 9:16 ` Daniel Vetter 2020-10-28 9:53 ` Christian König 0 siblings, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2020-10-28 9:16 UTC (permalink / raw) To: Christian König; +Cc: dri-devel On Wed, Oct 21, 2020 at 11:58:45AM +0200, Christian König wrote: > Am 21.10.20 um 06:40 schrieb Dave Airlie: > > From: Dave Airlie <airlied@redhat.com> > > > > The move notify callback is only used in one place, this should > > be removed in the future, but for now just rename it to the use > > case which is to notify the driver that the GPU memory is to be > > deleted. > > Probably the right thing to do is to call the move callback with move(from, > NULL) in this case as well. > > And then driver can call the necessary function to throw away the backing > store pipelined. > > > > > Drivers can be cleaned up after this separately. > > > > Signed-off-by: Dave Airlie <airlied@redhat.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> Not sure where to best ask this question, but while reading code I stumbled over the bo->pin_count check in ttm_bo_release(). And I'm confused. Allowing a bo to be pinned without holding a full reference to it feels like a pretty serious bug. Where&why is that needed? I'm kinda tempted to wrap this in a WARN_ON, just to make sure there's no surprises in usage (and maybe warn in unpin if we drop the pin count with the refcount at 0 already). -Daniel > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++++++- > > drivers/gpu/drm/drm_gem_vram_helper.c | 8 +++----- > > drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +++++++- > > drivers/gpu/drm/qxl/qxl_ttm.c | 7 ++++++- > > drivers/gpu/drm/radeon/radeon_ttm.c | 8 +++++++- > > drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- > > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 8 +++++++- > > include/drm/ttm/ttm_bo_driver.h | 10 ++-------- > > 8 files changed, 41 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > index 87e10a212b8a..62f9194b1dd1 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > @@ -1730,6 +1730,12 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, > > return ret; > > } > > +static void > > +amdgpu_bo_delete_mem_notify(struct ttm_buffer_object *bo) > > +{ > > + amdgpu_bo_move_notify(bo, false, NULL); > > +} > > + > > static struct ttm_bo_driver amdgpu_bo_driver = { > > .ttm_tt_create = &amdgpu_ttm_tt_create, > > .ttm_tt_populate = &amdgpu_ttm_tt_populate, > > @@ -1739,7 +1745,7 @@ static struct ttm_bo_driver amdgpu_bo_driver = { > > .evict_flags = &amdgpu_evict_flags, > > .move = &amdgpu_bo_move, > > .verify_access = &amdgpu_verify_access, > > - .move_notify = &amdgpu_bo_move_notify, > > + .delete_mem_notify = &amdgpu_bo_delete_mem_notify, > > .release_notify = &amdgpu_bo_release_notify, > > .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, > > .io_mem_pfn = amdgpu_ttm_io_mem_pfn, > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > > index 19087b22bdbb..9da823eb0edd 100644 > > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > > @@ -949,9 +949,7 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo, > > drm_gem_vram_bo_driver_evict_flags(gbo, placement); > > } > > -static void bo_driver_move_notify(struct ttm_buffer_object *bo, > > - bool evict, > > - struct ttm_resource *new_mem) > > +static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) > > { > > struct drm_gem_vram_object *gbo; > > @@ -961,7 +959,7 @@ static void bo_driver_move_notify(struct ttm_buffer_object *bo, > > gbo = drm_gem_vram_of_bo(bo); > > - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); > > + drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); > > } > > static int bo_driver_move(struct ttm_buffer_object *bo, > > @@ -1002,7 +1000,7 @@ static struct ttm_bo_driver bo_driver = { > > .eviction_valuable = ttm_bo_eviction_valuable, > > .evict_flags = bo_driver_evict_flags, > > .move = bo_driver_move, > > - .move_notify = bo_driver_move_notify, > > + .delete_mem_notify = bo_driver_delete_mem_notify, > > .io_mem_reserve = bo_driver_io_mem_reserve, > > }; > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > > index 70b6f3b1ae85..acff82afe260 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > > @@ -1401,6 +1401,12 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl > > dma_resv_add_shared_fence(resv, &fence->base); > > } > > +static void > > +nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) > > +{ > > + nouveau_bo_move_ntfy(bo, false, NULL); > > +} > > + > > struct ttm_bo_driver nouveau_bo_driver = { > > .ttm_tt_create = &nouveau_ttm_tt_create, > > .ttm_tt_populate = &nouveau_ttm_tt_populate, > > @@ -1408,7 +1414,7 @@ struct ttm_bo_driver nouveau_bo_driver = { > > .ttm_tt_destroy = &nouveau_ttm_tt_destroy, > > .eviction_valuable = ttm_bo_eviction_valuable, > > .evict_flags = nouveau_bo_evict_flags, > > - .move_notify = nouveau_bo_move_ntfy, > > + .delete_mem_notify = nouveau_bo_delete_mem_notify, > > .move = nouveau_bo_move, > > .verify_access = nouveau_bo_verify_access, > > .io_mem_reserve = &nouveau_ttm_io_mem_reserve, > > diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > > index 1cc3c14bc684..b52a4563b47b 100644 > > --- a/drivers/gpu/drm/qxl/qxl_ttm.c > > +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > > @@ -166,6 +166,11 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, > > return ret; > > } > > +static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) > > +{ > > + qxl_bo_move_notify(bo, false, NULL); > > +} > > + > > static struct ttm_bo_driver qxl_bo_driver = { > > .ttm_tt_create = &qxl_ttm_tt_create, > > .ttm_tt_destroy = &qxl_ttm_backend_destroy, > > @@ -173,7 +178,7 @@ static struct ttm_bo_driver qxl_bo_driver = { > > .evict_flags = &qxl_evict_flags, > > .move = &qxl_bo_move, > > .io_mem_reserve = &qxl_ttm_io_mem_reserve, > > - .move_notify = &qxl_bo_move_notify, > > + .delete_mem_notify = &qxl_bo_delete_mem_notify, > > }; > > static int qxl_ttm_init_mem_type(struct qxl_device *qdev, > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > > index cd454e5c802f..321c09d20c6c 100644 > > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > > @@ -824,6 +824,12 @@ bool radeon_ttm_tt_is_readonly(struct radeon_device *rdev, > > return !!(gtt->userflags & RADEON_GEM_USERPTR_READONLY); > > } > > +static void > > +radeon_bo_delete_mem_notify(struct ttm_buffer_object *bo) > > +{ > > + radeon_bo_move_notify(bo, false, NULL); > > +} > > + > > static struct ttm_bo_driver radeon_bo_driver = { > > .ttm_tt_create = &radeon_ttm_tt_create, > > .ttm_tt_populate = &radeon_ttm_tt_populate, > > @@ -833,7 +839,7 @@ static struct ttm_bo_driver radeon_bo_driver = { > > .evict_flags = &radeon_evict_flags, > > .move = &radeon_bo_move, > > .verify_access = &radeon_verify_access, > > - .move_notify = &radeon_bo_move_notify, > > + .delete_mem_notify = &radeon_bo_delete_mem_notify, > > .io_mem_reserve = &radeon_ttm_io_mem_reserve, > > }; > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > > index 2b578012cdef..e2afab3d97ee 100644 > > --- a/drivers/gpu/drm/ttm/ttm_bo.c > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > > @@ -284,8 +284,8 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > > static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > > { > > - if (bo->bdev->driver->move_notify) > > - bo->bdev->driver->move_notify(bo, false, NULL); > > + if (bo->bdev->driver->delete_mem_notify) > > + bo->bdev->driver->delete_mem_notify(bo); > > ttm_bo_tt_destroy(bo); > > ttm_resource_free(bo, &bo->mem); > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > > index de25cf016be2..88be48ad0344 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > > @@ -771,6 +771,12 @@ static int vmw_move(struct ttm_buffer_object *bo, > > return ret; > > } > > +static void > > +vmw_delete_mem_notify(struct ttm_buffer_object *bo) > > +{ > > + vmw_move_notify(bo, false, NULL); > > +} > > + > > struct ttm_bo_driver vmw_bo_driver = { > > .ttm_tt_create = &vmw_ttm_tt_create, > > .ttm_tt_populate = &vmw_ttm_populate, > > @@ -780,7 +786,7 @@ struct ttm_bo_driver vmw_bo_driver = { > > .evict_flags = vmw_evict_flags, > > .move = vmw_move, > > .verify_access = vmw_verify_access, > > - .move_notify = vmw_move_notify, > > + .delete_mem_notify = vmw_delete_mem_notify, > > .swap_notify = vmw_swap_notify, > > .io_mem_reserve = &vmw_ttm_io_mem_reserve, > > }; > > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > > index 72f106b335e9..29f6a1d1c853 100644 > > --- a/include/drm/ttm/ttm_bo_driver.h > > +++ b/include/drm/ttm/ttm_bo_driver.h > > @@ -156,15 +156,9 @@ struct ttm_bo_driver { > > struct file *filp); > > /** > > - * Hook to notify driver about a driver move so it > > - * can do tiling things and book-keeping. > > - * > > - * @evict: whether this move is evicting the buffer from the graphics > > - * address space > > + * Hook to notify driver about a resource delete. > > */ > > - void (*move_notify)(struct ttm_buffer_object *bo, > > - bool evict, > > - struct ttm_resource *new_mem); > > + void (*delete_mem_notify)(struct ttm_buffer_object *bo); > > /** > > * notify the driver that we're about to swap out this bo > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify 2020-10-28 9:16 ` Daniel Vetter @ 2020-10-28 9:53 ` Christian König 2020-10-28 10:19 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Christian König @ 2020-10-28 9:53 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel Am 28.10.20 um 10:16 schrieb Daniel Vetter: > On Wed, Oct 21, 2020 at 11:58:45AM +0200, Christian König wrote: >> Am 21.10.20 um 06:40 schrieb Dave Airlie: >>> From: Dave Airlie <airlied@redhat.com> >>> >>> The move notify callback is only used in one place, this should >>> be removed in the future, but for now just rename it to the use >>> case which is to notify the driver that the GPU memory is to be >>> deleted. >> Probably the right thing to do is to call the move callback with move(from, >> NULL) in this case as well. >> >> And then driver can call the necessary function to throw away the backing >> store pipelined. >> >>> Drivers can be cleaned up after this separately. >>> >>> Signed-off-by: Dave Airlie <airlied@redhat.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> > Not sure where to best ask this question, but while reading code I > stumbled over the bo->pin_count check in ttm_bo_release(). And I'm > confused. > > Allowing a bo to be pinned without holding a full reference to it feels > like a pretty serious bug. Where&why is that needed? I'm kinda tempted to > wrap this in a WARN_ON, just to make sure there's no surprises in usage > (and maybe warn in unpin if we drop the pin count with the refcount at 0 > already). Yeah, I was wondering about that as well. In general I don't see harm from the TTM perspective to drop the last reference while a BO is still pinned. Only from the driver side it sounds like a bug to me, so I decided to not going to enforce this in TTM. Christian. > -Daniel > >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++++++- >>> drivers/gpu/drm/drm_gem_vram_helper.c | 8 +++----- >>> drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +++++++- >>> drivers/gpu/drm/qxl/qxl_ttm.c | 7 ++++++- >>> drivers/gpu/drm/radeon/radeon_ttm.c | 8 +++++++- >>> drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- >>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 8 +++++++- >>> include/drm/ttm/ttm_bo_driver.h | 10 ++-------- >>> 8 files changed, 41 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> index 87e10a212b8a..62f9194b1dd1 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> @@ -1730,6 +1730,12 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, >>> return ret; >>> } >>> +static void >>> +amdgpu_bo_delete_mem_notify(struct ttm_buffer_object *bo) >>> +{ >>> + amdgpu_bo_move_notify(bo, false, NULL); >>> +} >>> + >>> static struct ttm_bo_driver amdgpu_bo_driver = { >>> .ttm_tt_create = &amdgpu_ttm_tt_create, >>> .ttm_tt_populate = &amdgpu_ttm_tt_populate, >>> @@ -1739,7 +1745,7 @@ static struct ttm_bo_driver amdgpu_bo_driver = { >>> .evict_flags = &amdgpu_evict_flags, >>> .move = &amdgpu_bo_move, >>> .verify_access = &amdgpu_verify_access, >>> - .move_notify = &amdgpu_bo_move_notify, >>> + .delete_mem_notify = &amdgpu_bo_delete_mem_notify, >>> .release_notify = &amdgpu_bo_release_notify, >>> .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, >>> .io_mem_pfn = amdgpu_ttm_io_mem_pfn, >>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c >>> index 19087b22bdbb..9da823eb0edd 100644 >>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >>> @@ -949,9 +949,7 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo, >>> drm_gem_vram_bo_driver_evict_flags(gbo, placement); >>> } >>> -static void bo_driver_move_notify(struct ttm_buffer_object *bo, >>> - bool evict, >>> - struct ttm_resource *new_mem) >>> +static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) >>> { >>> struct drm_gem_vram_object *gbo; >>> @@ -961,7 +959,7 @@ static void bo_driver_move_notify(struct ttm_buffer_object *bo, >>> gbo = drm_gem_vram_of_bo(bo); >>> - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); >>> + drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); >>> } >>> static int bo_driver_move(struct ttm_buffer_object *bo, >>> @@ -1002,7 +1000,7 @@ static struct ttm_bo_driver bo_driver = { >>> .eviction_valuable = ttm_bo_eviction_valuable, >>> .evict_flags = bo_driver_evict_flags, >>> .move = bo_driver_move, >>> - .move_notify = bo_driver_move_notify, >>> + .delete_mem_notify = bo_driver_delete_mem_notify, >>> .io_mem_reserve = bo_driver_io_mem_reserve, >>> }; >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c >>> index 70b6f3b1ae85..acff82afe260 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c >>> @@ -1401,6 +1401,12 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl >>> dma_resv_add_shared_fence(resv, &fence->base); >>> } >>> +static void >>> +nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) >>> +{ >>> + nouveau_bo_move_ntfy(bo, false, NULL); >>> +} >>> + >>> struct ttm_bo_driver nouveau_bo_driver = { >>> .ttm_tt_create = &nouveau_ttm_tt_create, >>> .ttm_tt_populate = &nouveau_ttm_tt_populate, >>> @@ -1408,7 +1414,7 @@ struct ttm_bo_driver nouveau_bo_driver = { >>> .ttm_tt_destroy = &nouveau_ttm_tt_destroy, >>> .eviction_valuable = ttm_bo_eviction_valuable, >>> .evict_flags = nouveau_bo_evict_flags, >>> - .move_notify = nouveau_bo_move_ntfy, >>> + .delete_mem_notify = nouveau_bo_delete_mem_notify, >>> .move = nouveau_bo_move, >>> .verify_access = nouveau_bo_verify_access, >>> .io_mem_reserve = &nouveau_ttm_io_mem_reserve, >>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c >>> index 1cc3c14bc684..b52a4563b47b 100644 >>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c >>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c >>> @@ -166,6 +166,11 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, >>> return ret; >>> } >>> +static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) >>> +{ >>> + qxl_bo_move_notify(bo, false, NULL); >>> +} >>> + >>> static struct ttm_bo_driver qxl_bo_driver = { >>> .ttm_tt_create = &qxl_ttm_tt_create, >>> .ttm_tt_destroy = &qxl_ttm_backend_destroy, >>> @@ -173,7 +178,7 @@ static struct ttm_bo_driver qxl_bo_driver = { >>> .evict_flags = &qxl_evict_flags, >>> .move = &qxl_bo_move, >>> .io_mem_reserve = &qxl_ttm_io_mem_reserve, >>> - .move_notify = &qxl_bo_move_notify, >>> + .delete_mem_notify = &qxl_bo_delete_mem_notify, >>> }; >>> static int qxl_ttm_init_mem_type(struct qxl_device *qdev, >>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c >>> index cd454e5c802f..321c09d20c6c 100644 >>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c >>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c >>> @@ -824,6 +824,12 @@ bool radeon_ttm_tt_is_readonly(struct radeon_device *rdev, >>> return !!(gtt->userflags & RADEON_GEM_USERPTR_READONLY); >>> } >>> +static void >>> +radeon_bo_delete_mem_notify(struct ttm_buffer_object *bo) >>> +{ >>> + radeon_bo_move_notify(bo, false, NULL); >>> +} >>> + >>> static struct ttm_bo_driver radeon_bo_driver = { >>> .ttm_tt_create = &radeon_ttm_tt_create, >>> .ttm_tt_populate = &radeon_ttm_tt_populate, >>> @@ -833,7 +839,7 @@ static struct ttm_bo_driver radeon_bo_driver = { >>> .evict_flags = &radeon_evict_flags, >>> .move = &radeon_bo_move, >>> .verify_access = &radeon_verify_access, >>> - .move_notify = &radeon_bo_move_notify, >>> + .delete_mem_notify = &radeon_bo_delete_mem_notify, >>> .io_mem_reserve = &radeon_ttm_io_mem_reserve, >>> }; >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >>> index 2b578012cdef..e2afab3d97ee 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_bo.c >>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c >>> @@ -284,8 +284,8 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, >>> static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) >>> { >>> - if (bo->bdev->driver->move_notify) >>> - bo->bdev->driver->move_notify(bo, false, NULL); >>> + if (bo->bdev->driver->delete_mem_notify) >>> + bo->bdev->driver->delete_mem_notify(bo); >>> ttm_bo_tt_destroy(bo); >>> ttm_resource_free(bo, &bo->mem); >>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >>> index de25cf016be2..88be48ad0344 100644 >>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >>> @@ -771,6 +771,12 @@ static int vmw_move(struct ttm_buffer_object *bo, >>> return ret; >>> } >>> +static void >>> +vmw_delete_mem_notify(struct ttm_buffer_object *bo) >>> +{ >>> + vmw_move_notify(bo, false, NULL); >>> +} >>> + >>> struct ttm_bo_driver vmw_bo_driver = { >>> .ttm_tt_create = &vmw_ttm_tt_create, >>> .ttm_tt_populate = &vmw_ttm_populate, >>> @@ -780,7 +786,7 @@ struct ttm_bo_driver vmw_bo_driver = { >>> .evict_flags = vmw_evict_flags, >>> .move = vmw_move, >>> .verify_access = vmw_verify_access, >>> - .move_notify = vmw_move_notify, >>> + .delete_mem_notify = vmw_delete_mem_notify, >>> .swap_notify = vmw_swap_notify, >>> .io_mem_reserve = &vmw_ttm_io_mem_reserve, >>> }; >>> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h >>> index 72f106b335e9..29f6a1d1c853 100644 >>> --- a/include/drm/ttm/ttm_bo_driver.h >>> +++ b/include/drm/ttm/ttm_bo_driver.h >>> @@ -156,15 +156,9 @@ struct ttm_bo_driver { >>> struct file *filp); >>> /** >>> - * Hook to notify driver about a driver move so it >>> - * can do tiling things and book-keeping. >>> - * >>> - * @evict: whether this move is evicting the buffer from the graphics >>> - * address space >>> + * Hook to notify driver about a resource delete. >>> */ >>> - void (*move_notify)(struct ttm_buffer_object *bo, >>> - bool evict, >>> - struct ttm_resource *new_mem); >>> + void (*delete_mem_notify)(struct ttm_buffer_object *bo); >>> /** >>> * notify the driver that we're about to swap out this bo >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce14a4829f0fd472cc9fd08d87b222578%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637394733878075272%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=R7%2FqQXVHnwsD%2Fj21FS%2FYXMsfFAA7%2BI84O54b6jP4bHs%3D&reserved=0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify 2020-10-28 9:53 ` Christian König @ 2020-10-28 10:19 ` Daniel Vetter 0 siblings, 0 replies; 13+ messages in thread From: Daniel Vetter @ 2020-10-28 10:19 UTC (permalink / raw) To: Christian König; +Cc: dri-devel On Wed, Oct 28, 2020 at 10:53 AM Christian König <christian.koenig@amd.com> wrote: > > Am 28.10.20 um 10:16 schrieb Daniel Vetter: > > On Wed, Oct 21, 2020 at 11:58:45AM +0200, Christian König wrote: > >> Am 21.10.20 um 06:40 schrieb Dave Airlie: > >>> From: Dave Airlie <airlied@redhat.com> > >>> > >>> The move notify callback is only used in one place, this should > >>> be removed in the future, but for now just rename it to the use > >>> case which is to notify the driver that the GPU memory is to be > >>> deleted. > >> Probably the right thing to do is to call the move callback with move(from, > >> NULL) in this case as well. > >> > >> And then driver can call the necessary function to throw away the backing > >> store pipelined. > >> > >>> Drivers can be cleaned up after this separately. > >>> > >>> Signed-off-by: Dave Airlie <airlied@redhat.com> > >> Reviewed-by: Christian König <christian.koenig@amd.com> > > Not sure where to best ask this question, but while reading code I > > stumbled over the bo->pin_count check in ttm_bo_release(). And I'm > > confused. > > > > Allowing a bo to be pinned without holding a full reference to it feels > > like a pretty serious bug. Where&why is that needed? I'm kinda tempted to > > wrap this in a WARN_ON, just to make sure there's no surprises in usage > > (and maybe warn in unpin if we drop the pin count with the refcount at 0 > > already). > > Yeah, I was wondering about that as well. > > In general I don't see harm from the TTM perspective to drop the last > reference while a BO is still pinned. > > Only from the driver side it sounds like a bug to me, so I decided to > not going to enforce this in TTM. Yeah implementing it doesn't sound tricky, but it also doesn't sound like something anyone would ever want to do intentionally. I think I'll stitch some WARN_ONs together. -Daniel > > Christian. > > > -Daniel > > > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++++++- > >>> drivers/gpu/drm/drm_gem_vram_helper.c | 8 +++----- > >>> drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +++++++- > >>> drivers/gpu/drm/qxl/qxl_ttm.c | 7 ++++++- > >>> drivers/gpu/drm/radeon/radeon_ttm.c | 8 +++++++- > >>> drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- > >>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 8 +++++++- > >>> include/drm/ttm/ttm_bo_driver.h | 10 ++-------- > >>> 8 files changed, 41 insertions(+), 20 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> index 87e10a212b8a..62f9194b1dd1 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> @@ -1730,6 +1730,12 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, > >>> return ret; > >>> } > >>> +static void > >>> +amdgpu_bo_delete_mem_notify(struct ttm_buffer_object *bo) > >>> +{ > >>> + amdgpu_bo_move_notify(bo, false, NULL); > >>> +} > >>> + > >>> static struct ttm_bo_driver amdgpu_bo_driver = { > >>> .ttm_tt_create = &amdgpu_ttm_tt_create, > >>> .ttm_tt_populate = &amdgpu_ttm_tt_populate, > >>> @@ -1739,7 +1745,7 @@ static struct ttm_bo_driver amdgpu_bo_driver = { > >>> .evict_flags = &amdgpu_evict_flags, > >>> .move = &amdgpu_bo_move, > >>> .verify_access = &amdgpu_verify_access, > >>> - .move_notify = &amdgpu_bo_move_notify, > >>> + .delete_mem_notify = &amdgpu_bo_delete_mem_notify, > >>> .release_notify = &amdgpu_bo_release_notify, > >>> .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, > >>> .io_mem_pfn = amdgpu_ttm_io_mem_pfn, > >>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > >>> index 19087b22bdbb..9da823eb0edd 100644 > >>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c > >>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > >>> @@ -949,9 +949,7 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo, > >>> drm_gem_vram_bo_driver_evict_flags(gbo, placement); > >>> } > >>> -static void bo_driver_move_notify(struct ttm_buffer_object *bo, > >>> - bool evict, > >>> - struct ttm_resource *new_mem) > >>> +static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) > >>> { > >>> struct drm_gem_vram_object *gbo; > >>> @@ -961,7 +959,7 @@ static void bo_driver_move_notify(struct ttm_buffer_object *bo, > >>> gbo = drm_gem_vram_of_bo(bo); > >>> - drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem); > >>> + drm_gem_vram_bo_driver_move_notify(gbo, false, NULL); > >>> } > >>> static int bo_driver_move(struct ttm_buffer_object *bo, > >>> @@ -1002,7 +1000,7 @@ static struct ttm_bo_driver bo_driver = { > >>> .eviction_valuable = ttm_bo_eviction_valuable, > >>> .evict_flags = bo_driver_evict_flags, > >>> .move = bo_driver_move, > >>> - .move_notify = bo_driver_move_notify, > >>> + .delete_mem_notify = bo_driver_delete_mem_notify, > >>> .io_mem_reserve = bo_driver_io_mem_reserve, > >>> }; > >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > >>> index 70b6f3b1ae85..acff82afe260 100644 > >>> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > >>> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > >>> @@ -1401,6 +1401,12 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl > >>> dma_resv_add_shared_fence(resv, &fence->base); > >>> } > >>> +static void > >>> +nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo) > >>> +{ > >>> + nouveau_bo_move_ntfy(bo, false, NULL); > >>> +} > >>> + > >>> struct ttm_bo_driver nouveau_bo_driver = { > >>> .ttm_tt_create = &nouveau_ttm_tt_create, > >>> .ttm_tt_populate = &nouveau_ttm_tt_populate, > >>> @@ -1408,7 +1414,7 @@ struct ttm_bo_driver nouveau_bo_driver = { > >>> .ttm_tt_destroy = &nouveau_ttm_tt_destroy, > >>> .eviction_valuable = ttm_bo_eviction_valuable, > >>> .evict_flags = nouveau_bo_evict_flags, > >>> - .move_notify = nouveau_bo_move_ntfy, > >>> + .delete_mem_notify = nouveau_bo_delete_mem_notify, > >>> .move = nouveau_bo_move, > >>> .verify_access = nouveau_bo_verify_access, > >>> .io_mem_reserve = &nouveau_ttm_io_mem_reserve, > >>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > >>> index 1cc3c14bc684..b52a4563b47b 100644 > >>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c > >>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > >>> @@ -166,6 +166,11 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, > >>> return ret; > >>> } > >>> +static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo) > >>> +{ > >>> + qxl_bo_move_notify(bo, false, NULL); > >>> +} > >>> + > >>> static struct ttm_bo_driver qxl_bo_driver = { > >>> .ttm_tt_create = &qxl_ttm_tt_create, > >>> .ttm_tt_destroy = &qxl_ttm_backend_destroy, > >>> @@ -173,7 +178,7 @@ static struct ttm_bo_driver qxl_bo_driver = { > >>> .evict_flags = &qxl_evict_flags, > >>> .move = &qxl_bo_move, > >>> .io_mem_reserve = &qxl_ttm_io_mem_reserve, > >>> - .move_notify = &qxl_bo_move_notify, > >>> + .delete_mem_notify = &qxl_bo_delete_mem_notify, > >>> }; > >>> static int qxl_ttm_init_mem_type(struct qxl_device *qdev, > >>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > >>> index cd454e5c802f..321c09d20c6c 100644 > >>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c > >>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > >>> @@ -824,6 +824,12 @@ bool radeon_ttm_tt_is_readonly(struct radeon_device *rdev, > >>> return !!(gtt->userflags & RADEON_GEM_USERPTR_READONLY); > >>> } > >>> +static void > >>> +radeon_bo_delete_mem_notify(struct ttm_buffer_object *bo) > >>> +{ > >>> + radeon_bo_move_notify(bo, false, NULL); > >>> +} > >>> + > >>> static struct ttm_bo_driver radeon_bo_driver = { > >>> .ttm_tt_create = &radeon_ttm_tt_create, > >>> .ttm_tt_populate = &radeon_ttm_tt_populate, > >>> @@ -833,7 +839,7 @@ static struct ttm_bo_driver radeon_bo_driver = { > >>> .evict_flags = &radeon_evict_flags, > >>> .move = &radeon_bo_move, > >>> .verify_access = &radeon_verify_access, > >>> - .move_notify = &radeon_bo_move_notify, > >>> + .delete_mem_notify = &radeon_bo_delete_mem_notify, > >>> .io_mem_reserve = &radeon_ttm_io_mem_reserve, > >>> }; > >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > >>> index 2b578012cdef..e2afab3d97ee 100644 > >>> --- a/drivers/gpu/drm/ttm/ttm_bo.c > >>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c > >>> @@ -284,8 +284,8 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > >>> static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > >>> { > >>> - if (bo->bdev->driver->move_notify) > >>> - bo->bdev->driver->move_notify(bo, false, NULL); > >>> + if (bo->bdev->driver->delete_mem_notify) > >>> + bo->bdev->driver->delete_mem_notify(bo); > >>> ttm_bo_tt_destroy(bo); > >>> ttm_resource_free(bo, &bo->mem); > >>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > >>> index de25cf016be2..88be48ad0344 100644 > >>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > >>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > >>> @@ -771,6 +771,12 @@ static int vmw_move(struct ttm_buffer_object *bo, > >>> return ret; > >>> } > >>> +static void > >>> +vmw_delete_mem_notify(struct ttm_buffer_object *bo) > >>> +{ > >>> + vmw_move_notify(bo, false, NULL); > >>> +} > >>> + > >>> struct ttm_bo_driver vmw_bo_driver = { > >>> .ttm_tt_create = &vmw_ttm_tt_create, > >>> .ttm_tt_populate = &vmw_ttm_populate, > >>> @@ -780,7 +786,7 @@ struct ttm_bo_driver vmw_bo_driver = { > >>> .evict_flags = vmw_evict_flags, > >>> .move = vmw_move, > >>> .verify_access = vmw_verify_access, > >>> - .move_notify = vmw_move_notify, > >>> + .delete_mem_notify = vmw_delete_mem_notify, > >>> .swap_notify = vmw_swap_notify, > >>> .io_mem_reserve = &vmw_ttm_io_mem_reserve, > >>> }; > >>> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > >>> index 72f106b335e9..29f6a1d1c853 100644 > >>> --- a/include/drm/ttm/ttm_bo_driver.h > >>> +++ b/include/drm/ttm/ttm_bo_driver.h > >>> @@ -156,15 +156,9 @@ struct ttm_bo_driver { > >>> struct file *filp); > >>> /** > >>> - * Hook to notify driver about a driver move so it > >>> - * can do tiling things and book-keeping. > >>> - * > >>> - * @evict: whether this move is evicting the buffer from the graphics > >>> - * address space > >>> + * Hook to notify driver about a resource delete. > >>> */ > >>> - void (*move_notify)(struct ttm_buffer_object *bo, > >>> - bool evict, > >>> - struct ttm_resource *new_mem); > >>> + void (*delete_mem_notify)(struct ttm_buffer_object *bo); > >>> /** > >>> * notify the driver that we're about to swap out this bo > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce14a4829f0fd472cc9fd08d87b222578%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637394733878075272%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=R7%2FqQXVHnwsD%2Fj21FS%2FYXMsfFAA7%2BI84O54b6jP4bHs%3D&reserved=0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter. 2020-10-21 4:40 [PATCH 0/3] ttm: change move notify + revised multi hop patch Dave Airlie 2020-10-21 4:40 ` [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify Dave Airlie @ 2020-10-21 4:40 ` Dave Airlie 2020-10-21 9:59 ` Christian König 2020-10-21 4:40 ` [PATCH 3/3] drm/ttm: avoid multihop moves in drivers Dave Airlie 2 siblings, 1 reply; 13+ messages in thread From: Dave Airlie @ 2020-10-21 4:40 UTC (permalink / raw) To: dri-devel; +Cc: christian.koenig From: Dave Airlie <airlied@redhat.com> Removed unused parameter. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index e2afab3d97ee..5b411252a857 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -830,7 +830,6 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, * @bo: BO to find memory for * @place: where to search * @mem: the memory object to fill in - * @ctx: operation context * * Check if placement is compatible and fill in mem structure. * Returns -EBUSY if placement won't work or negative error code. @@ -838,8 +837,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, */ static int ttm_bo_mem_placement(struct ttm_buffer_object *bo, const struct ttm_place *place, - struct ttm_resource *mem, - struct ttm_operation_ctx *ctx) + struct ttm_resource *mem) { struct ttm_bo_device *bdev = bo->bdev; struct ttm_resource_manager *man; @@ -884,7 +882,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, const struct ttm_place *place = &placement->placement[i]; struct ttm_resource_manager *man; - ret = ttm_bo_mem_placement(bo, place, mem, ctx); + ret = ttm_bo_mem_placement(bo, place, mem); if (ret) continue; @@ -910,7 +908,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, for (i = 0; i < placement->num_busy_placement; ++i) { const struct ttm_place *place = &placement->busy_placement[i]; - ret = ttm_bo_mem_placement(bo, place, mem, ctx); + ret = ttm_bo_mem_placement(bo, place, mem); if (ret) continue; -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter. 2020-10-21 4:40 ` [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter Dave Airlie @ 2020-10-21 9:59 ` Christian König 0 siblings, 0 replies; 13+ messages in thread From: Christian König @ 2020-10-21 9:59 UTC (permalink / raw) To: Dave Airlie, dri-devel Am 21.10.20 um 06:40 schrieb Dave Airlie: > From: Dave Airlie <airlied@redhat.com> > > Removed unused parameter. > > Signed-off-by: Dave Airlie <airlied@redhat.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index e2afab3d97ee..5b411252a857 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -830,7 +830,6 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, > * @bo: BO to find memory for > * @place: where to search > * @mem: the memory object to fill in > - * @ctx: operation context > * > * Check if placement is compatible and fill in mem structure. > * Returns -EBUSY if placement won't work or negative error code. > @@ -838,8 +837,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, > */ > static int ttm_bo_mem_placement(struct ttm_buffer_object *bo, > const struct ttm_place *place, > - struct ttm_resource *mem, > - struct ttm_operation_ctx *ctx) > + struct ttm_resource *mem) > { > struct ttm_bo_device *bdev = bo->bdev; > struct ttm_resource_manager *man; > @@ -884,7 +882,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > const struct ttm_place *place = &placement->placement[i]; > struct ttm_resource_manager *man; > > - ret = ttm_bo_mem_placement(bo, place, mem, ctx); > + ret = ttm_bo_mem_placement(bo, place, mem); > if (ret) > continue; > > @@ -910,7 +908,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > for (i = 0; i < placement->num_busy_placement; ++i) { > const struct ttm_place *place = &placement->busy_placement[i]; > > - ret = ttm_bo_mem_placement(bo, place, mem, ctx); > + ret = ttm_bo_mem_placement(bo, place, mem); > if (ret) > continue; > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] drm/ttm: avoid multihop moves in drivers. 2020-10-21 4:40 [PATCH 0/3] ttm: change move notify + revised multi hop patch Dave Airlie 2020-10-21 4:40 ` [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify Dave Airlie 2020-10-21 4:40 ` [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter Dave Airlie @ 2020-10-21 4:40 ` Dave Airlie 2020-10-21 8:33 ` Daniel Vetter 2020-10-28 9:39 ` Daniel Vetter 2 siblings, 2 replies; 13+ messages in thread From: Dave Airlie @ 2020-10-21 4:40 UTC (permalink / raw) To: dri-devel; +Cc: christian.koenig From: Dave Airlie <airlied@redhat.com> Currently drivers get called to move a buffer, but if they have to move it temporarily through another space (SYSTEM->VRAM via TT) then they can end up with a lot of ttm->driver->ttm call stacks, if the temprorary space moves requires eviction. Instead of letting the driver do all the placement/space for the temporary, allow it to report back (-EMULTIHOP) a placement (hop) to the move code, which will then do the temporary move, and the correct placement move afterwards. This removes a lot of code from drivers, at the expense of adding some midlayering. I've some further ideas on how to turn it inside out, but I think this is a good solution to the call stack problems. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 139 +++------------------ drivers/gpu/drm/drm_gem_vram_helper.c | 3 +- drivers/gpu/drm/nouveau/nouveau_bo.c | 115 +++-------------- drivers/gpu/drm/qxl/qxl_ttm.c | 3 +- drivers/gpu/drm/radeon/radeon_ttm.c | 122 +++--------------- drivers/gpu/drm/ttm/ttm_bo.c | 62 +++++++-- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 3 +- include/drm/ttm/ttm_bo_driver.h | 5 +- 8 files changed, 108 insertions(+), 344 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 62f9194b1dd1..0fd4f270d794 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -515,119 +515,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo, return r; } -/** - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer - * - * Called by amdgpu_bo_move(). - */ -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) -{ - struct ttm_resource *old_mem = &bo->mem; - struct ttm_resource tmp_mem; - struct ttm_place placements; - struct ttm_placement placement; - int r; - - /* create space/pages for new_mem in GTT space */ - tmp_mem = *new_mem; - tmp_mem.mm_node = NULL; - placement.num_placement = 1; - placement.placement = &placements; - placement.num_busy_placement = 1; - placement.busy_placement = &placements; - placements.fpfn = 0; - placements.lpfn = 0; - placements.mem_type = TTM_PL_TT; - placements.flags = 0; - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); - if (unlikely(r)) { - pr_err("Failed to find GTT space for blit from VRAM\n"); - return r; - } - - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (unlikely(r)) - goto out_cleanup; - - /* Bind the memory to the GTT space */ - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); - if (unlikely(r)) { - goto out_cleanup; - } - - /* blit VRAM to GTT */ - r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem); - if (unlikely(r)) { - goto out_cleanup; - } - - r = ttm_bo_wait_ctx(bo, ctx); - if (unlikely(r)) - goto out_cleanup; - - amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm); - ttm_resource_free(bo, &bo->mem); - ttm_bo_assign_mem(bo, new_mem); -out_cleanup: - ttm_resource_free(bo, &tmp_mem); - return r; -} - -/** - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM - * - * Called by amdgpu_bo_move(). - */ -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) -{ - struct ttm_resource *old_mem = &bo->mem; - struct ttm_resource tmp_mem; - struct ttm_placement placement; - struct ttm_place placements; - int r; - - /* make space in GTT for old_mem buffer */ - tmp_mem = *new_mem; - tmp_mem.mm_node = NULL; - placement.num_placement = 1; - placement.placement = &placements; - placement.num_busy_placement = 1; - placement.busy_placement = &placements; - placements.fpfn = 0; - placements.lpfn = 0; - placements.mem_type = TTM_PL_TT; - placements.flags = 0; - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); - if (unlikely(r)) { - pr_err("Failed to find GTT space for blit to VRAM\n"); - return r; - } - - /* move/bind old memory to GTT space */ - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (unlikely(r)) - return r; - - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); - if (unlikely(r)) { - goto out_cleanup; - } - - ttm_bo_assign_mem(bo, &tmp_mem); - /* copy to VRAM */ - r = amdgpu_move_blit(bo, evict, new_mem, old_mem); - if (unlikely(r)) { - goto out_cleanup; - } -out_cleanup: - ttm_resource_free(bo, &tmp_mem); - return r; -} - /** * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy * @@ -659,13 +546,25 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev, */ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) + struct ttm_resource *new_mem, + struct ttm_place *hop) { struct amdgpu_device *adev; struct amdgpu_bo *abo; struct ttm_resource *old_mem = &bo->mem; int r; + if ((old_mem->mem_type == TTM_PL_SYSTEM && + new_mem->mem_type == TTM_PL_VRAM) || + (old_mem->mem_type == TTM_PL_VRAM && + new_mem->mem_type == TTM_PL_SYSTEM)) { + hop->fpfn = 0; + hop->lpfn = 0; + hop->mem_type = TTM_PL_TT; + hop->flags = 0; + return -EMULTIHOP; + } + if (new_mem->mem_type == TTM_PL_TT) { r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem); if (r) @@ -719,16 +618,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, goto memcpy; } - if (old_mem->mem_type == TTM_PL_VRAM && - new_mem->mem_type == TTM_PL_SYSTEM) { - r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem); - } else if (old_mem->mem_type == TTM_PL_SYSTEM && - new_mem->mem_type == TTM_PL_VRAM) { - r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem); - } else { - r = amdgpu_move_blit(bo, evict, - new_mem, old_mem); - } + r = amdgpu_move_blit(bo, evict, + new_mem, old_mem); if (r) { memcpy: diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 9da823eb0edd..c51096cc7fb2 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -965,7 +965,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) static int bo_driver_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) + struct ttm_resource *new_mem, + struct ttm_place *hop) { struct drm_gem_vram_object *gbo; diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index acff82afe260..623577412d19 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -862,96 +862,6 @@ nouveau_bo_move_init(struct nouveau_drm *drm) NV_INFO(drm, "MM: using %s for buffer copies\n", name); } -static int -nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_reg) -{ - struct ttm_place placement_memtype = { - .fpfn = 0, - .lpfn = 0, - .mem_type = TTM_PL_TT, - .flags = 0 - }; - struct ttm_placement placement; - struct ttm_resource tmp_reg; - int ret; - - placement.num_placement = placement.num_busy_placement = 1; - placement.placement = placement.busy_placement = &placement_memtype; - - tmp_reg = *new_reg; - tmp_reg.mm_node = NULL; - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); - if (ret) - return ret; - - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (ret) - goto out; - - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); - if (ret) - goto out; - - ret = nouveau_bo_move_m2mf(bo, true, ctx, &tmp_reg); - if (ret) - goto out; - - ret = ttm_bo_wait_ctx(bo, ctx); - if (ret) - goto out; - - nouveau_ttm_tt_unbind(bo->bdev, bo->ttm); - ttm_resource_free(bo, &bo->mem); - ttm_bo_assign_mem(bo, &tmp_reg); -out: - ttm_resource_free(bo, &tmp_reg); - return ret; -} - -static int -nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_reg) -{ - struct ttm_place placement_memtype = { - .fpfn = 0, - .lpfn = 0, - .mem_type = TTM_PL_TT, - .flags = 0 - }; - struct ttm_placement placement; - struct ttm_resource tmp_reg; - int ret; - - placement.num_placement = placement.num_busy_placement = 1; - placement.placement = placement.busy_placement = &placement_memtype; - - tmp_reg = *new_reg; - tmp_reg.mm_node = NULL; - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); - if (ret) - return ret; - - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (unlikely(ret != 0)) - return ret; - - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); - if (unlikely(ret != 0)) - return ret; - - ttm_bo_assign_mem(bo, &tmp_reg); - ret = nouveau_bo_move_m2mf(bo, true, ctx, new_reg); - if (ret) - goto out; - -out: - ttm_resource_free(bo, &tmp_reg); - return ret; -} - static void nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *new_reg) @@ -1024,7 +934,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo, static int nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_reg) + struct ttm_resource *new_reg, + struct ttm_place *hop) { struct nouveau_drm *drm = nouveau_bdev(bo->bdev); struct nouveau_bo *nvbo = nouveau_bo(bo); @@ -1032,6 +943,17 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, struct nouveau_drm_tile *new_tile = NULL; int ret = 0; + if ((old_reg->mem_type == TTM_PL_SYSTEM && + new_reg->mem_type == TTM_PL_VRAM) || + (old_reg->mem_type == TTM_PL_VRAM && + new_reg->mem_type == TTM_PL_SYSTEM)) { + hop->fpfn = 0; + hop->lpfn = 0; + hop->mem_type = TTM_PL_TT; + hop->flags = 0; + return -EMULTIHOP; + } + if (new_reg->mem_type == TTM_PL_TT) { ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, new_reg); if (ret) @@ -1074,15 +996,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, /* Hardware assisted copy. */ if (drm->ttm.move) { - if (new_reg->mem_type == TTM_PL_SYSTEM) - ret = nouveau_bo_move_flipd(bo, evict, ctx, - new_reg); - else if (old_reg->mem_type == TTM_PL_SYSTEM) - ret = nouveau_bo_move_flips(bo, evict, ctx, - new_reg); - else - ret = nouveau_bo_move_m2mf(bo, evict, ctx, - new_reg); + ret = nouveau_bo_move_m2mf(bo, evict, ctx, + new_reg); if (!ret) goto out; } diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index b52a4563b47b..103e4f248f37 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -141,7 +141,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo, static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) + struct ttm_resource *new_mem, + struct ttm_place *hop) { struct ttm_resource *old_mem = &bo->mem; int ret; diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 321c09d20c6c..ddb04a2dc25f 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -207,110 +207,27 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, return r; } -static int radeon_move_vram_ram(struct ttm_buffer_object *bo, - bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) -{ - struct ttm_resource *old_mem = &bo->mem; - struct ttm_resource tmp_mem; - struct ttm_place placements; - struct ttm_placement placement; - int r; - - tmp_mem = *new_mem; - tmp_mem.mm_node = NULL; - placement.num_placement = 1; - placement.placement = &placements; - placement.num_busy_placement = 1; - placement.busy_placement = &placements; - placements.fpfn = 0; - placements.lpfn = 0; - placements.mem_type = TTM_PL_TT; - placements.flags = 0; - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); - if (unlikely(r)) { - return r; - } - - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (unlikely(r)) { - goto out_cleanup; - } - - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); - if (unlikely(r)) { - goto out_cleanup; - } - r = radeon_move_blit(bo, true, &tmp_mem, old_mem); - if (unlikely(r)) { - goto out_cleanup; - } - r = ttm_bo_wait_ctx(bo, ctx); - if (unlikely(r)) - goto out_cleanup; - - radeon_ttm_tt_unbind(bo->bdev, bo->ttm); - ttm_resource_free(bo, &bo->mem); - ttm_bo_assign_mem(bo, new_mem); -out_cleanup: - ttm_resource_free(bo, &tmp_mem); - return r; -} - -static int radeon_move_ram_vram(struct ttm_buffer_object *bo, - bool evict, - struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) -{ - struct ttm_resource *old_mem = &bo->mem; - struct ttm_resource tmp_mem; - struct ttm_placement placement; - struct ttm_place placements; - int r; - - tmp_mem = *new_mem; - tmp_mem.mm_node = NULL; - placement.num_placement = 1; - placement.placement = &placements; - placement.num_busy_placement = 1; - placement.busy_placement = &placements; - placements.fpfn = 0; - placements.lpfn = 0; - placements.mem_type = TTM_PL_TT; - placements.flags = 0; - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); - if (unlikely(r)) { - return r; - } - - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); - if (unlikely(r)) - goto out_cleanup; - - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); - if (unlikely(r)) - goto out_cleanup; - - ttm_bo_assign_mem(bo, &tmp_mem); - r = radeon_move_blit(bo, true, new_mem, old_mem); - if (unlikely(r)) { - goto out_cleanup; - } -out_cleanup: - ttm_resource_free(bo, &tmp_mem); - return r; -} - static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) + struct ttm_resource *new_mem, + struct ttm_place *hop) { struct radeon_device *rdev; struct radeon_bo *rbo; struct ttm_resource *old_mem = &bo->mem; int r; + if ((old_mem->mem_type == TTM_PL_SYSTEM && + new_mem->mem_type == TTM_PL_VRAM) || + (old_mem->mem_type == TTM_PL_VRAM && + new_mem->mem_type == TTM_PL_SYSTEM)) { + hop->fpfn = 0; + hop->lpfn = 0; + hop->mem_type = TTM_PL_TT; + hop->flags = 0; + return -EMULTIHOP; + } + if (new_mem->mem_type == TTM_PL_TT) { r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); if (r) @@ -351,17 +268,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, goto memcpy; } - if (old_mem->mem_type == TTM_PL_VRAM && - new_mem->mem_type == TTM_PL_SYSTEM) { - r = radeon_move_vram_ram(bo, evict, ctx, new_mem); - } else if (old_mem->mem_type == TTM_PL_SYSTEM && - new_mem->mem_type == TTM_PL_VRAM) { - r = radeon_move_ram_vram(bo, evict, ctx, new_mem); - } else { - r = radeon_move_blit(bo, evict, - new_mem, old_mem); - } - + r = radeon_move_blit(bo, evict, + new_mem, old_mem); if (r) { memcpy: r = ttm_bo_move_memcpy(bo, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 5b411252a857..a8830fdf8bc6 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail); static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, struct ttm_resource *mem, bool evict, - struct ttm_operation_ctx *ctx) + struct ttm_operation_ctx *ctx, + struct ttm_place *hop) { struct ttm_bo_device *bdev = bo->bdev; struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type); @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, } } - ret = bdev->driver->move(bo, evict, ctx, mem); - if (ret) + ret = bdev->driver->move(bo, evict, ctx, mem, hop); + if (ret) { + if (ret == -EMULTIHOP) + return ret; goto out_err; + } ctx->bytes_moved += bo->num_pages << PAGE_SHIFT; return 0; @@ -596,7 +600,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, goto out; } - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx); + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, NULL); if (unlikely(ret)) { if (ret != -ERESTARTSYS) pr_err("Buffer eviction failed\n"); @@ -936,11 +940,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, } EXPORT_SYMBOL(ttm_bo_mem_space); +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo, + struct ttm_resource *mem, + struct ttm_operation_ctx *ctx, + struct ttm_place *hop) +{ + struct ttm_placement hop_placement; + int ret; + struct ttm_resource hop_mem = *mem; + + hop_mem.mm_node = NULL; + hop_mem.mem_type = TTM_PL_SYSTEM; + hop_mem.placement = 0; + + hop_placement.num_placement = hop_placement.num_busy_placement = 1; + hop_placement.placement = hop_placement.busy_placement = hop; + + /* find space in the bounce domain */ + ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx); + if (ret) + return ret; + /* move to the bounce domain */ + ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL); + if (ret) + return ret; + return 0; +} + static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, struct ttm_placement *placement, struct ttm_operation_ctx *ctx) { int ret = 0; + struct ttm_place hop = {}; struct ttm_resource mem; dma_resv_assert_held(bo->base.resv); @@ -954,12 +986,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, /* * Determine where to move the buffer. + * + * If driver determines move is going to need + * an extra step then it will return -EMULTIHOP + * and the buffer will be moved to the temporary + * stop and the driver will be called to make + * the second hop. */ +bounce: ret = ttm_bo_mem_space(bo, placement, &mem, ctx); if (ret) - goto out_unlock; - ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx); -out_unlock: + return ret; + ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop); + if (ret == -EMULTIHOP) { + ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop); + if (ret) + return ret; + /* try and move to final place now. */ + goto bounce; + } if (ret) ttm_resource_free(bo, &mem); return ret; @@ -1435,7 +1480,7 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx) evict_mem.placement = 0; evict_mem.mem_type = TTM_PL_SYSTEM; - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx); + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, NULL); if (unlikely(ret != 0)) goto out; } @@ -1481,4 +1526,3 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo) ttm_tt_destroy(bo->bdev, bo->ttm); bo->ttm = NULL; } - diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index 88be48ad0344..d48b70605a56 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -731,7 +731,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo) static int vmw_move(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem) + struct ttm_resource *new_mem, + struct ttm_place *hop) { struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type); struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 29f6a1d1c853..e4eab7a45ace 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -133,12 +133,15 @@ struct ttm_bo_driver { * the graphics address space * @ctx: context for this move with parameters * @new_mem: the new memory region receiving the buffer + @ @hop: placement for driver directed intermediate hop * * Move a buffer between two memory regions. + * Returns errno -EMULTIHOP if driver requests a hop */ int (*move)(struct ttm_buffer_object *bo, bool evict, struct ttm_operation_ctx *ctx, - struct ttm_resource *new_mem); + struct ttm_resource *new_mem, + struct ttm_place *hop); /** * struct ttm_bo_driver_member verify_access -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/ttm: avoid multihop moves in drivers. 2020-10-21 4:40 ` [PATCH 3/3] drm/ttm: avoid multihop moves in drivers Dave Airlie @ 2020-10-21 8:33 ` Daniel Vetter 2020-10-21 10:58 ` Christian König 2020-10-28 9:39 ` Daniel Vetter 1 sibling, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2020-10-21 8:33 UTC (permalink / raw) To: Dave Airlie; +Cc: christian.koenig, dri-devel On Wed, Oct 21, 2020 at 02:40:31PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > Currently drivers get called to move a buffer, but if they have to > move it temporarily through another space (SYSTEM->VRAM via TT) > then they can end up with a lot of ttm->driver->ttm call stacks, > if the temprorary space moves requires eviction. > > Instead of letting the driver do all the placement/space for the > temporary, allow it to report back (-EMULTIHOP) a placement (hop) > to the move code, which will then do the temporary move, and the > correct placement move afterwards. > > This removes a lot of code from drivers, at the expense of > adding some midlayering. I've some further ideas on how to turn > it inside out, but I think this is a good solution to the call > stack problems. > > Signed-off-by: Dave Airlie <airlied@redhat.com> So at first I freaked out about this a bit on irc, as in "this is horrible midlayer madness". But it does look a lot cleaner, it's definitely a step in the right direction, and I guess for most drivers it's going to be good enough. Maybe there's going to be drivers which want to have even better control over where buffers are placed, who's victimized, and what intermediate steps to take. But doing that demidlayering can be done when there's a need, I think all the building blocks with the in-flight untangling are there now. So I guess I like this now after the initial shock passed :-) -Daniel > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 139 +++------------------ > drivers/gpu/drm/drm_gem_vram_helper.c | 3 +- > drivers/gpu/drm/nouveau/nouveau_bo.c | 115 +++-------------- > drivers/gpu/drm/qxl/qxl_ttm.c | 3 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 122 +++--------------- > drivers/gpu/drm/ttm/ttm_bo.c | 62 +++++++-- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 3 +- > include/drm/ttm/ttm_bo_driver.h | 5 +- > 8 files changed, 108 insertions(+), 344 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 62f9194b1dd1..0fd4f270d794 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -515,119 +515,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo, > return r; > } > > -/** > - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer > - * > - * Called by amdgpu_bo_move(). > - */ > -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_place placements; > - struct ttm_placement placement; > - int r; > - > - /* create space/pages for new_mem in GTT space */ > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - pr_err("Failed to find GTT space for blit from VRAM\n"); > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - /* Bind the memory to the GTT space */ > - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - /* blit VRAM to GTT */ > - r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - r = ttm_bo_wait_ctx(bo, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, new_mem); > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > -/** > - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM > - * > - * Called by amdgpu_bo_move(). > - */ > -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_placement placement; > - struct ttm_place placements; > - int r; > - > - /* make space in GTT for old_mem buffer */ > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - pr_err("Failed to find GTT space for blit to VRAM\n"); > - return r; > - } > - > - /* move/bind old memory to GTT space */ > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - return r; > - > - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - ttm_bo_assign_mem(bo, &tmp_mem); > - /* copy to VRAM */ > - r = amdgpu_move_blit(bo, evict, new_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > /** > * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy > * > @@ -659,13 +546,25 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev, > */ > static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct amdgpu_device *adev; > struct amdgpu_bo *abo; > struct ttm_resource *old_mem = &bo->mem; > int r; > > + if ((old_mem->mem_type == TTM_PL_SYSTEM && > + new_mem->mem_type == TTM_PL_VRAM) || > + (old_mem->mem_type == TTM_PL_VRAM && > + new_mem->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_mem->mem_type == TTM_PL_TT) { > r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem); > if (r) > @@ -719,16 +618,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, > goto memcpy; > } > > - if (old_mem->mem_type == TTM_PL_VRAM && > - new_mem->mem_type == TTM_PL_SYSTEM) { > - r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem); > - } else if (old_mem->mem_type == TTM_PL_SYSTEM && > - new_mem->mem_type == TTM_PL_VRAM) { > - r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem); > - } else { > - r = amdgpu_move_blit(bo, evict, > - new_mem, old_mem); > - } > + r = amdgpu_move_blit(bo, evict, > + new_mem, old_mem); > > if (r) { > memcpy: > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index 9da823eb0edd..c51096cc7fb2 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -965,7 +965,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) > static int bo_driver_move(struct ttm_buffer_object *bo, > bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct drm_gem_vram_object *gbo; > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index acff82afe260..623577412d19 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -862,96 +862,6 @@ nouveau_bo_move_init(struct nouveau_drm *drm) > NV_INFO(drm, "MM: using %s for buffer copies\n", name); > } > > -static int > -nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > -{ > - struct ttm_place placement_memtype = { > - .fpfn = 0, > - .lpfn = 0, > - .mem_type = TTM_PL_TT, > - .flags = 0 > - }; > - struct ttm_placement placement; > - struct ttm_resource tmp_reg; > - int ret; > - > - placement.num_placement = placement.num_busy_placement = 1; > - placement.placement = placement.busy_placement = &placement_memtype; > - > - tmp_reg = *new_reg; > - tmp_reg.mm_node = NULL; > - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); > - if (ret) > - return ret; > - > - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (ret) > - goto out; > - > - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); > - if (ret) > - goto out; > - > - ret = nouveau_bo_move_m2mf(bo, true, ctx, &tmp_reg); > - if (ret) > - goto out; > - > - ret = ttm_bo_wait_ctx(bo, ctx); > - if (ret) > - goto out; > - > - nouveau_ttm_tt_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, &tmp_reg); > -out: > - ttm_resource_free(bo, &tmp_reg); > - return ret; > -} > - > -static int > -nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > -{ > - struct ttm_place placement_memtype = { > - .fpfn = 0, > - .lpfn = 0, > - .mem_type = TTM_PL_TT, > - .flags = 0 > - }; > - struct ttm_placement placement; > - struct ttm_resource tmp_reg; > - int ret; > - > - placement.num_placement = placement.num_busy_placement = 1; > - placement.placement = placement.busy_placement = &placement_memtype; > - > - tmp_reg = *new_reg; > - tmp_reg.mm_node = NULL; > - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); > - if (ret) > - return ret; > - > - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(ret != 0)) > - return ret; > - > - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); > - if (unlikely(ret != 0)) > - return ret; > - > - ttm_bo_assign_mem(bo, &tmp_reg); > - ret = nouveau_bo_move_m2mf(bo, true, ctx, new_reg); > - if (ret) > - goto out; > - > -out: > - ttm_resource_free(bo, &tmp_reg); > - return ret; > -} > - > static void > nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, > struct ttm_resource *new_reg) > @@ -1024,7 +934,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo, > static int > nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > + struct ttm_resource *new_reg, > + struct ttm_place *hop) > { > struct nouveau_drm *drm = nouveau_bdev(bo->bdev); > struct nouveau_bo *nvbo = nouveau_bo(bo); > @@ -1032,6 +943,17 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > struct nouveau_drm_tile *new_tile = NULL; > int ret = 0; > > + if ((old_reg->mem_type == TTM_PL_SYSTEM && > + new_reg->mem_type == TTM_PL_VRAM) || > + (old_reg->mem_type == TTM_PL_VRAM && > + new_reg->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_reg->mem_type == TTM_PL_TT) { > ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, new_reg); > if (ret) > @@ -1074,15 +996,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > > /* Hardware assisted copy. */ > if (drm->ttm.move) { > - if (new_reg->mem_type == TTM_PL_SYSTEM) > - ret = nouveau_bo_move_flipd(bo, evict, ctx, > - new_reg); > - else if (old_reg->mem_type == TTM_PL_SYSTEM) > - ret = nouveau_bo_move_flips(bo, evict, ctx, > - new_reg); > - else > - ret = nouveau_bo_move_m2mf(bo, evict, ctx, > - new_reg); > + ret = nouveau_bo_move_m2mf(bo, evict, ctx, > + new_reg); > if (!ret) > goto out; > } > diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > index b52a4563b47b..103e4f248f37 100644 > --- a/drivers/gpu/drm/qxl/qxl_ttm.c > +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > @@ -141,7 +141,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo, > > static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct ttm_resource *old_mem = &bo->mem; > int ret; > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index 321c09d20c6c..ddb04a2dc25f 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -207,110 +207,27 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, > return r; > } > > -static int radeon_move_vram_ram(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_place placements; > - struct ttm_placement placement; > - int r; > - > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - r = radeon_move_blit(bo, true, &tmp_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - r = ttm_bo_wait_ctx(bo, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - radeon_ttm_tt_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, new_mem); > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > -static int radeon_move_ram_vram(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_placement placement; > - struct ttm_place placements; > - int r; > - > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) > - goto out_cleanup; > - > - ttm_bo_assign_mem(bo, &tmp_mem); > - r = radeon_move_blit(bo, true, new_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct radeon_device *rdev; > struct radeon_bo *rbo; > struct ttm_resource *old_mem = &bo->mem; > int r; > > + if ((old_mem->mem_type == TTM_PL_SYSTEM && > + new_mem->mem_type == TTM_PL_VRAM) || > + (old_mem->mem_type == TTM_PL_VRAM && > + new_mem->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_mem->mem_type == TTM_PL_TT) { > r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); > if (r) > @@ -351,17 +268,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, > goto memcpy; > } > > - if (old_mem->mem_type == TTM_PL_VRAM && > - new_mem->mem_type == TTM_PL_SYSTEM) { > - r = radeon_move_vram_ram(bo, evict, ctx, new_mem); > - } else if (old_mem->mem_type == TTM_PL_SYSTEM && > - new_mem->mem_type == TTM_PL_VRAM) { > - r = radeon_move_ram_vram(bo, evict, ctx, new_mem); > - } else { > - r = radeon_move_blit(bo, evict, > - new_mem, old_mem); > - } > - > + r = radeon_move_blit(bo, evict, > + new_mem, old_mem); > if (r) { > memcpy: > r = ttm_bo_move_memcpy(bo, ctx, new_mem); > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 5b411252a857..a8830fdf8bc6 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail); > > static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > struct ttm_resource *mem, bool evict, > - struct ttm_operation_ctx *ctx) > + struct ttm_operation_ctx *ctx, > + struct ttm_place *hop) > { > struct ttm_bo_device *bdev = bo->bdev; > struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type); > @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > } > } > > - ret = bdev->driver->move(bo, evict, ctx, mem); > - if (ret) > + ret = bdev->driver->move(bo, evict, ctx, mem, hop); > + if (ret) { > + if (ret == -EMULTIHOP) > + return ret; > goto out_err; > + } > > ctx->bytes_moved += bo->num_pages << PAGE_SHIFT; > return 0; > @@ -596,7 +600,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, > goto out; > } > > - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx); > + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, NULL); > if (unlikely(ret)) { > if (ret != -ERESTARTSYS) > pr_err("Buffer eviction failed\n"); > @@ -936,11 +940,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > } > EXPORT_SYMBOL(ttm_bo_mem_space); > > +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo, > + struct ttm_resource *mem, > + struct ttm_operation_ctx *ctx, > + struct ttm_place *hop) > +{ > + struct ttm_placement hop_placement; > + int ret; > + struct ttm_resource hop_mem = *mem; > + > + hop_mem.mm_node = NULL; > + hop_mem.mem_type = TTM_PL_SYSTEM; > + hop_mem.placement = 0; > + > + hop_placement.num_placement = hop_placement.num_busy_placement = 1; > + hop_placement.placement = hop_placement.busy_placement = hop; > + > + /* find space in the bounce domain */ > + ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx); > + if (ret) > + return ret; > + /* move to the bounce domain */ > + ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL); > + if (ret) > + return ret; > + return 0; > +} > + > static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, > struct ttm_placement *placement, > struct ttm_operation_ctx *ctx) > { > int ret = 0; > + struct ttm_place hop = {}; > struct ttm_resource mem; > > dma_resv_assert_held(bo->base.resv); > @@ -954,12 +986,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, > > /* > * Determine where to move the buffer. > + * > + * If driver determines move is going to need > + * an extra step then it will return -EMULTIHOP > + * and the buffer will be moved to the temporary > + * stop and the driver will be called to make > + * the second hop. > */ > +bounce: > ret = ttm_bo_mem_space(bo, placement, &mem, ctx); > if (ret) > - goto out_unlock; > - ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx); > -out_unlock: > + return ret; > + ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop); > + if (ret == -EMULTIHOP) { > + ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop); > + if (ret) > + return ret; > + /* try and move to final place now. */ > + goto bounce; > + } > if (ret) > ttm_resource_free(bo, &mem); > return ret; > @@ -1435,7 +1480,7 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx) > evict_mem.placement = 0; > evict_mem.mem_type = TTM_PL_SYSTEM; > > - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx); > + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, NULL); > if (unlikely(ret != 0)) > goto out; > } > @@ -1481,4 +1526,3 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo) > ttm_tt_destroy(bo->bdev, bo->ttm); > bo->ttm = NULL; > } > - > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > index 88be48ad0344..d48b70605a56 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > @@ -731,7 +731,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo) > static int vmw_move(struct ttm_buffer_object *bo, > bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type); > struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index 29f6a1d1c853..e4eab7a45ace 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -133,12 +133,15 @@ struct ttm_bo_driver { > * the graphics address space > * @ctx: context for this move with parameters > * @new_mem: the new memory region receiving the buffer > + @ @hop: placement for driver directed intermediate hop > * > * Move a buffer between two memory regions. > + * Returns errno -EMULTIHOP if driver requests a hop > */ > int (*move)(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem); > + struct ttm_resource *new_mem, > + struct ttm_place *hop); > > /** > * struct ttm_bo_driver_member verify_access > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/ttm: avoid multihop moves in drivers. 2020-10-21 8:33 ` Daniel Vetter @ 2020-10-21 10:58 ` Christian König 2020-10-28 2:02 ` Dave Airlie 0 siblings, 1 reply; 13+ messages in thread From: Christian König @ 2020-10-21 10:58 UTC (permalink / raw) To: Daniel Vetter, Dave Airlie; +Cc: dri-devel Am 21.10.20 um 10:33 schrieb Daniel Vetter: > On Wed, Oct 21, 2020 at 02:40:31PM +1000, Dave Airlie wrote: >> From: Dave Airlie <airlied@redhat.com> >> >> Currently drivers get called to move a buffer, but if they have to >> move it temporarily through another space (SYSTEM->VRAM via TT) >> then they can end up with a lot of ttm->driver->ttm call stacks, >> if the temprorary space moves requires eviction. >> >> Instead of letting the driver do all the placement/space for the >> temporary, allow it to report back (-EMULTIHOP) a placement (hop) >> to the move code, which will then do the temporary move, and the >> correct placement move afterwards. >> >> This removes a lot of code from drivers, at the expense of >> adding some midlayering. I've some further ideas on how to turn >> it inside out, but I think this is a good solution to the call >> stack problems. >> >> Signed-off-by: Dave Airlie <airlied@redhat.com> > So at first I freaked out about this a bit on irc, as in "this is horrible > midlayer madness". But it does look a lot cleaner, it's definitely a step > in the right direction, and I guess for most drivers it's going to be good > enough. Maybe there's going to be drivers which want to have even better > control over where buffers are placed, who's victimized, and what > intermediate steps to take. But doing that demidlayering can be done when > there's a need, I think all the building blocks with the in-flight > untangling are there now. > > So I guess I like this now after the initial shock passed :-) Essentially this is a really big de-midlayering :) See previously the call chain was like this: driver->ttm->driver->ttm->driver->ttm->driver.... For each multi hop we played ping/pong once between driver and ttm :) Now we at least only have driver->ttm->driver. Which is not ideal either, but still a lot better than before. Christian. > -Daniel > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 139 +++------------------ >> drivers/gpu/drm/drm_gem_vram_helper.c | 3 +- >> drivers/gpu/drm/nouveau/nouveau_bo.c | 115 +++-------------- >> drivers/gpu/drm/qxl/qxl_ttm.c | 3 +- >> drivers/gpu/drm/radeon/radeon_ttm.c | 122 +++--------------- >> drivers/gpu/drm/ttm/ttm_bo.c | 62 +++++++-- >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 3 +- >> include/drm/ttm/ttm_bo_driver.h | 5 +- >> 8 files changed, 108 insertions(+), 344 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 62f9194b1dd1..0fd4f270d794 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -515,119 +515,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo, >> return r; >> } >> >> -/** >> - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer >> - * >> - * Called by amdgpu_bo_move(). >> - */ >> -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> -{ >> - struct ttm_resource *old_mem = &bo->mem; >> - struct ttm_resource tmp_mem; >> - struct ttm_place placements; >> - struct ttm_placement placement; >> - int r; >> - >> - /* create space/pages for new_mem in GTT space */ >> - tmp_mem = *new_mem; >> - tmp_mem.mm_node = NULL; >> - placement.num_placement = 1; >> - placement.placement = &placements; >> - placement.num_busy_placement = 1; >> - placement.busy_placement = &placements; >> - placements.fpfn = 0; >> - placements.lpfn = 0; >> - placements.mem_type = TTM_PL_TT; >> - placements.flags = 0; >> - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); >> - if (unlikely(r)) { >> - pr_err("Failed to find GTT space for blit from VRAM\n"); >> - return r; >> - } >> - >> - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (unlikely(r)) >> - goto out_cleanup; >> - >> - /* Bind the memory to the GTT space */ >> - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - >> - /* blit VRAM to GTT */ >> - r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - >> - r = ttm_bo_wait_ctx(bo, ctx); >> - if (unlikely(r)) >> - goto out_cleanup; >> - >> - amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm); >> - ttm_resource_free(bo, &bo->mem); >> - ttm_bo_assign_mem(bo, new_mem); >> -out_cleanup: >> - ttm_resource_free(bo, &tmp_mem); >> - return r; >> -} >> - >> -/** >> - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM >> - * >> - * Called by amdgpu_bo_move(). >> - */ >> -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> -{ >> - struct ttm_resource *old_mem = &bo->mem; >> - struct ttm_resource tmp_mem; >> - struct ttm_placement placement; >> - struct ttm_place placements; >> - int r; >> - >> - /* make space in GTT for old_mem buffer */ >> - tmp_mem = *new_mem; >> - tmp_mem.mm_node = NULL; >> - placement.num_placement = 1; >> - placement.placement = &placements; >> - placement.num_busy_placement = 1; >> - placement.busy_placement = &placements; >> - placements.fpfn = 0; >> - placements.lpfn = 0; >> - placements.mem_type = TTM_PL_TT; >> - placements.flags = 0; >> - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); >> - if (unlikely(r)) { >> - pr_err("Failed to find GTT space for blit to VRAM\n"); >> - return r; >> - } >> - >> - /* move/bind old memory to GTT space */ >> - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (unlikely(r)) >> - return r; >> - >> - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - >> - ttm_bo_assign_mem(bo, &tmp_mem); >> - /* copy to VRAM */ >> - r = amdgpu_move_blit(bo, evict, new_mem, old_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> -out_cleanup: >> - ttm_resource_free(bo, &tmp_mem); >> - return r; >> -} >> - >> /** >> * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy >> * >> @@ -659,13 +546,25 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev, >> */ >> static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop) >> { >> struct amdgpu_device *adev; >> struct amdgpu_bo *abo; >> struct ttm_resource *old_mem = &bo->mem; >> int r; >> >> + if ((old_mem->mem_type == TTM_PL_SYSTEM && >> + new_mem->mem_type == TTM_PL_VRAM) || >> + (old_mem->mem_type == TTM_PL_VRAM && >> + new_mem->mem_type == TTM_PL_SYSTEM)) { >> + hop->fpfn = 0; >> + hop->lpfn = 0; >> + hop->mem_type = TTM_PL_TT; >> + hop->flags = 0; >> + return -EMULTIHOP; >> + } >> + >> if (new_mem->mem_type == TTM_PL_TT) { >> r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem); >> if (r) >> @@ -719,16 +618,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, >> goto memcpy; >> } >> >> - if (old_mem->mem_type == TTM_PL_VRAM && >> - new_mem->mem_type == TTM_PL_SYSTEM) { >> - r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem); >> - } else if (old_mem->mem_type == TTM_PL_SYSTEM && >> - new_mem->mem_type == TTM_PL_VRAM) { >> - r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem); >> - } else { >> - r = amdgpu_move_blit(bo, evict, >> - new_mem, old_mem); >> - } >> + r = amdgpu_move_blit(bo, evict, >> + new_mem, old_mem); >> >> if (r) { >> memcpy: >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c >> index 9da823eb0edd..c51096cc7fb2 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -965,7 +965,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) >> static int bo_driver_move(struct ttm_buffer_object *bo, >> bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop) >> { >> struct drm_gem_vram_object *gbo; >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c >> index acff82afe260..623577412d19 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c >> @@ -862,96 +862,6 @@ nouveau_bo_move_init(struct nouveau_drm *drm) >> NV_INFO(drm, "MM: using %s for buffer copies\n", name); >> } >> >> -static int >> -nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_reg) >> -{ >> - struct ttm_place placement_memtype = { >> - .fpfn = 0, >> - .lpfn = 0, >> - .mem_type = TTM_PL_TT, >> - .flags = 0 >> - }; >> - struct ttm_placement placement; >> - struct ttm_resource tmp_reg; >> - int ret; >> - >> - placement.num_placement = placement.num_busy_placement = 1; >> - placement.placement = placement.busy_placement = &placement_memtype; >> - >> - tmp_reg = *new_reg; >> - tmp_reg.mm_node = NULL; >> - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); >> - if (ret) >> - return ret; >> - >> - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (ret) >> - goto out; >> - >> - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); >> - if (ret) >> - goto out; >> - >> - ret = nouveau_bo_move_m2mf(bo, true, ctx, &tmp_reg); >> - if (ret) >> - goto out; >> - >> - ret = ttm_bo_wait_ctx(bo, ctx); >> - if (ret) >> - goto out; >> - >> - nouveau_ttm_tt_unbind(bo->bdev, bo->ttm); >> - ttm_resource_free(bo, &bo->mem); >> - ttm_bo_assign_mem(bo, &tmp_reg); >> -out: >> - ttm_resource_free(bo, &tmp_reg); >> - return ret; >> -} >> - >> -static int >> -nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_reg) >> -{ >> - struct ttm_place placement_memtype = { >> - .fpfn = 0, >> - .lpfn = 0, >> - .mem_type = TTM_PL_TT, >> - .flags = 0 >> - }; >> - struct ttm_placement placement; >> - struct ttm_resource tmp_reg; >> - int ret; >> - >> - placement.num_placement = placement.num_busy_placement = 1; >> - placement.placement = placement.busy_placement = &placement_memtype; >> - >> - tmp_reg = *new_reg; >> - tmp_reg.mm_node = NULL; >> - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); >> - if (ret) >> - return ret; >> - >> - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (unlikely(ret != 0)) >> - return ret; >> - >> - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); >> - if (unlikely(ret != 0)) >> - return ret; >> - >> - ttm_bo_assign_mem(bo, &tmp_reg); >> - ret = nouveau_bo_move_m2mf(bo, true, ctx, new_reg); >> - if (ret) >> - goto out; >> - >> -out: >> - ttm_resource_free(bo, &tmp_reg); >> - return ret; >> -} >> - >> static void >> nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, >> struct ttm_resource *new_reg) >> @@ -1024,7 +934,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo, >> static int >> nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_reg) >> + struct ttm_resource *new_reg, >> + struct ttm_place *hop) >> { >> struct nouveau_drm *drm = nouveau_bdev(bo->bdev); >> struct nouveau_bo *nvbo = nouveau_bo(bo); >> @@ -1032,6 +943,17 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, >> struct nouveau_drm_tile *new_tile = NULL; >> int ret = 0; >> >> + if ((old_reg->mem_type == TTM_PL_SYSTEM && >> + new_reg->mem_type == TTM_PL_VRAM) || >> + (old_reg->mem_type == TTM_PL_VRAM && >> + new_reg->mem_type == TTM_PL_SYSTEM)) { >> + hop->fpfn = 0; >> + hop->lpfn = 0; >> + hop->mem_type = TTM_PL_TT; >> + hop->flags = 0; >> + return -EMULTIHOP; >> + } >> + >> if (new_reg->mem_type == TTM_PL_TT) { >> ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, new_reg); >> if (ret) >> @@ -1074,15 +996,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, >> >> /* Hardware assisted copy. */ >> if (drm->ttm.move) { >> - if (new_reg->mem_type == TTM_PL_SYSTEM) >> - ret = nouveau_bo_move_flipd(bo, evict, ctx, >> - new_reg); >> - else if (old_reg->mem_type == TTM_PL_SYSTEM) >> - ret = nouveau_bo_move_flips(bo, evict, ctx, >> - new_reg); >> - else >> - ret = nouveau_bo_move_m2mf(bo, evict, ctx, >> - new_reg); >> + ret = nouveau_bo_move_m2mf(bo, evict, ctx, >> + new_reg); >> if (!ret) >> goto out; >> } >> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c >> index b52a4563b47b..103e4f248f37 100644 >> --- a/drivers/gpu/drm/qxl/qxl_ttm.c >> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c >> @@ -141,7 +141,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo, >> >> static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop) >> { >> struct ttm_resource *old_mem = &bo->mem; >> int ret; >> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c >> index 321c09d20c6c..ddb04a2dc25f 100644 >> --- a/drivers/gpu/drm/radeon/radeon_ttm.c >> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c >> @@ -207,110 +207,27 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, >> return r; >> } >> >> -static int radeon_move_vram_ram(struct ttm_buffer_object *bo, >> - bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> -{ >> - struct ttm_resource *old_mem = &bo->mem; >> - struct ttm_resource tmp_mem; >> - struct ttm_place placements; >> - struct ttm_placement placement; >> - int r; >> - >> - tmp_mem = *new_mem; >> - tmp_mem.mm_node = NULL; >> - placement.num_placement = 1; >> - placement.placement = &placements; >> - placement.num_busy_placement = 1; >> - placement.busy_placement = &placements; >> - placements.fpfn = 0; >> - placements.lpfn = 0; >> - placements.mem_type = TTM_PL_TT; >> - placements.flags = 0; >> - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); >> - if (unlikely(r)) { >> - return r; >> - } >> - >> - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - >> - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - r = radeon_move_blit(bo, true, &tmp_mem, old_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> - r = ttm_bo_wait_ctx(bo, ctx); >> - if (unlikely(r)) >> - goto out_cleanup; >> - >> - radeon_ttm_tt_unbind(bo->bdev, bo->ttm); >> - ttm_resource_free(bo, &bo->mem); >> - ttm_bo_assign_mem(bo, new_mem); >> -out_cleanup: >> - ttm_resource_free(bo, &tmp_mem); >> - return r; >> -} >> - >> -static int radeon_move_ram_vram(struct ttm_buffer_object *bo, >> - bool evict, >> - struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> -{ >> - struct ttm_resource *old_mem = &bo->mem; >> - struct ttm_resource tmp_mem; >> - struct ttm_placement placement; >> - struct ttm_place placements; >> - int r; >> - >> - tmp_mem = *new_mem; >> - tmp_mem.mm_node = NULL; >> - placement.num_placement = 1; >> - placement.placement = &placements; >> - placement.num_busy_placement = 1; >> - placement.busy_placement = &placements; >> - placements.fpfn = 0; >> - placements.lpfn = 0; >> - placements.mem_type = TTM_PL_TT; >> - placements.flags = 0; >> - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); >> - if (unlikely(r)) { >> - return r; >> - } >> - >> - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); >> - if (unlikely(r)) >> - goto out_cleanup; >> - >> - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); >> - if (unlikely(r)) >> - goto out_cleanup; >> - >> - ttm_bo_assign_mem(bo, &tmp_mem); >> - r = radeon_move_blit(bo, true, new_mem, old_mem); >> - if (unlikely(r)) { >> - goto out_cleanup; >> - } >> -out_cleanup: >> - ttm_resource_free(bo, &tmp_mem); >> - return r; >> -} >> - >> static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop) >> { >> struct radeon_device *rdev; >> struct radeon_bo *rbo; >> struct ttm_resource *old_mem = &bo->mem; >> int r; >> >> + if ((old_mem->mem_type == TTM_PL_SYSTEM && >> + new_mem->mem_type == TTM_PL_VRAM) || >> + (old_mem->mem_type == TTM_PL_VRAM && >> + new_mem->mem_type == TTM_PL_SYSTEM)) { >> + hop->fpfn = 0; >> + hop->lpfn = 0; >> + hop->mem_type = TTM_PL_TT; >> + hop->flags = 0; >> + return -EMULTIHOP; >> + } >> + >> if (new_mem->mem_type == TTM_PL_TT) { >> r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); >> if (r) >> @@ -351,17 +268,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, >> goto memcpy; >> } >> >> - if (old_mem->mem_type == TTM_PL_VRAM && >> - new_mem->mem_type == TTM_PL_SYSTEM) { >> - r = radeon_move_vram_ram(bo, evict, ctx, new_mem); >> - } else if (old_mem->mem_type == TTM_PL_SYSTEM && >> - new_mem->mem_type == TTM_PL_VRAM) { >> - r = radeon_move_ram_vram(bo, evict, ctx, new_mem); >> - } else { >> - r = radeon_move_blit(bo, evict, >> - new_mem, old_mem); >> - } >> - >> + r = radeon_move_blit(bo, evict, >> + new_mem, old_mem); >> if (r) { >> memcpy: >> r = ttm_bo_move_memcpy(bo, ctx, new_mem); >> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >> index 5b411252a857..a8830fdf8bc6 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo.c >> @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail); >> >> static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, >> struct ttm_resource *mem, bool evict, >> - struct ttm_operation_ctx *ctx) >> + struct ttm_operation_ctx *ctx, >> + struct ttm_place *hop) >> { >> struct ttm_bo_device *bdev = bo->bdev; >> struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type); >> @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, >> } >> } >> >> - ret = bdev->driver->move(bo, evict, ctx, mem); >> - if (ret) >> + ret = bdev->driver->move(bo, evict, ctx, mem, hop); >> + if (ret) { >> + if (ret == -EMULTIHOP) >> + return ret; >> goto out_err; >> + } >> >> ctx->bytes_moved += bo->num_pages << PAGE_SHIFT; >> return 0; >> @@ -596,7 +600,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, >> goto out; >> } >> >> - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx); >> + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, NULL); >> if (unlikely(ret)) { >> if (ret != -ERESTARTSYS) >> pr_err("Buffer eviction failed\n"); >> @@ -936,11 +940,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, >> } >> EXPORT_SYMBOL(ttm_bo_mem_space); >> >> +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo, >> + struct ttm_resource *mem, >> + struct ttm_operation_ctx *ctx, >> + struct ttm_place *hop) >> +{ >> + struct ttm_placement hop_placement; >> + int ret; >> + struct ttm_resource hop_mem = *mem; >> + >> + hop_mem.mm_node = NULL; >> + hop_mem.mem_type = TTM_PL_SYSTEM; >> + hop_mem.placement = 0; >> + >> + hop_placement.num_placement = hop_placement.num_busy_placement = 1; >> + hop_placement.placement = hop_placement.busy_placement = hop; >> + >> + /* find space in the bounce domain */ >> + ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx); >> + if (ret) >> + return ret; >> + /* move to the bounce domain */ >> + ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL); >> + if (ret) >> + return ret; >> + return 0; >> +} >> + >> static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, >> struct ttm_placement *placement, >> struct ttm_operation_ctx *ctx) >> { >> int ret = 0; >> + struct ttm_place hop = {}; >> struct ttm_resource mem; >> >> dma_resv_assert_held(bo->base.resv); >> @@ -954,12 +986,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, >> >> /* >> * Determine where to move the buffer. >> + * >> + * If driver determines move is going to need >> + * an extra step then it will return -EMULTIHOP >> + * and the buffer will be moved to the temporary >> + * stop and the driver will be called to make >> + * the second hop. >> */ >> +bounce: >> ret = ttm_bo_mem_space(bo, placement, &mem, ctx); >> if (ret) >> - goto out_unlock; >> - ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx); >> -out_unlock: >> + return ret; >> + ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop); >> + if (ret == -EMULTIHOP) { >> + ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop); >> + if (ret) >> + return ret; >> + /* try and move to final place now. */ >> + goto bounce; >> + } >> if (ret) >> ttm_resource_free(bo, &mem); >> return ret; >> @@ -1435,7 +1480,7 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx) >> evict_mem.placement = 0; >> evict_mem.mem_type = TTM_PL_SYSTEM; >> >> - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx); >> + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, NULL); >> if (unlikely(ret != 0)) >> goto out; >> } >> @@ -1481,4 +1526,3 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo) >> ttm_tt_destroy(bo->bdev, bo->ttm); >> bo->ttm = NULL; >> } >> - >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >> index 88be48ad0344..d48b70605a56 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c >> @@ -731,7 +731,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo) >> static int vmw_move(struct ttm_buffer_object *bo, >> bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem) >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop) >> { >> struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type); >> struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type); >> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h >> index 29f6a1d1c853..e4eab7a45ace 100644 >> --- a/include/drm/ttm/ttm_bo_driver.h >> +++ b/include/drm/ttm/ttm_bo_driver.h >> @@ -133,12 +133,15 @@ struct ttm_bo_driver { >> * the graphics address space >> * @ctx: context for this move with parameters >> * @new_mem: the new memory region receiving the buffer >> + @ @hop: placement for driver directed intermediate hop >> * >> * Move a buffer between two memory regions. >> + * Returns errno -EMULTIHOP if driver requests a hop >> */ >> int (*move)(struct ttm_buffer_object *bo, bool evict, >> struct ttm_operation_ctx *ctx, >> - struct ttm_resource *new_mem); >> + struct ttm_resource *new_mem, >> + struct ttm_place *hop); >> >> /** >> * struct ttm_bo_driver_member verify_access >> -- >> 2.27.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce4ee898f6eb94250ce3008d8759bfa2c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637388660089425780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QHhSO95cYUjBsm%2FP%2FOUNlaQK56NLp0zHUw2QuAoZ0tY%3D&reserved=0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/ttm: avoid multihop moves in drivers. 2020-10-21 10:58 ` Christian König @ 2020-10-28 2:02 ` Dave Airlie 0 siblings, 0 replies; 13+ messages in thread From: Dave Airlie @ 2020-10-28 2:02 UTC (permalink / raw) To: Christian König; +Cc: dri-devel Since I know both of you have looked at this, can someone give me an r-b? Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/ttm: avoid multihop moves in drivers. 2020-10-21 4:40 ` [PATCH 3/3] drm/ttm: avoid multihop moves in drivers Dave Airlie 2020-10-21 8:33 ` Daniel Vetter @ 2020-10-28 9:39 ` Daniel Vetter 1 sibling, 0 replies; 13+ messages in thread From: Daniel Vetter @ 2020-10-28 9:39 UTC (permalink / raw) To: Dave Airlie; +Cc: christian.koenig, dri-devel On Wed, Oct 21, 2020 at 02:40:31PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > Currently drivers get called to move a buffer, but if they have to > move it temporarily through another space (SYSTEM->VRAM via TT) > then they can end up with a lot of ttm->driver->ttm call stacks, > if the temprorary space moves requires eviction. > > Instead of letting the driver do all the placement/space for the > temporary, allow it to report back (-EMULTIHOP) a placement (hop) > to the move code, which will then do the temporary move, and the > correct placement move afterwards. > > This removes a lot of code from drivers, at the expense of > adding some midlayering. I've some further ideas on how to turn > it inside out, but I think this is a good solution to the call > stack problems. > > Signed-off-by: Dave Airlie <airlied@redhat.com> I think it'd be good to split out the ttm infrastructure work from the driver conversions. I'm not seeing any dependencies that require the flag day approach here, and might help if this blows up somehow. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 139 +++------------------ > drivers/gpu/drm/drm_gem_vram_helper.c | 3 +- > drivers/gpu/drm/nouveau/nouveau_bo.c | 115 +++-------------- > drivers/gpu/drm/qxl/qxl_ttm.c | 3 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 122 +++--------------- > drivers/gpu/drm/ttm/ttm_bo.c | 62 +++++++-- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 3 +- > include/drm/ttm/ttm_bo_driver.h | 5 +- > 8 files changed, 108 insertions(+), 344 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 62f9194b1dd1..0fd4f270d794 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -515,119 +515,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo, > return r; > } > > -/** > - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer > - * > - * Called by amdgpu_bo_move(). > - */ > -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_place placements; > - struct ttm_placement placement; > - int r; > - > - /* create space/pages for new_mem in GTT space */ > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - pr_err("Failed to find GTT space for blit from VRAM\n"); > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - /* Bind the memory to the GTT space */ > - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - /* blit VRAM to GTT */ > - r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - r = ttm_bo_wait_ctx(bo, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, new_mem); > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > -/** > - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM > - * > - * Called by amdgpu_bo_move(). > - */ > -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_placement placement; > - struct ttm_place placements; > - int r; > - > - /* make space in GTT for old_mem buffer */ > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - pr_err("Failed to find GTT space for blit to VRAM\n"); > - return r; > - } > - > - /* move/bind old memory to GTT space */ > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - return r; > - > - r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - ttm_bo_assign_mem(bo, &tmp_mem); > - /* copy to VRAM */ > - r = amdgpu_move_blit(bo, evict, new_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > /** > * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy > * > @@ -659,13 +546,25 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev, > */ > static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct amdgpu_device *adev; > struct amdgpu_bo *abo; > struct ttm_resource *old_mem = &bo->mem; > int r; > > + if ((old_mem->mem_type == TTM_PL_SYSTEM && > + new_mem->mem_type == TTM_PL_VRAM) || > + (old_mem->mem_type == TTM_PL_VRAM && > + new_mem->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_mem->mem_type == TTM_PL_TT) { > r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem); > if (r) > @@ -719,16 +618,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict, > goto memcpy; > } > > - if (old_mem->mem_type == TTM_PL_VRAM && > - new_mem->mem_type == TTM_PL_SYSTEM) { > - r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem); > - } else if (old_mem->mem_type == TTM_PL_SYSTEM && > - new_mem->mem_type == TTM_PL_VRAM) { > - r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem); > - } else { > - r = amdgpu_move_blit(bo, evict, > - new_mem, old_mem); > - } > + r = amdgpu_move_blit(bo, evict, > + new_mem, old_mem); > > if (r) { > memcpy: > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index 9da823eb0edd..c51096cc7fb2 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -965,7 +965,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo) > static int bo_driver_move(struct ttm_buffer_object *bo, > bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct drm_gem_vram_object *gbo; > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index acff82afe260..623577412d19 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -862,96 +862,6 @@ nouveau_bo_move_init(struct nouveau_drm *drm) > NV_INFO(drm, "MM: using %s for buffer copies\n", name); > } > > -static int > -nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > -{ > - struct ttm_place placement_memtype = { > - .fpfn = 0, > - .lpfn = 0, > - .mem_type = TTM_PL_TT, > - .flags = 0 > - }; > - struct ttm_placement placement; > - struct ttm_resource tmp_reg; > - int ret; > - > - placement.num_placement = placement.num_busy_placement = 1; > - placement.placement = placement.busy_placement = &placement_memtype; > - > - tmp_reg = *new_reg; > - tmp_reg.mm_node = NULL; > - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); > - if (ret) > - return ret; > - > - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (ret) > - goto out; > - > - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); > - if (ret) > - goto out; > - > - ret = nouveau_bo_move_m2mf(bo, true, ctx, &tmp_reg); > - if (ret) > - goto out; > - > - ret = ttm_bo_wait_ctx(bo, ctx); > - if (ret) > - goto out; > - > - nouveau_ttm_tt_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, &tmp_reg); > -out: > - ttm_resource_free(bo, &tmp_reg); > - return ret; > -} > - > -static int > -nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > -{ > - struct ttm_place placement_memtype = { > - .fpfn = 0, > - .lpfn = 0, > - .mem_type = TTM_PL_TT, > - .flags = 0 > - }; > - struct ttm_placement placement; > - struct ttm_resource tmp_reg; > - int ret; > - > - placement.num_placement = placement.num_busy_placement = 1; > - placement.placement = placement.busy_placement = &placement_memtype; > - > - tmp_reg = *new_reg; > - tmp_reg.mm_node = NULL; > - ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx); > - if (ret) > - return ret; > - > - ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(ret != 0)) > - return ret; > - > - ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg); > - if (unlikely(ret != 0)) > - return ret; > - > - ttm_bo_assign_mem(bo, &tmp_reg); > - ret = nouveau_bo_move_m2mf(bo, true, ctx, new_reg); > - if (ret) > - goto out; > - > -out: > - ttm_resource_free(bo, &tmp_reg); > - return ret; > -} > - > static void > nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, > struct ttm_resource *new_reg) > @@ -1024,7 +934,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo, > static int > nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_reg) > + struct ttm_resource *new_reg, > + struct ttm_place *hop) > { > struct nouveau_drm *drm = nouveau_bdev(bo->bdev); > struct nouveau_bo *nvbo = nouveau_bo(bo); > @@ -1032,6 +943,17 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > struct nouveau_drm_tile *new_tile = NULL; > int ret = 0; > > + if ((old_reg->mem_type == TTM_PL_SYSTEM && > + new_reg->mem_type == TTM_PL_VRAM) || > + (old_reg->mem_type == TTM_PL_VRAM && > + new_reg->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_reg->mem_type == TTM_PL_TT) { > ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, new_reg); > if (ret) > @@ -1074,15 +996,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, > > /* Hardware assisted copy. */ > if (drm->ttm.move) { > - if (new_reg->mem_type == TTM_PL_SYSTEM) > - ret = nouveau_bo_move_flipd(bo, evict, ctx, > - new_reg); > - else if (old_reg->mem_type == TTM_PL_SYSTEM) > - ret = nouveau_bo_move_flips(bo, evict, ctx, > - new_reg); > - else > - ret = nouveau_bo_move_m2mf(bo, evict, ctx, > - new_reg); > + ret = nouveau_bo_move_m2mf(bo, evict, ctx, > + new_reg); > if (!ret) > goto out; > } > diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > index b52a4563b47b..103e4f248f37 100644 > --- a/drivers/gpu/drm/qxl/qxl_ttm.c > +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > @@ -141,7 +141,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo, > > static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct ttm_resource *old_mem = &bo->mem; > int ret; > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index 321c09d20c6c..ddb04a2dc25f 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -207,110 +207,27 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, > return r; > } > > -static int radeon_move_vram_ram(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_place placements; > - struct ttm_placement placement; > - int r; > - > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - > - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - r = radeon_move_blit(bo, true, &tmp_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > - r = ttm_bo_wait_ctx(bo, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - radeon_ttm_tt_unbind(bo->bdev, bo->ttm); > - ttm_resource_free(bo, &bo->mem); > - ttm_bo_assign_mem(bo, new_mem); > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > -static int radeon_move_ram_vram(struct ttm_buffer_object *bo, > - bool evict, > - struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > -{ > - struct ttm_resource *old_mem = &bo->mem; > - struct ttm_resource tmp_mem; > - struct ttm_placement placement; > - struct ttm_place placements; > - int r; > - > - tmp_mem = *new_mem; > - tmp_mem.mm_node = NULL; > - placement.num_placement = 1; > - placement.placement = &placements; > - placement.num_busy_placement = 1; > - placement.busy_placement = &placements; > - placements.fpfn = 0; > - placements.lpfn = 0; > - placements.mem_type = TTM_PL_TT; > - placements.flags = 0; > - r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx); > - if (unlikely(r)) { > - return r; > - } > - > - r = ttm_tt_populate(bo->bdev, bo->ttm, ctx); > - if (unlikely(r)) > - goto out_cleanup; > - > - r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem); > - if (unlikely(r)) > - goto out_cleanup; > - > - ttm_bo_assign_mem(bo, &tmp_mem); > - r = radeon_move_blit(bo, true, new_mem, old_mem); > - if (unlikely(r)) { > - goto out_cleanup; > - } > -out_cleanup: > - ttm_resource_free(bo, &tmp_mem); > - return r; > -} > - > static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct radeon_device *rdev; > struct radeon_bo *rbo; > struct ttm_resource *old_mem = &bo->mem; > int r; > > + if ((old_mem->mem_type == TTM_PL_SYSTEM && > + new_mem->mem_type == TTM_PL_VRAM) || > + (old_mem->mem_type == TTM_PL_VRAM && > + new_mem->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > if (new_mem->mem_type == TTM_PL_TT) { > r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); > if (r) > @@ -351,17 +268,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, > goto memcpy; > } > > - if (old_mem->mem_type == TTM_PL_VRAM && > - new_mem->mem_type == TTM_PL_SYSTEM) { > - r = radeon_move_vram_ram(bo, evict, ctx, new_mem); > - } else if (old_mem->mem_type == TTM_PL_SYSTEM && > - new_mem->mem_type == TTM_PL_VRAM) { > - r = radeon_move_ram_vram(bo, evict, ctx, new_mem); > - } else { > - r = radeon_move_blit(bo, evict, > - new_mem, old_mem); > - } > - > + r = radeon_move_blit(bo, evict, > + new_mem, old_mem); > if (r) { > memcpy: > r = ttm_bo_move_memcpy(bo, ctx, new_mem); > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 5b411252a857..a8830fdf8bc6 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail); > > static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > struct ttm_resource *mem, bool evict, > - struct ttm_operation_ctx *ctx) > + struct ttm_operation_ctx *ctx, > + struct ttm_place *hop) > { > struct ttm_bo_device *bdev = bo->bdev; > struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type); > @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > } > } > > - ret = bdev->driver->move(bo, evict, ctx, mem); > - if (ret) > + ret = bdev->driver->move(bo, evict, ctx, mem, hop); > + if (ret) { > + if (ret == -EMULTIHOP) > + return ret; > goto out_err; > + } > > ctx->bytes_moved += bo->num_pages << PAGE_SHIFT; > return 0; > @@ -596,7 +600,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, > goto out; > } > > - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx); > + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, NULL); I think passing NULL here could lead to surprising oopses when drivers get some inconsistency between their evict_flags callback and what ->move does. I think it'd be better to pass in a dummy structure here, and WARN_ON if you get EMULTIHOP, dumping both what we got from evict_flags and here. Also I think an updated comment for both evict_flags and ->move callbacks in the kerneldoc highlighting this connection sounds like a good idea. I'm also wondering whether imposing this limitation is a good idea, since fancy multi-node gpus might want/need to evict in mutliple hops too. But I guess that can be readded later on. > if (unlikely(ret)) { > if (ret != -ERESTARTSYS) > pr_err("Buffer eviction failed\n"); > @@ -936,11 +940,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > } > EXPORT_SYMBOL(ttm_bo_mem_space); > > +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo, > + struct ttm_resource *mem, > + struct ttm_operation_ctx *ctx, > + struct ttm_place *hop) > +{ > + struct ttm_placement hop_placement; > + int ret; > + struct ttm_resource hop_mem = *mem; > + > + hop_mem.mm_node = NULL; > + hop_mem.mem_type = TTM_PL_SYSTEM; > + hop_mem.placement = 0; > + > + hop_placement.num_placement = hop_placement.num_busy_placement = 1; > + hop_placement.placement = hop_placement.busy_placement = hop; > + > + /* find space in the bounce domain */ > + ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx); > + if (ret) > + return ret; > + /* move to the bounce domain */ > + ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL); Again I'd pass a dummy here and complain loudly if we get an EMULTIHOP. > + if (ret) > + return ret; > + return 0; > +} > + > static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, > struct ttm_placement *placement, > struct ttm_operation_ctx *ctx) > { > int ret = 0; > + struct ttm_place hop = {}; > struct ttm_resource mem; > > dma_resv_assert_held(bo->base.resv); > @@ -954,12 +986,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, > > /* > * Determine where to move the buffer. > + * > + * If driver determines move is going to need > + * an extra step then it will return -EMULTIHOP > + * and the buffer will be moved to the temporary > + * stop and the driver will be called to make > + * the second hop. > */ > +bounce: > ret = ttm_bo_mem_space(bo, placement, &mem, ctx); > if (ret) > - goto out_unlock; > - ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx); > -out_unlock: > + return ret; > + ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop); > + if (ret == -EMULTIHOP) { > + ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop); Now that this is all lifted I wonder whether ttm could keep track of the stack of eviction locations. And complain loudly when that loops, since that's a driver bug :-) But for that to be effective I think we'd need to disallow any bo allocations/reservations from the ->move callback, and I'm not sure how to do that really. I think the only way would be a PF_ flag to notice ttm recursions, but might be good to add this to make really sure this ping-pong is dead for good. Or maybe we can use a fake lockdep lock that we take, which would also complain about recursion. But might cause lockdep false positives ... The other really nice thing this allows is that now it's feasible to wire through the ww_acquire_ctx in the ttm eviction code. That would allow us to resolve the TODO in ttm_mem_evict_wait_busy(). > + if (ret) > + return ret; > + /* try and move to final place now. */ > + goto bounce; > + } > if (ret) > ttm_resource_free(bo, &mem); > return ret; > @@ -1435,7 +1480,7 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx) > evict_mem.placement = 0; > evict_mem.mem_type = TTM_PL_SYSTEM; > > - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx); > + ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, NULL); > if (unlikely(ret != 0)) > goto out; > } > @@ -1481,4 +1526,3 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo) > ttm_tt_destroy(bo->bdev, bo->ttm); > bo->ttm = NULL; > } > - > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > index 88be48ad0344..d48b70605a56 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > @@ -731,7 +731,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo) > static int vmw_move(struct ttm_buffer_object *bo, > bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem) > + struct ttm_resource *new_mem, > + struct ttm_place *hop) > { > struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type); > struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index 29f6a1d1c853..e4eab7a45ace 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -133,12 +133,15 @@ struct ttm_bo_driver { > * the graphics address space > * @ctx: context for this move with parameters > * @new_mem: the new memory region receiving the buffer > + @ @hop: placement for driver directed intermediate hop > * > * Move a buffer between two memory regions. > + * Returns errno -EMULTIHOP if driver requests a hop > */ > int (*move)(struct ttm_buffer_object *bo, bool evict, > struct ttm_operation_ctx *ctx, > - struct ttm_resource *new_mem); > + struct ttm_resource *new_mem, > + struct ttm_place *hop); > > /** > * struct ttm_bo_driver_member verify_access Aside from the nit to never pass a NULL @hop for some driver safety and sanity checking I think ttm part looks all good. I didn't yet look at the driver conversions in detail to make sure they match the current code. Cheers, Daniel > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-10-28 10:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-21 4:40 [PATCH 0/3] ttm: change move notify + revised multi hop patch Dave Airlie 2020-10-21 4:40 ` [PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify Dave Airlie 2020-10-21 9:58 ` Christian König 2020-10-28 9:16 ` Daniel Vetter 2020-10-28 9:53 ` Christian König 2020-10-28 10:19 ` Daniel Vetter 2020-10-21 4:40 ` [PATCH 2/3] drm/ttm: ttm_bo_mem_placement doesn't need ctx parameter Dave Airlie 2020-10-21 9:59 ` Christian König 2020-10-21 4:40 ` [PATCH 3/3] drm/ttm: avoid multihop moves in drivers Dave Airlie 2020-10-21 8:33 ` Daniel Vetter 2020-10-21 10:58 ` Christian König 2020-10-28 2:02 ` Dave Airlie 2020-10-28 9:39 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).