On Wed, Oct 28, 2020 at 10:53 AM Christian König
<christian.koenig@xxxxxxx> 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@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >> Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
