Re: [PATCH 4/5] drm/ttm: initialize globals during device init
On 10/22/2018 08:35 PM, Christian König wrote:
Am 22.10.18 um 08:45 schrieb Zhang, Jerry(Junwei):
A question in ttm_bo.c
[SNIP]
int ttm_bo_device_release(struct ttm_bo_device *bdev)
{
@@ -1623,18 +1620,25 @@ int ttm_bo_device_release(struct
ttm_bo_device *bdev)
drm_vma_offset_manager_destroy(&bdev->vma_manager);
+ if (!ret)
+ ttm_bo_global_release();
if ttm_bo_clean_mm() fails, it will skip ttm_bo_global_release().
When will it be called?
Never.
Shall add it to delayed work? or maybe we could release it directly?
No, when ttm_bo_device_release() fails somebody is trying to unload a
driver while this driver still has memory allocated.
In this case BO accounting should not be released because we should
make sure that all the leaked memory is still accounted.
In this case, it's rather a bug to fix then.
Thanks to explain it .
looks fine for me, feel free to add
Reviewed-by: Junwei Zhang
Jerry
Christian.
Regards,
Jerry
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/5] drm/ttm: initialize globals during device init
Am 22.10.18 um 08:45 schrieb Zhang, Jerry(Junwei):
A question in ttm_bo.c
[SNIP]
int ttm_bo_device_release(struct ttm_bo_device *bdev)
{
@@ -1623,18 +1620,25 @@ int ttm_bo_device_release(struct
ttm_bo_device *bdev)
drm_vma_offset_manager_destroy(&bdev->vma_manager);
+ if (!ret)
+ ttm_bo_global_release();
if ttm_bo_clean_mm() fails, it will skip ttm_bo_global_release().
When will it be called?
Never.
Shall add it to delayed work? or maybe we could release it directly?
No, when ttm_bo_device_release() fails somebody is trying to unload a
driver while this driver still has memory allocated.
In this case BO accounting should not be released because we should make
sure that all the leaked memory is still accounted.
Christian.
Regards,
Jerry
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/5] drm/ttm: initialize globals during device init
A question for ttm_bo.c
On 10/20/2018 12:41 AM, Christian König wrote:
Make sure that the global BO state is always correctly initialized.
This allows removing all the device code to initialize it.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 59 +
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 1 -
drivers/gpu/drm/ast/ast_drv.h | 1 -
drivers/gpu/drm/ast/ast_ttm.c | 36 ---
drivers/gpu/drm/bochs/bochs.h | 1 -
drivers/gpu/drm/bochs/bochs_mm.c| 35 ---
drivers/gpu/drm/cirrus/cirrus_drv.h | 1 -
drivers/gpu/drm/cirrus/cirrus_ttm.c | 36 ---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 1 -
drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 34 --
drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
drivers/gpu/drm/mgag200/mgag200_ttm.c | 36 ---
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 -
drivers/gpu/drm/nouveau/nouveau_ttm.c | 39
drivers/gpu/drm/qxl/qxl_drv.h | 2 -
drivers/gpu/drm/qxl/qxl_ttm.c | 33 --
drivers/gpu/drm/radeon/radeon.h | 2 -
drivers/gpu/drm/radeon/radeon_ttm.c | 39
drivers/gpu/drm/ttm/ttm_bo.c| 19 +---
drivers/gpu/drm/virtio/virtgpu_drv.h| 2 -
drivers/gpu/drm/virtio/virtgpu_ttm.c| 35 ---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 --
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c| 27 ---
drivers/staging/vboxvideo/vbox_ttm.c| 36 ---
include/drm/ttm/ttm_bo_driver.h | 41 +
26 files changed, 16 insertions(+), 516 deletions(-)
[... skip above ...]
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d89183f95570..df028805b7e2 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1530,7 +1530,7 @@ static void ttm_bo_global_kobj_release(struct kobject
*kobj)
kfree(glob);
}
-void ttm_bo_global_release(void)
+static void ttm_bo_global_release(void)
{
struct ttm_bo_global *glob = &ttm_bo_glob;
@@ -1544,9 +1544,8 @@ void ttm_bo_global_release(void)
out:
mutex_unlock(&ttm_global_mutex);
}
-EXPORT_SYMBOL(ttm_bo_global_release);
-int ttm_bo_global_init(void)
+static int ttm_bo_global_init(void)
{
struct ttm_bo_global *glob = &ttm_bo_glob;
int ret = 0;
@@ -1583,8 +1582,6 @@ int ttm_bo_global_init(void)
mutex_unlock(&ttm_global_mutex);
return ret;
}
-EXPORT_SYMBOL(ttm_bo_global_init);
-
int ttm_bo_device_release(struct ttm_bo_device *bdev)
{
@@ -1623,18 +1620,25 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
drm_vma_offset_manager_destroy(&bdev->vma_manager);
+ if (!ret)
+ ttm_bo_global_release();
+
If ttm_bo_clean_mm() fails, it will skip ttm_bo_global_release()
When it will be called?
Shall we add it to a delayed work or we may call it directly here.
Regards
Jerry
return ret;
}
EXPORT_SYMBOL(ttm_bo_device_release);
int ttm_bo_device_init(struct ttm_bo_device *bdev,
- struct ttm_bo_global *glob,
struct ttm_bo_driver *driver,
struct address_space *mapping,
uint64_t file_page_offset,
bool need_dma32)
{
- int ret = -EINVAL;
+ struct ttm_bo_global *glob = &ttm_bo_glob;
+ int ret;
+
+ ret = ttm_bo_global_init();
+ if (ret)
+ return ret;
bdev->driver = driver;
@@ -1661,6 +1665,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
return 0;
out_no_sys:
+ ttm_bo_global_release();
return ret;
}
EXPORT_SYMBOL(ttm_bo_device_init);
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index dec42d421e00..30caa20d9fcf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -132,8 +132,6 @@ struct virtio_gpu_framebuffer {
container_of(x, struct virtio_gpu_framebuffer, base)
struct virtio_gpu_mman {
- struct ttm_bo_global_refbo_global_ref;
- boolmem_global_referenced;
struct ttm_bo_devicebdev;
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index b99ecc6d97d3..c1a56d640121 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -50,35 +50,6 @@ virtio_gpu_device *virtio_gpu_get_vgdev(struct ttm_bo_device
*bdev)
return vgdev;
}
-static int virtio_gpu_ttm_global_init(s
Re: [PATCH 4/5] drm/ttm: initialize globals during device init
A question in ttm_bo.c
On 10/20/2018 12:41 AM, Christian König wrote:
Make sure that the global BO state is always correctly initialized.
This allows removing all the device code to initialize it.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 59 +
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 1 -
drivers/gpu/drm/ast/ast_drv.h | 1 -
drivers/gpu/drm/ast/ast_ttm.c | 36 ---
drivers/gpu/drm/bochs/bochs.h | 1 -
drivers/gpu/drm/bochs/bochs_mm.c| 35 ---
drivers/gpu/drm/cirrus/cirrus_drv.h | 1 -
drivers/gpu/drm/cirrus/cirrus_ttm.c | 36 ---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 1 -
drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 34 --
drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
drivers/gpu/drm/mgag200/mgag200_ttm.c | 36 ---
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 -
drivers/gpu/drm/nouveau/nouveau_ttm.c | 39
drivers/gpu/drm/qxl/qxl_drv.h | 2 -
drivers/gpu/drm/qxl/qxl_ttm.c | 33 --
drivers/gpu/drm/radeon/radeon.h | 2 -
drivers/gpu/drm/radeon/radeon_ttm.c | 39
drivers/gpu/drm/ttm/ttm_bo.c| 19 +---
drivers/gpu/drm/virtio/virtgpu_drv.h| 2 -
drivers/gpu/drm/virtio/virtgpu_ttm.c| 35 ---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 --
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c| 27 ---
drivers/staging/vboxvideo/vbox_ttm.c| 36 ---
include/drm/ttm/ttm_bo_driver.h | 41 +
26 files changed, 16 insertions(+), 516 deletions(-)
[... skip above ...]
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d89183f95570..df028805b7e2 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1530,7 +1530,7 @@ static void ttm_bo_global_kobj_release(struct kobject
*kobj)
kfree(glob);
}
-void ttm_bo_global_release(void)
+static void ttm_bo_global_release(void)
{
struct ttm_bo_global *glob = &ttm_bo_glob;
@@ -1544,9 +1544,8 @@ void ttm_bo_global_release(void)
out:
mutex_unlock(&ttm_global_mutex);
}
-EXPORT_SYMBOL(ttm_bo_global_release);
-int ttm_bo_global_init(void)
+static int ttm_bo_global_init(void)
{
struct ttm_bo_global *glob = &ttm_bo_glob;
int ret = 0;
@@ -1583,8 +1582,6 @@ int ttm_bo_global_init(void)
mutex_unlock(&ttm_global_mutex);
return ret;
}
-EXPORT_SYMBOL(ttm_bo_global_init);
-
int ttm_bo_device_release(struct ttm_bo_device *bdev)
{
@@ -1623,18 +1620,25 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
drm_vma_offset_manager_destroy(&bdev->vma_manager);
+ if (!ret)
+ ttm_bo_global_release();
if ttm_bo_clean_mm() fails, it will skip ttm_bo_global_release().
When will it be called?
Shall add it to delayed work? or maybe we could release it directly?
Regards,
Jerry
+
return ret;
}
EXPORT_SYMBOL(ttm_bo_device_release);
int ttm_bo_device_init(struct ttm_bo_device *bdev,
- struct ttm_bo_global *glob,
struct ttm_bo_driver *driver,
struct address_space *mapping,
uint64_t file_page_offset,
bool need_dma32)
{
- int ret = -EINVAL;
+ struct ttm_bo_global *glob = &ttm_bo_glob;
+ int ret;
+
+ ret = ttm_bo_global_init();
+ if (ret)
+ return ret;
bdev->driver = driver;
@@ -1661,6 +1665,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
return 0;
out_no_sys:
+ ttm_bo_global_release();
return ret;
}
EXPORT_SYMBOL(ttm_bo_device_init);
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index dec42d421e00..30caa20d9fcf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -132,8 +132,6 @@ struct virtio_gpu_framebuffer {
container_of(x, struct virtio_gpu_framebuffer, base)
struct virtio_gpu_mman {
- struct ttm_bo_global_refbo_global_ref;
- boolmem_global_referenced;
struct ttm_bo_devicebdev;
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index b99ecc6d97d3..c1a56d640121 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -50,35 +50,6 @@ virtio_gpu_device *virtio_gpu_get_vgdev(struct ttm_bo_device
*bdev)
return vgdev;
}
-static int virtio_gpu_ttm_global_init
[PATCH 4/5] drm/ttm: initialize globals during device init
Make sure that the global BO state is always correctly initialized.
This allows removing all the device code to initialize it.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 59 +
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 1 -
drivers/gpu/drm/ast/ast_drv.h | 1 -
drivers/gpu/drm/ast/ast_ttm.c | 36 ---
drivers/gpu/drm/bochs/bochs.h | 1 -
drivers/gpu/drm/bochs/bochs_mm.c| 35 ---
drivers/gpu/drm/cirrus/cirrus_drv.h | 1 -
drivers/gpu/drm/cirrus/cirrus_ttm.c | 36 ---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 1 -
drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 34 --
drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
drivers/gpu/drm/mgag200/mgag200_ttm.c | 36 ---
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 -
drivers/gpu/drm/nouveau/nouveau_ttm.c | 39
drivers/gpu/drm/qxl/qxl_drv.h | 2 -
drivers/gpu/drm/qxl/qxl_ttm.c | 33 --
drivers/gpu/drm/radeon/radeon.h | 2 -
drivers/gpu/drm/radeon/radeon_ttm.c | 39
drivers/gpu/drm/ttm/ttm_bo.c| 19 +---
drivers/gpu/drm/virtio/virtgpu_drv.h| 2 -
drivers/gpu/drm/virtio/virtgpu_ttm.c| 35 ---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 --
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c| 27 ---
drivers/staging/vboxvideo/vbox_ttm.c| 36 ---
include/drm/ttm/ttm_bo_driver.h | 41 +
26 files changed, 16 insertions(+), 516 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index fda252022b15..31fe85dd0b50 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -61,56 +61,6 @@ static int amdgpu_map_buffer(struct ttm_buffer_object *bo,
static int amdgpu_ttm_debugfs_init(struct amdgpu_device *adev);
static void amdgpu_ttm_debugfs_fini(struct amdgpu_device *adev);
-/*
- * Global memory.
- */
-
-/**
- * amdgpu_ttm_global_init - Initialize global TTM memory reference structures.
- *
- * @adev: AMDGPU device for which the global structures need to be registered.
- *
- * This is called as part of the AMDGPU ttm init from amdgpu_ttm_init()
- * during bring up.
- */
-static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
-{
- struct drm_global_reference *global_ref;
- int r;
-
- /* ensure reference is false in case init fails */
- adev->mman.mem_global_referenced = false;
-
- global_ref = &adev->mman.bo_global_ref.ref;
- global_ref->global_type = DRM_GLOBAL_TTM_BO;
- global_ref->size = sizeof(struct ttm_bo_global);
- global_ref->init = &ttm_bo_global_ref_init;
- global_ref->release = &ttm_bo_global_ref_release;
- r = drm_global_item_ref(global_ref);
- if (r) {
- DRM_ERROR("Failed setting up TTM BO subsystem.\n");
- goto error_bo;
- }
-
- mutex_init(&adev->mman.gtt_window_lock);
-
- adev->mman.mem_global_referenced = true;
-
- return 0;
-
-error_bo:
- return r;
-}
-
-static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
-{
- if (adev->mman.mem_global_referenced) {
- mutex_destroy(&adev->mman.gtt_window_lock);
- drm_global_item_unref(&adev->mman.bo_global_ref.ref);
- adev->mman.mem_global_referenced = false;
- }
-}
-
static int amdgpu_invalidate_caches(struct ttm_bo_device *bdev, uint32_t flags)
{
return 0;
@@ -1714,14 +1664,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
int r;
u64 vis_vram_limit;
- /* initialize global references for vram/gtt */
- r = amdgpu_ttm_global_init(adev);
- if (r) {
- return r;
- }
+ mutex_init(&adev->mman.gtt_window_lock);
+
/* No others user of address space so set it to 0 */
r = ttm_bo_device_init(&adev->mman.bdev,
- adev->mman.bo_global_ref.ref.object,
&amdgpu_bo_driver,
adev->ddev->anon_inode->i_mapping,
DRM_FILE_PAGE_OFFSET,
@@ -1878,7 +1824,6 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
ttm_bo_clean_mm(&adev->mman.bdev, AMDGPU_PL_GWS);
ttm_bo_clean_mm(&adev->mman.bdev, AMDGPU_PL_OA);
ttm_bo_device_release(&adev->mman.bdev);
- amdgpu_ttm_global_fini(adev);
adev->mman.initialized = false;
DRM_INFO("amdgpu: ttm finalized\n");
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu
