Almost there. Still getting this on driver unbind:
kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put(= ) is being called [ ... ] Call Trace: ttm_device_fini+0x133/0x1b0 [ttm] qxl_ttm_fini+0x2f/0x40 [qxl] qxl_device_fini+0x88/0x120 [qxl] drm_minor_release+0x3d/0x60
but I don't think this is the qxl driver's fault.
Gerd Hoffmann (5): drm/qxl: use drmm_mode_config_init drm/qxl: unpin release objects drm/qxl: release shadow on shutdown drm/qxl: handle shadow in primary destroy drm/qxl: properly free qxl releases
drivers/gpu/drm/qxl/qxl_drv.h | 1 + drivers/gpu/drm/qxl/qxl_display.c | 11 +++++++++-- drivers/gpu/drm/qxl/qxl_kms.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/qxl/qxl_release.c | 3 +++ 4 files changed, 33 insertions(+), 4 deletions(-)
--=20 2.29.2
Signed-off-by: Gerd Hoffmann kraxel@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Acked-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/qxl/qxl_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 012bce0cdb65..38d6b596094d 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1195,7 +1195,9 @@ int qxl_modeset_init(struct qxl_device *qdev) int i; int ret;
- drm_mode_config_init(&qdev->ddev); + ret = drmm_mode_config_init(&qdev->ddev); + if (ret) + return ret;
ret = qxl_create_monitors_object(qdev); if (ret) @@ -1228,5 +1230,4 @@ int qxl_modeset_init(struct qxl_device *qdev) void qxl_modeset_fini(struct qxl_device *qdev) { qxl_destroy_monitors_object(qdev); - drm_mode_config_cleanup(&qdev->ddev); }
Balances the qxl_create_bo(..., pinned=true, ...); call in qxl_release_bo_alloc().
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_release.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index c52412724c26..28013fd1f8ea 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -347,6 +347,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
mutex_lock(&qdev->release_mutex); if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) { + qxl_bo_unpin(qdev->current_release_bo[cur_idx]); qxl_bo_unref(&qdev->current_release_bo[cur_idx]); qdev->current_release_bo_offset[cur_idx] = 0; qdev->current_release_bo[cur_idx] = NULL;
In case we have a shadow surface on shutdown release it so it doesn't leak.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 38d6b596094d..60331e31861a 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1229,5 +1229,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev) { + if (qdev->dumb_shadow_bo) { + drm_gem_object_put(&qdev->dumb_shadow_bo->tbo.base); + qdev->dumb_shadow_bo = NULL; + } qxl_destroy_monitors_object(qdev); }
qxl_primary_atomic_disable must check whenever the framebuffer bo has a shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 60331e31861a..f5ee8cd72b5b 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -562,6 +562,8 @@ static void qxl_primary_atomic_disable(struct drm_plane *plane, if (old_state->fb) { struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow) + bo = bo->shadow; if (bo->is_primary) { qxl_io_destroy_primary(qdev); bo->is_primary = false;
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_drv.h | 1 + drivers/gpu/drm/qxl/qxl_kms.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/qxl/qxl_release.c | 2 ++ 3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 01354b43c413..1c57b587b6a7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -214,6 +214,7 @@ struct qxl_device { spinlock_t release_lock; struct idr release_idr; uint32_t release_seqno; + atomic_t release_count; spinlock_t release_idr_lock; struct mutex async_io_mutex; unsigned int last_sent_io_cmd; diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 4a60a52ab62e..f177f72bfc12 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -25,6 +25,7 @@
#include <linux/io-mapping.h> #include <linux/pci.h> +#include <linux/delay.h>
#include <drm/drm_drv.h> #include <drm/drm_managed.h> @@ -286,8 +287,25 @@ int qxl_device_init(struct qxl_device *qdev,
void qxl_device_fini(struct qxl_device *qdev) { - qxl_bo_unref(&qdev->current_release_bo[0]); - qxl_bo_unref(&qdev->current_release_bo[1]); + int cur_idx, try; + + for (cur_idx = 0; cur_idx < 3; cur_idx++) { + if (!qdev->current_release_bo[cur_idx]) + continue; + qxl_bo_unpin(qdev->current_release_bo[cur_idx]); + qxl_bo_unref(&qdev->current_release_bo[cur_idx]); + qdev->current_release_bo_offset[cur_idx] = 0; + qdev->current_release_bo[cur_idx] = NULL; + } + + /* + * Ask host to release resources (+fill release ring), + * then wait for the release actually happening. + */ + qxl_io_notify_oom(qdev); + for (try = 0; try < 20 && atomic_read(&qdev->release_count) > 0; try++) + msleep(20); + qxl_gem_fini(qdev); qxl_bo_fini(qdev); flush_work(&qdev->gc_work); diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 28013fd1f8ea..43a5436853b7 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -196,6 +196,7 @@ qxl_release_free(struct qxl_device *qdev, qxl_release_free_list(release); kfree(release); } + atomic_dec(&qdev->release_count); }
static int qxl_release_bo_alloc(struct qxl_device *qdev, @@ -344,6 +345,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, *rbo = NULL; return idr_ret; } + atomic_inc(&qdev->release_count);
mutex_lock(&qdev->release_mutex); if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) {
On Tue, Jan 26, 2021 at 05:58:12PM +0100, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_drv.h | 1 + drivers/gpu/drm/qxl/qxl_kms.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/qxl/qxl_release.c | 2 ++ 3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 01354b43c413..1c57b587b6a7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -214,6 +214,7 @@ struct qxl_device { spinlock_t release_lock; struct idr release_idr; uint32_t release_seqno;
- atomic_t release_count; spinlock_t release_idr_lock; struct mutex async_io_mutex; unsigned int last_sent_io_cmd;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 4a60a52ab62e..f177f72bfc12 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -25,6 +25,7 @@
#include <linux/io-mapping.h> #include <linux/pci.h> +#include <linux/delay.h>
#include <drm/drm_drv.h> #include <drm/drm_managed.h> @@ -286,8 +287,25 @@ int qxl_device_init(struct qxl_device *qdev,
void qxl_device_fini(struct qxl_device *qdev) {
- qxl_bo_unref(&qdev->current_release_bo[0]);
- qxl_bo_unref(&qdev->current_release_bo[1]);
- int cur_idx, try;
- for (cur_idx = 0; cur_idx < 3; cur_idx++) {
if (!qdev->current_release_bo[cur_idx])continue;qxl_bo_unpin(qdev->current_release_bo[cur_idx]);qxl_bo_unref(&qdev->current_release_bo[cur_idx]);qdev->current_release_bo_offset[cur_idx] = 0;qdev->current_release_bo[cur_idx] = NULL;- }
- /*
* Ask host to release resources (+fill release ring),* then wait for the release actually happening.*/- qxl_io_notify_oom(qdev);
- for (try = 0; try < 20 && atomic_read(&qdev->release_count) > 0; try++)
msleep(20);
A bit icky, why not use a wait queue or something like that instead of hand-rolling this? Not for perf reasons, just so it's a bit clear who waits for whom and why. -Daniel
- qxl_gem_fini(qdev); qxl_bo_fini(qdev); flush_work(&qdev->gc_work);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 28013fd1f8ea..43a5436853b7 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -196,6 +196,7 @@ qxl_release_free(struct qxl_device *qdev, qxl_release_free_list(release); kfree(release); }
- atomic_dec(&qdev->release_count);
}
static int qxl_release_bo_alloc(struct qxl_device *qdev, @@ -344,6 +345,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, *rbo = NULL; return idr_ret; }
atomic_inc(&qdev->release_count);
mutex_lock(&qdev->release_mutex); if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) {
-- 2.29.2
- /*
* Ask host to release resources (+fill release ring),* then wait for the release actually happening.*/- qxl_io_notify_oom(qdev);
- for (try = 0; try < 20 && atomic_read(&qdev->release_count) > 0; try++)
msleep(20);A bit icky, why not use a wait queue or something like that instead of hand-rolling this? Not for perf reasons, just so it's a bit clear who waits for whom and why.
There isn't one ready for use, and adding a new one (to wait for the garbage collection having released something) just for a clean shutdown looked a bit like overkill.
But after digging a bit more and looking at the qxl_fence_wait() mess I think adding a wait queue looks like a good idea ...
v5 coming soon ...
take care, Gerd
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gerd Hoffmann