[PATCH 1/3] drm/ttm: replace last move_notify with delete_mem_notify
Christian König
christian.koenig at amd.com
Wed Oct 28 09:53:30 UTC 2020
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 at 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 at redhat.com>
>> Reviewed-by: Christian König <christian.koenig at 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 at 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
More information about the dri-devel
mailing list