v5: - add release_event wait queue. - also cleanup qxl_fence_wait(). v6: - add shadow pinning fix (Thomas). - use ram for dumb allocation.
Gerd Hoffmann (10): [hack] silence ttm fini WARNING Revert "drm/qxl: do not run release if qxl failed to init" drm/qxl: use drmm_mode_config_init drm/qxl: unpin release objects drm/qxl: release shadow on shutdown drm/qxl: properly pin/unpin shadow drm/qxl: handle shadow in primary destroy drm/qxl: properly free qxl releases drm/qxl: simplify qxl_fence_wait drm/qxl: allocate dumb buffers in ram
drivers/gpu/drm/qxl/qxl_drv.h | 2 ++ drivers/gpu/drm/qxl/qxl_cmd.c | 1 + drivers/gpu/drm/qxl/qxl_display.c | 15 ++++++++-- drivers/gpu/drm/qxl/qxl_drv.c | 2 -- drivers/gpu/drm/qxl/qxl_dumb.c | 2 +- drivers/gpu/drm/qxl/qxl_irq.c | 1 + drivers/gpu/drm/qxl/qxl_kms.c | 28 +++++++++++++++--- drivers/gpu/drm/qxl/qxl_release.c | 47 +++++-------------------------- drivers/gpu/drm/ttm/ttm_device.c | 2 +- 9 files changed, 50 insertions(+), 50 deletions(-)
kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put() is being called. WARNING: CPU: 0 PID: 209 at lib/kobject.c:750 kobject_put+0x3a/0x60 [ ... ] Call Trace: ttm_device_fini+0x133/0x1b0 [ttm] qxl_ttm_fini+0x2f/0x40 [qxl] --- drivers/gpu/drm/ttm/ttm_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index ac0903c9e60a..b638cbb0bd45 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -50,7 +50,7 @@ static void ttm_global_release(void) goto out;
kobject_del(&glob->kobj); - kobject_put(&glob->kobj); +// kobject_put(&glob->kobj); ttm_mem_global_release(&ttm_mem_glob); __free_page(glob->dummy_read_page); memset(glob, 0, sizeof(*glob));
?
What's the background here?
Christian.
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put() is being called. WARNING: CPU: 0 PID: 209 at lib/kobject.c:750 kobject_put+0x3a/0x60 [ ... ] Call Trace: ttm_device_fini+0x133/0x1b0 [ttm] qxl_ttm_fini+0x2f/0x40 [qxl]
drivers/gpu/drm/ttm/ttm_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index ac0903c9e60a..b638cbb0bd45 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -50,7 +50,7 @@ static void ttm_global_release(void) goto out;
kobject_del(&glob->kobj);
- kobject_put(&glob->kobj);
+// kobject_put(&glob->kobj); ttm_mem_global_release(&ttm_mem_glob); __free_page(glob->dummy_read_page); memset(glob, 0, sizeof(*glob));
On Thu, Feb 04, 2021 at 03:58:33PM +0100, Christian König wrote:
?
What's the background here?
Christian.
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put() is being called. WARNING: CPU: 0 PID: 209 at lib/kobject.c:750 kobject_put+0x3a/0x60 [ ... ] Call Trace: ttm_device_fini+0x133/0x1b0 [ttm] qxl_ttm_fini+0x2f/0x40 [qxl]
Happens on driver removal. Seen with both qxl and bochs (the later using vram helpers).
Testcase: "drmtest --unbind" (https://git.kraxel.org/cgit/drminfo/).
static int try_unbind(int card) { char path[256]; char *device, *name; int fd;
snprintf(path, sizeof(path), "/sys/class/drm/card%d/device", card); device = realpath(path, NULL); if (device == NULL) { fprintf(stderr, "%s: can't resolve sysfs device path\n", __func__); return -1; }
snprintf(path, sizeof(path), "%s/driver/unbind", device); fd = open(path, O_WRONLY); if (fd < 0) { fprintf(stderr, "open %s: %s\n", path, strerror(errno)); return -1; }
name = strrchr(device, '/') + 1; write(fd, name, strlen(name)); close(fd); return 0; }
take care, Gerd
This reverts commit b91907a6241193465ca92e357adf16822242296d.
Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister().
Cc: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */ - if (!dev->registered) - return; qxl_modeset_fini(qdev); qxl_device_fini(qdev); }
Hi
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
This reverts commit b91907a6241193465ca92e357adf16822242296d.
This should be in the correct format, as given by 'dim cite'.
dim cite b91907a6241193465ca92e357adf16822242296d b91907a62411 ("drm/qxl: do not run release if qxl failed to init")
Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister().
Cc: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */
- if (!dev->registered)
return;
I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly.
With the citation style address:
Acked-by: Thomas Zimmermann tzimmermann@suse.de
qxl_modeset_fini(qdev); qxl_device_fini(qdev); }
Hi Thomas,
The original problem was qxl_device_init() can fail, when it fails there is no need to call qxl_modeset_fini(qdev); qxl_device_fini(qdev); But those two functions are otherwise called in the qxl_drm_release() -
I have posted an updated patch. The new patch use the following logic
+ if (!qdev->ddev.mode_config.funcs) + return; qxl_modeset_fini(qdev); qxl_device_fini(qdev);
Thanks, - Tong
On Feb 4, 2021, at 1:34 PM, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
This reverts commit b91907a6241193465ca92e357adf16822242296d.
This should be in the correct format, as given by 'dim cite'.
dim cite b91907a6241193465ca92e357adf16822242296d b91907a62411 ("drm/qxl: do not run release if qxl failed to init")
Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister(). Cc: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */
- if (!dev->registered)
return;I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly.
With the citation style address:
Acked-by: Thomas Zimmermann tzimmermann@suse.de
qxl_modeset_fini(qdev); qxl_device_fini(qdev); }
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Hi Tong
Am 04.02.21 um 19:52 schrieb Tong Zhang:
Hi Thomas,
The original problem was qxl_device_init() can fail, when it fails there is no need to call qxl_modeset_fini(qdev); qxl_device_fini(qdev); But those two functions are otherwise called in the qxl_drm_release() -
OK, makes sense. Thanks for the explanation.
I have posted an updated patch. The new patch use the following logic
- if (!qdev->ddev.mode_config.funcs)
return;
This is again just papering over the issue. Better don't call qxl_drm_release() in the error path if qxl_device_init() fails.
I see two solutions: either roll-back manually, or use our new managed DRM interfaces. This is what the other drivers do.
Best regards Thomas
qxl_modeset_fini(qdev); qxl_device_fini(qdev);
Thanks,
- Tong
On Feb 4, 2021, at 1:34 PM, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
This reverts commit b91907a6241193465ca92e357adf16822242296d.
This should be in the correct format, as given by 'dim cite'.
dim cite b91907a6241193465ca92e357adf16822242296d b91907a62411 ("drm/qxl: do not run release if qxl failed to init")
Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister(). Cc: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */
- if (!dev->registered)
return;I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly.
With the citation style address:
Acked-by: Thomas Zimmermann tzimmermann@suse.de
qxl_modeset_fini(qdev); qxl_device_fini(qdev); }
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
On Feb 4, 2021, at 2:06 PM, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi Tong
I have posted an updated patch. The new patch use the following logic
- if (!qdev->ddev.mode_config.funcs)
return;This is again just papering over the issue. Better don't call qxl_drm_release() in the error path if qxl_device_init() fails.
I see two solutions: either roll-back manually, or use our new managed DRM interfaces. This is what the other drivers do.
Best regards Thomas
IMHO - qdev->ddev.mode_config.funcs is set only if the initialization is successful, so using this as an indicator of error case looks ok to me.
The other two options you suggested would require some serious significant amount of work to be done, which I don’t think I currently have such ability to do.
Thanks, - Tong
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 Acked-by: Thomas Zimmermann tzimmermann@suse.de --- 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); }
Am 04.02.21 um 15:57 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
Acked-by: Thomas Zimmermann tzimmermann@suse.de
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); }
Suggested-by: Thomas Zimmermann tzimmermann@suse.de 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 60331e31861a..d25fd3acc891 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -802,12 +802,14 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } if (user_bo->shadow != qdev->dumb_shadow_bo) { if (user_bo->shadow) { + qxl_bo_unpin(user_bo->shadow); drm_gem_object_put (&user_bo->shadow->tbo.base); user_bo->shadow = NULL; } drm_gem_object_get(&qdev->dumb_shadow_bo->tbo.base); user_bo->shadow = qdev->dumb_shadow_bo; + qxl_bo_pin(user_bo->shadow); } }
@@ -833,6 +835,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, qxl_bo_unpin(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) { + qxl_bo_unpin(user_bo->shadow); drm_gem_object_put(&user_bo->shadow->tbo.base); user_bo->shadow = NULL; } @@ -1230,6 +1233,7 @@ int qxl_modeset_init(struct qxl_device *qdev) void qxl_modeset_fini(struct qxl_device *qdev) { if (qdev->dumb_shadow_bo) { + qxl_bo_unpin(qdev->dumb_shadow_bo); drm_gem_object_put(&qdev->dumb_shadow_bo->tbo.base); qdev->dumb_shadow_bo = NULL; }
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
Suggested-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Gerd Hoffmann kraxel@redhat.com
Thanks for this.
Acked-by: Thomas Zimmermann tzimmermann@suse.de
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 60331e31861a..d25fd3acc891 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -802,12 +802,14 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } if (user_bo->shadow != qdev->dumb_shadow_bo) { if (user_bo->shadow) {
qxl_bo_unpin(user_bo->shadow); drm_gem_object_put (&user_bo->shadow->tbo.base); user_bo->shadow = NULL; } drm_gem_object_get(&qdev->dumb_shadow_bo->tbo.base); user_bo->shadow = qdev->dumb_shadow_bo;qxl_bo_pin(user_bo->shadow);@@ -833,6 +835,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, qxl_bo_unpin(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) {
qxl_bo_unpin(user_bo->shadow);@@ -1230,6 +1233,7 @@ int qxl_modeset_init(struct qxl_device *qdev) void qxl_modeset_fini(struct qxl_device *qdev) { if (qdev->dumb_shadow_bo) {
qxl_bo_unpin(qdev->dumb_shadow_bo);
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 Acked-by: Thomas Zimmermann tzimmermann@suse.de --- 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 d25fd3acc891..c326412136c5 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;
Reorganize qxl_device_fini() a bit. Add missing unpin() calls.
Count releases. Add wait queue for releases. That way qxl_device_fini() can easily wait until everything is ready for proper shutdown.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com Acked-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/qxl/qxl_drv.h | 2 ++ drivers/gpu/drm/qxl/qxl_cmd.c | 1 + drivers/gpu/drm/qxl/qxl_irq.c | 1 + drivers/gpu/drm/qxl/qxl_kms.c | 28 ++++++++++++++++++++++++---- drivers/gpu/drm/qxl/qxl_release.c | 2 ++ 5 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 01354b43c413..6dd57cfb2e7c 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -214,6 +214,8 @@ struct qxl_device { spinlock_t release_lock; struct idr release_idr; uint32_t release_seqno; + atomic_t release_count; + wait_queue_head_t release_event; spinlock_t release_idr_lock; struct mutex async_io_mutex; unsigned int last_sent_io_cmd; diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index 54e3c3a97440..7e22a81bfb36 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -254,6 +254,7 @@ int qxl_garbage_collect(struct qxl_device *qdev) } }
+ wake_up_all(&qdev->release_event); DRM_DEBUG_DRIVER("%d\n", i);
return i; diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c index ddf6588a2a38..d312322cacd1 100644 --- a/drivers/gpu/drm/qxl/qxl_irq.c +++ b/drivers/gpu/drm/qxl/qxl_irq.c @@ -87,6 +87,7 @@ int qxl_irq_init(struct qxl_device *qdev) init_waitqueue_head(&qdev->display_event); init_waitqueue_head(&qdev->cursor_event); init_waitqueue_head(&qdev->io_cmd_event); + init_waitqueue_head(&qdev->release_event); INIT_WORK(&qdev->client_monitors_config_work, qxl_client_monitors_config_work_func); atomic_set(&qdev->irq_received, 0); diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 4a60a52ab62e..66d74aaaee06 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -286,11 +286,31 @@ 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]); - qxl_gem_fini(qdev); - qxl_bo_fini(qdev); + int cur_idx; + + 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); + wait_event_timeout(qdev->release_event, + atomic_read(&qdev->release_count) == 0, + HZ); flush_work(&qdev->gc_work); + qxl_surf_evict(qdev); + qxl_vram_evict(qdev); + + qxl_gem_fini(qdev); + qxl_bo_fini(qdev); qxl_ring_free(qdev->command_ring); qxl_ring_free(qdev->cursor_ring); qxl_ring_free(qdev->release_ring); 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]) {
Now that we have the new release_event wait queue we can just use that in qxl_fence_wait() and simplify the code alot.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com Acked-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/qxl/qxl_release.c | 44 +++---------------------------- 1 file changed, 4 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 43a5436853b7..6ed673d75f9f 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -58,54 +58,18 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr, signed long timeout) { struct qxl_device *qdev; - struct qxl_release *release; - int count = 0, sc = 0; - bool have_drawable_releases; unsigned long cur, end = jiffies + timeout;
qdev = container_of(fence->lock, struct qxl_device, release_lock); - release = container_of(fence, struct qxl_release, base); - have_drawable_releases = release->type == QXL_RELEASE_DRAWABLE; - -retry: - sc++;
if (dma_fence_is_signaled(fence)) goto signaled;
qxl_io_notify_oom(qdev); - - for (count = 0; count < 11; count++) { - if (!qxl_queue_garbage_collect(qdev, true)) - break; - - if (dma_fence_is_signaled(fence)) - goto signaled; - } - - if (dma_fence_is_signaled(fence)) - goto signaled; - - if (have_drawable_releases || sc < 4) { - if (sc > 2) - /* back off */ - usleep_range(500, 1000); - - if (time_after(jiffies, end)) - return 0; - - if (have_drawable_releases && sc > 300) { - DMA_FENCE_WARN(fence, "failed to wait on release %llu " - "after spincount %d\n", - fence->context & ~0xf0000000, sc); - goto signaled; - } - goto retry; - } - /* - * yeah, original sync_obj_wait gave up after 3 spins when - * have_drawable_releases is not set. - */ + if (!wait_event_timeout(qdev->release_event, + dma_fence_is_signaled(fence), + timeout)) + return 0;
signaled: cur = jiffies;
dumb buffers are shadowed anyway, so there is no need to store them in device memory. Use QXL_GEM_DOMAIN_CPU (TTM_PL_SYSTEM) instead.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_dumb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c index c04cd5a2553c..48a58ba1db96 100644 --- a/drivers/gpu/drm/qxl/qxl_dumb.c +++ b/drivers/gpu/drm/qxl/qxl_dumb.c @@ -59,7 +59,7 @@ int qxl_mode_dumb_create(struct drm_file *file_priv, surf.stride = pitch; surf.format = format; r = qxl_gem_object_create_with_handle(qdev, file_priv, - QXL_GEM_DOMAIN_SURFACE, + QXL_GEM_DOMAIN_CPU, args->size, &surf, &qobj, &handle); if (r)
Am 04.02.21 um 15:57 schrieb Gerd Hoffmann:
dumb buffers are shadowed anyway, so there is no need to store them in device memory. Use QXL_GEM_DOMAIN_CPU (TTM_PL_SYSTEM) instead.
Makes sense. I had similar issues in other drivers about the placement of buffers. For them, all new buffers now go into system ram by default, and only move into device memory when they have to.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/qxl/qxl_dumb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c index c04cd5a2553c..48a58ba1db96 100644 --- a/drivers/gpu/drm/qxl/qxl_dumb.c +++ b/drivers/gpu/drm/qxl/qxl_dumb.c @@ -59,7 +59,7 @@ int qxl_mode_dumb_create(struct drm_file *file_priv, surf.stride = pitch; surf.format = format; r = qxl_gem_object_create_with_handle(qdev, file_priv,
QXL_GEM_DOMAIN_SURFACE,
QXL_GEM_DOMAIN_CPU, args->size, &surf, &qobj, &handle);
dri-devel@lists.freedesktop.org
-
Christian König -
Gerd Hoffmann -
Thomas Zimmermann -
Tong Zhang