Some progress. Not complete though, I still get an unclean mm warning on shutdown due to some release objects not being freed yet.
Gerd Hoffmann (4): 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
drivers/gpu/drm/qxl/qxl_display.c | 11 +++++++++-- drivers/gpu/drm/qxl/qxl_release.c | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)
Signed-off-by: Gerd Hoffmann kraxel@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- 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); }
Am 20.01.21 um 12:12 schrieb Gerd Hoffmann:
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 0fcfc952d5e9..add979cba11b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) entry = container_of(release->bos.next, struct qxl_bo_list, tv.head); bo = to_qxl_bo(entry->tv.bo); + bo->tbo.pin_count = 0; /* ttm_bo_unpin(&bo->tbo); */ qxl_bo_unref(&bo); list_del(&entry->tv.head); kfree(entry);
Hi
Am 20.01.21 um 12:12 schrieb Gerd Hoffmann:
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 0fcfc952d5e9..add979cba11b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) entry = container_of(release->bos.next, struct qxl_bo_list, tv.head); bo = to_qxl_bo(entry->tv.bo);
bo->tbo.pin_count = 0; /* ttm_bo_unpin(&bo->tbo); */
This code looks like a workaround or a bug.
AFAICT the only place with pre-pinned BO is qdev->dumb_shadow_bo. Can you remove the pinned flag entirely and handle pinning as part of dumb_shadow_bo's code.
Otherwise maybe use
if (pin_count) ttm_bo_unpin(); WARN_ON(pin_count); /* should always be 0 now */
with a comment similar to the commit's description.
Best regards Thomas
qxl_bo_unref(&bo); list_del(&entry->tv.head); kfree(entry);
On Fri, Jan 22, 2021 at 09:13:42AM +0100, Thomas Zimmermann wrote:
Hi
Am 20.01.21 um 12:12 schrieb Gerd Hoffmann:
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 0fcfc952d5e9..add979cba11b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) entry = container_of(release->bos.next, struct qxl_bo_list, tv.head); bo = to_qxl_bo(entry->tv.bo);
bo->tbo.pin_count = 0; /* ttm_bo_unpin(&bo->tbo); */This code looks like a workaround or a bug.
AFAICT the only place with pre-pinned BO is qdev->dumb_shadow_bo. Can you remove the pinned flag entirely and handle pinning as part of dumb_shadow_bo's code.
No, the release objects are pinned too, and they must be pinned (qxl commands are in there, and references are placed in the qxl rings, so allowing them to roam is a non-starter).
if (pin_count) ttm_bo_unpin(); WARN_ON(pin_count); /* should always be 0 now */
Well, the pin_count is 1 at this point. No need for the if().
Just calling ttm_bo_unpin() here makes lockdep unhappy.
Not calling ttm_bo_unpin() makes ttm_bo_release() throw a WARN() because of the pin.
Clearing pin_count (which is how ttm fixes things up in the error path) works.
I'm open to better ideas.
take care, Gerd
On Fri, Jan 22, 2021 at 2:35 PM Gerd Hoffmann kraxel@redhat.com wrote:
On Fri, Jan 22, 2021 at 09:13:42AM +0100, Thomas Zimmermann wrote:
Hi
Am 20.01.21 um 12:12 schrieb Gerd Hoffmann:
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 0fcfc952d5e9..add979cba11b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) entry = container_of(release->bos.next, struct qxl_bo_list, tv.head); bo = to_qxl_bo(entry->tv.bo);
bo->tbo.pin_count = 0; /* ttm_bo_unpin(&bo->tbo); */This code looks like a workaround or a bug.
AFAICT the only place with pre-pinned BO is qdev->dumb_shadow_bo. Can you remove the pinned flag entirely and handle pinning as part of dumb_shadow_bo's code.
No, the release objects are pinned too, and they must be pinned (qxl commands are in there, and references are placed in the qxl rings, so allowing them to roam is a non-starter).
if (pin_count) ttm_bo_unpin(); WARN_ON(pin_count); /* should always be 0 now */
Well, the pin_count is 1 at this point. No need for the if().
Just calling ttm_bo_unpin() here makes lockdep unhappy.
How does that one splat? But yeah if that's a problem should be explained in the comment. I'd then also only do a pin_count--; to make sure you can still catch other pin leaks if you have them. Setting it to 0 kinda defeats the warning. -Daniel
Not calling ttm_bo_unpin() makes ttm_bo_release() throw a WARN() because of the pin.
Clearing pin_count (which is how ttm fixes things up in the error path) works.
I'm open to better ideas.
take care, Gerd
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Just calling ttm_bo_unpin() here makes lockdep unhappy.
How does that one splat? But yeah if that's a problem should be explained in the comment. I'd then also only do a pin_count--; to make sure you can still catch other pin leaks if you have them. Setting it to 0 kinda defeats the warning.
Figured the unpin is at the completely wrong place while trying to reproduce the lockdep splat ...
take care, Gerd
From 43befab4a935114e8620af62781666fa81288255 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann kraxel@redhat.com Date: Mon, 25 Jan 2021 13:10:50 +0100 Subject: [PATCH] drm/qxl: unpin release objects
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); }
Hi
Am 20.01.21 um 12:12 schrieb Gerd Hoffmann:
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) {
Wrt to my comment on patch 2, this might be the place to unpin the 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;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gerd Hoffmann -
Thomas Zimmermann