[PATCH v6 01/10] [hack] silence ttm fini WARNING

[PATCH v6 01/10] [hack] silence ttm fini WARNING

[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));
-- 
2.29.2

[PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

[Gerd Hoffmann]

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);
 }
-- 
2.29.2

[PATCH v6 03/10] drm/qxl: use drmm_mode_config_init

[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);
 }
-- 
2.29.2

[PATCH v6 04/10] drm/qxl: unpin release objects

[Gerd Hoffmann]

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;
-- 
2.29.2

[PATCH v6 05/10] drm/qxl: release shadow on shutdown

[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) {
+		drm_gem_object_put(&qdev->dumb_shadow_bo->tbo.base);
+		qdev->dumb_shadow_bo = NULL;
+	}
 	qxl_destroy_monitors_object(qdev);
 }
-- 
2.29.2

[PATCH v6 06/10] drm/qxl: properly pin/unpin shadow

[Gerd Hoffmann]

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;
 	}
-- 
2.29.2

[PATCH v6 07/10] drm/qxl: handle shadow in primary destroy

[Gerd Hoffmann]

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;
-- 
2.29.2

[PATCH v6 08/10] drm/qxl: properly free qxl releases

[Gerd Hoffmann]

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]) {
-- 
2.29.2

[PATCH v6 09/10] drm/qxl: simplify qxl_fence_wait

[Gerd Hoffmann]

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;
-- 
2.29.2

[PATCH v6 10/10] drm/qxl: allocate dumb buffers in ram

[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.

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)
-- 
2.29.2

Re: [PATCH v6 01/10] [hack] silence ttm fini WARNING

[Christian König]

?

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));

Re: [PATCH v6 05/10] drm/qxl: release shadow on shutdown

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1075 bytes --]



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);
>   }
> 

-- 
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 06/10] drm/qxl: properly pin/unpin shadow

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1867 bytes --]



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);
>   		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;
>   	}
> 

-- 
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 10/10] drm/qxl: allocate dumb buffers in ram

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1363 bytes --]



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);
>   	if (r)
> 

-- 
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1626 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

[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() -

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
> 

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2860 bytes --]

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
>>
> 

-- 
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

[Tong Zhang]

> 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

Re: [PATCH v6 01/10] [hack] silence ttm fini WARNING

[Gerd Hoffmann]

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