This series of patches aims to fix some bugs and memory leaks in virtio-gpu deinitialization paths.
While denitialization paths is not usually executed in any virtio-gpu usecase, it is useful for testing during implementation of virtio-gpu device part.
Damir Shaikhutdinov (4): drm/virtio: Fix memory leak during framebuffer destruction. drm/virtio: Remove incorrect kfree during connector destruction. drm/virtio: Fix virtio gpu fbdev deinitialization. drm/virtio: Fix connector leak during virtio-gpu deinitialization.
drivers/gpu/drm/virtio/virtgpu_display.c | 6 +----- drivers/gpu/drm/virtio/virtgpu_fb.c | 7 +++++-- drivers/gpu/drm/virtio/virtgpu_kms.c | 2 ++ 3 files changed, 8 insertions(+), 7 deletions(-)
In function virtio_gpufb_create, a virtio_gpu_object is allocated for framebuffer using virtio_gpu_alloc_object.
In virtio_gpu_fbdev_destroy, instead of freeing the object, pointer to it is set to NULL. This leads to memory leak during framebuffer destruction, which is reported to kmesg with a message like this:
Memory manager not clean during takedown.
With DRM_DEBUG_MM enabled, following additional information is printed about the leak: node [00100000 + 000001d5]: inserted at save_stack.isra.9+0x67/0xc0 drm_mm_insert_node_in_range+0x325/0x4f0 drm_vma_offset_add+0x46/0x60 ttm_bo_init_reserved+0x2c9/0x400 ttm_bo_init+0x2a/0x80 virtio_gpu_object_create+0x139/0x180 virtio_gpu_alloc_object+0x2f/0x60 virtio_gpufb_create+0xac/0x2a0
Correctly freeing virtio_gpu_object during framebuffer destruction fixes the issue.
Signed-off-by: Damir Shaikhutdinov damir.shaikhutdinov@opensynergy.com Signed-off-by: Kiran Pawar kiran.pawar@opensynergy.com --- drivers/gpu/drm/virtio/virtgpu_fb.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c index 15d18fd0c64b..10a66a387bfb 100644 --- a/drivers/gpu/drm/virtio/virtgpu_fb.c +++ b/drivers/gpu/drm/virtio/virtgpu_fb.c @@ -301,11 +301,14 @@ static int virtio_gpu_fbdev_destroy(struct drm_device *dev,
drm_fb_helper_unregister_fbi(&vgfbdev->helper);
- if (vgfb->obj) - vgfb->obj = NULL; drm_fb_helper_fini(&vgfbdev->helper); drm_framebuffer_cleanup(&vgfb->base);
+ if (vgfb->obj) { + virtio_gpu_gem_free_object(vgfb->obj); + vgfb->obj = NULL; + } + return 0; } static const struct drm_fb_helper_funcs virtio_gpu_fb_helper_funcs = {
In function virtio_gpu_conn_destroy a pointer to a containing structure virtio_gpu_output is received using drm_connector_to_virtio_gpu_output (container_of), and then it is passed to kfree function.
But this pointer points to a member of array (vgdev->outputs + index) (see vgdev_output_init):
struct virtio_gpu_output *output = vgdev->outputs + index; struct drm_connector *connector = &output->conn; ... drm_connector_init(dev, connector, &virtio_gpu_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
Signed-off-by: Damir Shaikhutdinov damir.shaikhutdinov@opensynergy.com --- drivers/gpu/drm/virtio/virtgpu_display.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index b6d52055a11f..d211d4e98b46 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -243,12 +243,8 @@ static enum drm_connector_status virtio_gpu_conn_detect(
static void virtio_gpu_conn_destroy(struct drm_connector *connector) { - struct virtio_gpu_output *virtio_gpu_output = - drm_connector_to_virtio_gpu_output(connector); - drm_connector_unregister(connector); drm_connector_cleanup(connector); - kfree(virtio_gpu_output); }
static const struct drm_connector_funcs virtio_gpu_connector_funcs = {
Module parameter virtio_gpu_fbdev is used to enable or disable fbdev in virtio. It is checked during fbdev initialization, but is not checked during deinitialization.
Moving fbdev destruction to virtgpu_kms.c instead of virtgpu_display.c places deinitialization to the same file as initialization, and allows checking for virtio_gpu_fbdev module parameter.
Signed-off-by: Damir Shaikhutdinov damir.shaikhutdinov@opensynergy.com --- drivers/gpu/drm/virtio/virtgpu_display.c | 1 - drivers/gpu/drm/virtio/virtgpu_kms.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d211d4e98b46..d314e3c672f2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -377,6 +377,5 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev) { - virtio_gpu_fbdev_fini(vgdev); drm_mode_config_cleanup(vgdev->ddev); } diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c index 6400506a06b0..b994ecfdd378 100644 --- a/drivers/gpu/drm/virtio/virtgpu_kms.c +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c @@ -259,6 +259,8 @@ void virtio_gpu_driver_unload(struct drm_device *dev) flush_work(&vgdev->config_changed_work); vgdev->vdev->config->del_vqs(vgdev->vdev);
+ if (virtio_gpu_fbdev) + virtio_gpu_fbdev_fini(vgdev); virtio_gpu_modeset_fini(vgdev); virtio_gpu_ttm_fini(vgdev); virtio_gpu_free_vbufs(vgdev);
Attaching CRTC to a connector increases its reference count, preventing it from correct deinitialization. Following kernel log is printed when the leak is found:
Console: switching to colour VGA+ 80x25 WARNING: at drivers/gpu/drm/drm_mode_config.c:431 ... Call Trace: drm_mode_config_cleanup virtio_gpu_modeset_fini virtio_gpu_driver_unload drm_dev_unregister drm_put_dev virtio_gpu_remove virtio_dev_remove device_release_driver_internal device_release_driver bus_remove_device device_del device_unregister unregister_virtio_device ... [drm:drm_mode_config_cleanup] ERROR connector Virtual-1 leaked!
Calling drm_atomic_helper_shutdown disconnects CRTCs from connectors, allowing them to be freed during drm_mode_config_cleanup.
Signed-off-by: Damir Shaikhutdinov damir.shaikhutdinov@opensynergy.com Signed-off-by: Kiran Pawar kiran.pawar@opensynergy.com --- drivers/gpu/drm/virtio/virtgpu_display.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d314e3c672f2..088a751a35e9 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -377,5 +377,6 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev) { + drm_atomic_helper_shutdown(vgdev->ddev); drm_mode_config_cleanup(vgdev->ddev); }
dri-devel@lists.freedesktop.org
-
Damir Shaikhutdinov