[PATCH 01/10] drm/qxl: properly handle device init failures

[PATCH 01/10] drm/qxl: properly handle device init failures

[Gerd Hoffmann]

Specifically do not try release resources which where
not allocated in the first place.

Cc: Tong Zhang <ztong0001@gmail.com>
Tested-by: Tong Zhang <ztong0001@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 drivers/gpu/drm/qxl/qxl_kms.c     | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index c326412136c5..ec50d2cfd4e1 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
 {
 	int ret;
 
+	if (!qdev->monitors_config_bo)
+		return 0;
+
 	qdev->monitors_config = NULL;
 	qdev->ram_header->monitors_config = 0;
 
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 66d74aaaee06..4dc5ad13f12c 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev)
 {
 	int cur_idx;
 
+	/* check if qxl_device_init() was successful (gc_work is initialized last) */
+	if (!qdev->gc_work.func)
+		return;
+
 	for (cur_idx = 0; cur_idx < 3; cur_idx++) {
 		if (!qdev->current_release_bo[cur_idx])
 			continue;
-- 
2.29.2

Re: [PATCH 01/10] drm/qxl: properly handle device init failures

[Thomas Zimmermann]

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



Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Specifically do not try release resources which where
> not allocated in the first place.

I still think this should eventually be resolved by using managed code. 
But for now

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> 
> Cc: Tong Zhang <ztong0001@gmail.com>
> Tested-by: Tong Zhang <ztong0001@gmail.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/qxl/qxl_display.c | 3 +++
>   drivers/gpu/drm/qxl/qxl_kms.c     | 4 ++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index c326412136c5..ec50d2cfd4e1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
>   {
>   	int ret;
>   
> +	if (!qdev->monitors_config_bo)
> +		return 0;
> +
>   	qdev->monitors_config = NULL;
>   	qdev->ram_header->monitors_config = 0;
>   
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index 66d74aaaee06..4dc5ad13f12c 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev)
>   {
>   	int cur_idx;
>   
> +	/* check if qxl_device_init() was successful (gc_work is initialized last) */
> +	if (!qdev->gc_work.func)
> +		return;
> +
>   	for (cur_idx = 0; cur_idx < 3; cur_idx++) {
>   		if (!qdev->current_release_bo[cur_idx])
>   			continue;
> 

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

[PATCH 02/10] drm/qxl: more fence wait rework

[Gerd Hoffmann]

Move qxl_io_notify_oom() call into wait condition.
That way the driver will call it again if one call
wasn't enough.

Also allows to remove the extra dma_fence_is_signaled()
check and the goto.

Fixes: 5a838e5d5825 ("drm/qxl: simplify qxl_fence_wait")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 6ed673d75f9f..63818b56218c 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -62,16 +62,13 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr,
 
 	qdev = container_of(fence->lock, struct qxl_device, release_lock);
 
-	if (dma_fence_is_signaled(fence))
-		goto signaled;
-
-	qxl_io_notify_oom(qdev);
 	if (!wait_event_timeout(qdev->release_event,
-				dma_fence_is_signaled(fence),
+				(dma_fence_is_signaled(fence) || (
+					qxl_io_notify_oom(qdev),
+					0)),
 				timeout))
 		return 0;
 
-signaled:
 	cur = jiffies;
 	if (time_after(cur, end))
 		return 0;
-- 
2.29.2

[PATCH 03/10] drm/qxl: use ttm bo priorities

[Gerd Hoffmann]

Allow to set priorities for buffer objects.  Use priority 1 for surface
and cursor command releases.  Use priority 0 for drawing command
releases.  That way the short-living drawing commands are first in line
when it comes to eviction, making it *much* less likely that
ttm_bo_mem_force_space() picks something which can't be evicted and
throws an error after waiting a while without success.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_object.h  |  1 +
 drivers/gpu/drm/qxl/qxl_cmd.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_display.c |  4 ++--
 drivers/gpu/drm/qxl/qxl_gem.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c  |  5 +++--
 drivers/gpu/drm/qxl/qxl_release.c | 19 +++++++++++++------
 6 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index e60a8f88e226..dc1659e717f1 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -61,6 +61,7 @@ static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
 extern int qxl_bo_create(struct qxl_device *qdev,
 			 unsigned long size,
 			 bool kernel, bool pinned, u32 domain,
+			 u32 priority,
 			 struct qxl_surface *surf,
 			 struct qxl_bo **bo_ptr);
 extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 7e22a81bfb36..7b00c955cd82 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -269,7 +269,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev,
 	int ret;
 
 	ret = qxl_bo_create(qdev, size, false /* not kernel - device */,
-			    false, QXL_GEM_DOMAIN_VRAM, NULL, &bo);
+			    false, QXL_GEM_DOMAIN_VRAM, 0, NULL, &bo);
 	if (ret) {
 		DRM_ERROR("failed to allocate VRAM BO\n");
 		return ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ec50d2cfd4e1..a1b5cc5918bc 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -799,8 +799,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
 				qdev->dumb_shadow_bo = NULL;
 			}
 			qxl_bo_create(qdev, surf.height * surf.stride,
-				      true, true, QXL_GEM_DOMAIN_SURFACE, &surf,
-				      &qdev->dumb_shadow_bo);
+				      true, true, QXL_GEM_DOMAIN_SURFACE, 0,
+				      &surf, &qdev->dumb_shadow_bo);
 		}
 		if (user_bo->shadow != qdev->dumb_shadow_bo) {
 			if (user_bo->shadow) {
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 48e096285b4c..a08da0bd9098 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size,
 	/* At least align on page size */
 	if (alignment < PAGE_SIZE)
 		alignment = PAGE_SIZE;
-	r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf, &qbo);
+	r = qxl_bo_create(qdev, size, kernel, false, initial_domain, 0, surf, &qbo);
 	if (r) {
 		if (r != -ERESTARTSYS)
 			DRM_ERROR(
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 705b51535492..7eada4ad217b 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -103,8 +103,8 @@ static const struct drm_gem_object_funcs qxl_object_funcs = {
 	.print_info = drm_gem_ttm_print_info,
 };
 
-int qxl_bo_create(struct qxl_device *qdev,
-		  unsigned long size, bool kernel, bool pinned, u32 domain,
+int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
+		  bool kernel, bool pinned, u32 domain, u32 priority,
 		  struct qxl_surface *surf,
 		  struct qxl_bo **bo_ptr)
 {
@@ -137,6 +137,7 @@ int qxl_bo_create(struct qxl_device *qdev,
 
 	qxl_ttm_placement_from_domain(bo, domain);
 
+	bo->tbo.priority = priority;
 	r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type,
 				 &bo->placement, 0, &ctx, NULL, NULL,
 				 &qxl_ttm_bo_destroy);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 63818b56218c..a831184e014a 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -161,11 +161,12 @@ qxl_release_free(struct qxl_device *qdev,
 }
 
 static int qxl_release_bo_alloc(struct qxl_device *qdev,
-				struct qxl_bo **bo)
+				struct qxl_bo **bo,
+				u32 priority)
 {
 	/* pin releases bo's they are too messy to evict */
 	return qxl_bo_create(qdev, PAGE_SIZE, false, true,
-			     QXL_GEM_DOMAIN_VRAM, NULL, bo);
+			     QXL_GEM_DOMAIN_VRAM, priority, NULL, bo);
 }
 
 int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo)
@@ -288,13 +289,19 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 	int ret = 0;
 	union qxl_release_info *info;
 	int cur_idx;
+	u32 priority;
 
-	if (type == QXL_RELEASE_DRAWABLE)
+	if (type == QXL_RELEASE_DRAWABLE) {
 		cur_idx = 0;
-	else if (type == QXL_RELEASE_SURFACE_CMD)
+		priority = 0;
+	} else if (type == QXL_RELEASE_SURFACE_CMD) {
 		cur_idx = 1;
-	else if (type == QXL_RELEASE_CURSOR_CMD)
+		priority = 1;
+	}
+	else if (type == QXL_RELEASE_CURSOR_CMD) {
 		cur_idx = 2;
+		priority = 1;
+	}
 	else {
 		DRM_ERROR("got illegal type: %d\n", type);
 		return -EINVAL;
@@ -316,7 +323,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		qdev->current_release_bo[cur_idx] = NULL;
 	}
 	if (!qdev->current_release_bo[cur_idx]) {
-		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]);
+		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx], priority);
 		if (ret) {
 			mutex_unlock(&qdev->release_mutex);
 			qxl_release_free(qdev, *release);
-- 
2.29.2

[PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved

[Gerd Hoffmann]

Call qxl_bo_unpin (which does a reservation) without holding the
release_mutex lock.  Fixes lockdep (correctly) warning on a possible
deadlock.

Fixes: 65ffea3c6e73 ("drm/qxl: unpin release objects")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index a831184e014a..352a11a8485b 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -284,7 +284,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 				       int type, struct qxl_release **release,
 				       struct qxl_bo **rbo)
 {
-	struct qxl_bo *bo;
+	struct qxl_bo *bo, *free_bo = NULL;
 	int idr_ret;
 	int ret = 0;
 	union qxl_release_info *info;
@@ -317,8 +317,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]);
+		free_bo = qdev->current_release_bo[cur_idx];
 		qdev->current_release_bo_offset[cur_idx] = 0;
 		qdev->current_release_bo[cur_idx] = NULL;
 	}
@@ -326,6 +325,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx], priority);
 		if (ret) {
 			mutex_unlock(&qdev->release_mutex);
+			if (free_bo) {
+				qxl_bo_unpin(free_bo);
+				qxl_bo_unref(&free_bo);
+			}
 			qxl_release_free(qdev, *release);
 			return ret;
 		}
@@ -341,6 +344,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		*rbo = bo;
 
 	mutex_unlock(&qdev->release_mutex);
+	if (free_bo) {
+		qxl_bo_unpin(free_bo);
+		qxl_bo_unref(&free_bo);
+	}
 
 	ret = qxl_release_list_add(*release, bo);
 	qxl_bo_unref(&bo);
-- 
2.29.2

[PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked

[Gerd Hoffmann]

Make clear that these functions should be called with reserved
bo's only.  No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
 drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
 drivers/gpu/drm/qxl/qxl_draw.c    |  8 ++++----
 drivers/gpu/drm/qxl/qxl_image.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c  |  8 ++++----
 drivers/gpu/drm/qxl/qxl_prime.c   |  4 ++--
 6 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index dc1659e717f1..5bd33650183f 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev,
 			 u32 priority,
 			 struct qxl_surface *surf,
 			 struct qxl_bo **bo_ptr);
-extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
-extern void qxl_bo_kunmap(struct qxl_bo *bo);
+extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
+extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);
 void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
 void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
 extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo);
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index a1b5cc5918bc..8672385a1caf 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		user_bo = gem_to_qxl_bo(obj);
 
 		/* pinning is done in the prepare/cleanup framevbuffer */
-		ret = qxl_bo_kmap(user_bo, &user_map);
+		ret = qxl_bo_kmap_locked(user_bo, &user_map);
 		if (ret)
 			goto out_free_release;
 		user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		if (ret)
 			goto out_unpin;
 
-		ret = qxl_bo_kmap(cursor_bo, &cursor_map);
+		ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map);
 		if (ret)
 			goto out_backoff;
 		if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */
@@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		cursor->chunk.prev_chunk = 0;
 		cursor->chunk.data_size = size;
 		memcpy(cursor->chunk.data, user_ptr, size);
-		qxl_bo_kunmap(cursor_bo);
-		qxl_bo_kunmap(user_bo);
+		qxl_bo_kunmap_locked(cursor_bo);
+		qxl_bo_kunmap_locked(user_bo);
 
 		cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
 		cmd->u.set.visible = 1;
@@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 out_free_bo:
 	qxl_bo_unref(&cursor_bo);
 out_kunmap:
-	qxl_bo_kunmap(user_bo);
+	qxl_bo_kunmap_locked(user_bo);
 out_free_release:
 	qxl_release_free(qdev, release);
 	return;
@@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 	if (ret)
 		return ret;
 
-	qxl_bo_kmap(qdev->monitors_config_bo, &map);
+	qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
 
 	qdev->monitors_config = qdev->monitors_config_bo->kptr;
 	qdev->ram_header->monitors_config =
@@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
 	qdev->monitors_config = NULL;
 	qdev->ram_header->monitors_config = 0;
 
-	qxl_bo_kunmap(qdev->monitors_config_bo);
+	qxl_bo_kunmap_locked(qdev->monitors_config_bo);
 	ret = qxl_bo_unpin(qdev->monitors_config_bo);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 7b7acb910780..294542d49015 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev,
 	struct qxl_clip_rects *dev_clips;
 	int ret;
 
-	ret = qxl_bo_kmap(clips_bo, &map);
+	ret = qxl_bo_kmap_locked(clips_bo, &map);
 	if (ret)
 		return NULL;
 	dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 	if (ret)
 		goto out_release_backoff;
 
-	ret = qxl_bo_kmap(bo, &surface_map);
+	ret = qxl_bo_kmap_locked(bo, &surface_map);
 	if (ret)
 		goto out_release_backoff;
 	surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 	ret = qxl_image_init(qdev, release, dimage, surface_base,
 			     left - dumb_shadow_offset,
 			     top, width, height, depth, stride);
-	qxl_bo_kunmap(bo);
+	qxl_bo_kunmap_locked(bo);
 	if (ret)
 		goto out_release_backoff;
 
@@ -247,7 +247,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 		rects[i].top    = clips_ptr->y1;
 		rects[i].bottom = clips_ptr->y2;
 	}
-	qxl_bo_kunmap(clips_bo);
+	qxl_bo_kunmap_locked(clips_bo);
 
 	qxl_release_fence_buffer_objects(release);
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
diff --git a/drivers/gpu/drm/qxl/qxl_image.c b/drivers/gpu/drm/qxl/qxl_image.c
index 60ab7151b84d..d4db21ca1d5d 100644
--- a/drivers/gpu/drm/qxl/qxl_image.c
+++ b/drivers/gpu/drm/qxl/qxl_image.c
@@ -186,7 +186,7 @@ qxl_image_init_helper(struct qxl_device *qdev,
 			}
 		}
 	}
-	qxl_bo_kunmap(chunk_bo);
+	qxl_bo_kunmap_locked(chunk_bo);
 
 	image_bo = dimage->bo;
 	ptr = qxl_bo_kmap_atomic_page(qdev, image_bo, 0);
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 7eada4ad217b..b56d4dfc28cb 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -155,7 +155,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
 	return 0;
 }
 
-int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map)
+int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
 {
 	int r;
 
@@ -203,7 +203,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
 		return rptr;
 	}
 
-	ret = qxl_bo_kmap(bo, &bo_map);
+	ret = qxl_bo_kmap_locked(bo, &bo_map);
 	if (ret)
 		return NULL;
 	rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -212,7 +212,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
 	return rptr;
 }
 
-void qxl_bo_kunmap(struct qxl_bo *bo)
+void qxl_bo_kunmap_locked(struct qxl_bo *bo)
 {
 	if (bo->kptr == NULL)
 		return;
@@ -233,7 +233,7 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 	io_mapping_unmap_atomic(pmap);
 	return;
  fallback:
-	qxl_bo_kunmap(bo);
+	qxl_bo_kunmap_locked(bo);
 }
 
 void qxl_bo_unref(struct qxl_bo **bo)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index 4aa949799446..dc295b2e2b52 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 	int ret;
 
-	ret = qxl_bo_kmap(bo, map);
+	ret = qxl_bo_kmap_locked(bo, map);
 	if (ret < 0)
 		return ret;
 
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
 {
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 
-	qxl_bo_kunmap(bo);
+	qxl_bo_kunmap_locked(bo);
 }
 
 int qxl_gem_prime_mmap(struct drm_gem_object *obj,
-- 
2.29.2

Re: [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked

[Thomas Zimmermann]

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

Hi

Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Make clear that these functions should be called with reserved
> bo's only.  No functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
>   drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
>   drivers/gpu/drm/qxl/qxl_draw.c    |  8 ++++----
>   drivers/gpu/drm/qxl/qxl_image.c   |  2 +-
>   drivers/gpu/drm/qxl/qxl_object.c  |  8 ++++----
>   drivers/gpu/drm/qxl/qxl_prime.c   |  4 ++--
>   6 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
> index dc1659e717f1..5bd33650183f 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.h
> +++ b/drivers/gpu/drm/qxl/qxl_object.h
> @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev,
>   			 u32 priority,
>   			 struct qxl_surface *surf,
>   			 struct qxl_bo **bo_ptr);
> -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
> -extern void qxl_bo_kunmap(struct qxl_bo *bo);
> +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
> +extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);

Nowadaways kmap/kunmap is a misnomer. Since you're renaming; maybe 
rename them to vmap/vunmap.

Best regards
Thomas

>   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
>   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
>   extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo);
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index a1b5cc5918bc..8672385a1caf 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   		user_bo = gem_to_qxl_bo(obj);
>   
>   		/* pinning is done in the prepare/cleanup framevbuffer */
> -		ret = qxl_bo_kmap(user_bo, &user_map);
> +		ret = qxl_bo_kmap_locked(user_bo, &user_map);
>   		if (ret)
>   			goto out_free_release;
>   		user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
> @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   		if (ret)
>   			goto out_unpin;
>   
> -		ret = qxl_bo_kmap(cursor_bo, &cursor_map);
> +		ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map);
>   		if (ret)
>   			goto out_backoff;
>   		if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */
> @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   		cursor->chunk.prev_chunk = 0;
>   		cursor->chunk.data_size = size;
>   		memcpy(cursor->chunk.data, user_ptr, size);
> -		qxl_bo_kunmap(cursor_bo);
> -		qxl_bo_kunmap(user_bo);
> +		qxl_bo_kunmap_locked(cursor_bo);
> +		qxl_bo_kunmap_locked(user_bo);
>   
>   		cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
>   		cmd->u.set.visible = 1;
> @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   out_free_bo:
>   	qxl_bo_unref(&cursor_bo);
>   out_kunmap:
> -	qxl_bo_kunmap(user_bo);
> +	qxl_bo_kunmap_locked(user_bo);
>   out_free_release:
>   	qxl_release_free(qdev, release);
>   	return;
> @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>   	if (ret)
>   		return ret;
>   
> -	qxl_bo_kmap(qdev->monitors_config_bo, &map);
> +	qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
>   
>   	qdev->monitors_config = qdev->monitors_config_bo->kptr;
>   	qdev->ram_header->monitors_config =
> @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
>   	qdev->monitors_config = NULL;
>   	qdev->ram_header->monitors_config = 0;
>   
> -	qxl_bo_kunmap(qdev->monitors_config_bo);
> +	qxl_bo_kunmap_locked(qdev->monitors_config_bo);
>   	ret = qxl_bo_unpin(qdev->monitors_config_bo);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
> index 7b7acb910780..294542d49015 100644
> --- a/drivers/gpu/drm/qxl/qxl_draw.c
> +++ b/drivers/gpu/drm/qxl/qxl_draw.c
> @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev,
>   	struct qxl_clip_rects *dev_clips;
>   	int ret;
>   
> -	ret = qxl_bo_kmap(clips_bo, &map);
> +	ret = qxl_bo_kmap_locked(clips_bo, &map);
>   	if (ret)
>   		return NULL;
>   	dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */
> @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   	if (ret)
>   		goto out_release_backoff;
>   
> -	ret = qxl_bo_kmap(bo, &surface_map);
> +	ret = qxl_bo_kmap_locked(bo, &surface_map);
>   	if (ret)
>   		goto out_release_backoff;
>   	surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */
> @@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   	ret = qxl_image_init(qdev, release, dimage, surface_base,
>   			     left - dumb_shadow_offset,
>   			     top, width, height, depth, stride);
> -	qxl_bo_kunmap(bo);
> +	qxl_bo_kunmap_locked(bo);
>   	if (ret)
>   		goto out_release_backoff;
>   
> @@ -247,7 +247,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   		rects[i].top    = clips_ptr->y1;
>   		rects[i].bottom = clips_ptr->y2;
>   	}
> -	qxl_bo_kunmap(clips_bo);
> +	qxl_bo_kunmap_locked(clips_bo);
>   
>   	qxl_release_fence_buffer_objects(release);
>   	qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
> diff --git a/drivers/gpu/drm/qxl/qxl_image.c b/drivers/gpu/drm/qxl/qxl_image.c
> index 60ab7151b84d..d4db21ca1d5d 100644
> --- a/drivers/gpu/drm/qxl/qxl_image.c
> +++ b/drivers/gpu/drm/qxl/qxl_image.c
> @@ -186,7 +186,7 @@ qxl_image_init_helper(struct qxl_device *qdev,
>   			}
>   		}
>   	}
> -	qxl_bo_kunmap(chunk_bo);
> +	qxl_bo_kunmap_locked(chunk_bo);
>   
>   	image_bo = dimage->bo;
>   	ptr = qxl_bo_kmap_atomic_page(qdev, image_bo, 0);
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 7eada4ad217b..b56d4dfc28cb 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -155,7 +155,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
>   	return 0;
>   }
>   
> -int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map)
> +int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
>   {
>   	int r;
>   
> @@ -203,7 +203,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>   		return rptr;
>   	}
>   
> -	ret = qxl_bo_kmap(bo, &bo_map);
> +	ret = qxl_bo_kmap_locked(bo, &bo_map);
>   	if (ret)
>   		return NULL;
>   	rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */
> @@ -212,7 +212,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>   	return rptr;
>   }
>   
> -void qxl_bo_kunmap(struct qxl_bo *bo)
> +void qxl_bo_kunmap_locked(struct qxl_bo *bo)
>   {
>   	if (bo->kptr == NULL)
>   		return;
> @@ -233,7 +233,7 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
>   	io_mapping_unmap_atomic(pmap);
>   	return;
>    fallback:
> -	qxl_bo_kunmap(bo);
> +	qxl_bo_kunmap_locked(bo);
>   }
>   
>   void qxl_bo_unref(struct qxl_bo **bo)
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index 4aa949799446..dc295b2e2b52 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   	int ret;
>   
> -	ret = qxl_bo_kmap(bo, map);
> +	ret = qxl_bo_kmap_locked(bo, map);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
>   {
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   
> -	qxl_bo_kunmap(bo);
> +	qxl_bo_kunmap_locked(bo);
>   }
>   
>   int qxl_gem_prime_mmap(struct drm_gem_object *obj,
> 

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

[PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap

[Gerd Hoffmann]

Add kmap/kunmap variants which reserve (and pin) the bo.
They can be used in case the caller doesn't hold a reservation
for the bo.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_object.h |  2 ++
 drivers/gpu/drm/qxl/qxl_object.c | 36 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 5bd33650183f..360972ae4869 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
 			 u32 priority,
 			 struct qxl_surface *surf,
 			 struct qxl_bo **bo_ptr);
+extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
 extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
+extern int qxl_bo_kunmap(struct qxl_bo *bo);
 extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);
 void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
 void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b56d4dfc28cb..22748b9566af 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -29,6 +29,9 @@
 #include "qxl_drv.h"
 #include "qxl_object.h"
 
+static int __qxl_bo_pin(struct qxl_bo *bo);
+static void __qxl_bo_unpin(struct qxl_bo *bo);
+
 static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 {
 	struct qxl_bo *bo;
@@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
 	return 0;
 }
 
+int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map)
+{
+	int r;
+
+	r = qxl_bo_reserve(bo);
+	if (r)
+		return r;
+
+	r = __qxl_bo_pin(bo);
+	if (r) {
+		qxl_bo_unreserve(bo);
+		return r;
+	}
+
+	r = qxl_bo_kmap_locked(bo, map);
+	qxl_bo_unreserve(bo);
+	return r;
+}
+
 void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
 			      struct qxl_bo *bo, int page_offset)
 {
@@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo)
 	ttm_bo_vunmap(&bo->tbo, &bo->map);
 }
 
+int qxl_bo_kunmap(struct qxl_bo *bo)
+{
+	int r;
+
+	r = qxl_bo_reserve(bo);
+	if (r)
+		return r;
+
+	qxl_bo_kunmap_locked(bo);
+	__qxl_bo_unpin(bo);
+	qxl_bo_unreserve(bo);
+	return 0;
+}
+
 void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 			       struct qxl_bo *bo, void *pmap)
 {
-- 
2.29.2

Re: [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap

[Thomas Zimmermann]

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

Hi

Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Add kmap/kunmap variants which reserve (and pin) the bo.
> They can be used in case the caller doesn't hold a reservation
> for the bo.

Again, these functions should rather be called vmap/vunamp.

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/qxl/qxl_object.h |  2 ++
>   drivers/gpu/drm/qxl/qxl_object.c | 36 ++++++++++++++++++++++++++++++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
> index 5bd33650183f..360972ae4869 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.h
> +++ b/drivers/gpu/drm/qxl/qxl_object.h
> @@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
>   			 u32 priority,
>   			 struct qxl_surface *surf,
>   			 struct qxl_bo **bo_ptr);
> +extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
>   extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
> +extern int qxl_bo_kunmap(struct qxl_bo *bo);
>   extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);
>   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
>   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index b56d4dfc28cb..22748b9566af 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -29,6 +29,9 @@
>   #include "qxl_drv.h"
>   #include "qxl_object.h"
>   
> +static int __qxl_bo_pin(struct qxl_bo *bo);
> +static void __qxl_bo_unpin(struct qxl_bo *bo);
> +
>   static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
>   {
>   	struct qxl_bo *bo;
> @@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
>   	return 0;
>   }
>   
> +int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map)
> +{
> +	int r;
> +
> +	r = qxl_bo_reserve(bo);
> +	if (r)
> +		return r;
> +
> +	r = __qxl_bo_pin(bo);
> +	if (r) {
> +		qxl_bo_unreserve(bo);
> +		return r;
> +	}
> +
> +	r = qxl_bo_kmap_locked(bo, map);
> +	qxl_bo_unreserve(bo);
> +	return r;
> +}
> +
>   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>   			      struct qxl_bo *bo, int page_offset)
>   {
> @@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo)
>   	ttm_bo_vunmap(&bo->tbo, &bo->map);
>   }
>   
> +int qxl_bo_kunmap(struct qxl_bo *bo)
> +{
> +	int r;
> +
> +	r = qxl_bo_reserve(bo);
> +	if (r)
> +		return r;
> +
> +	qxl_bo_kunmap_locked(bo);
> +	__qxl_bo_unpin(bo);
> +	qxl_bo_unreserve(bo);
> +	return 0;
> +}
> +
>   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
>   			       struct qxl_bo *bo, void *pmap)
>   {
> 

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

[PATCH 07/10] drm/qxl: fix prime kmap

[Gerd Hoffmann]

Use the correct kmap variant.  We don't have a reservation here,
so we can't use the _locked version.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index dc295b2e2b52..4aa949799446 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 	int ret;
 
-	ret = qxl_bo_kmap_locked(bo, map);
+	ret = qxl_bo_kmap(bo, map);
 	if (ret < 0)
 		return ret;
 
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
 {
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 
-	qxl_bo_kunmap_locked(bo);
+	qxl_bo_kunmap(bo);
 }
 
 int qxl_gem_prime_mmap(struct drm_gem_object *obj,
-- 
2.29.2

Re: [PATCH 07/10] drm/qxl: fix prime kmap

[Thomas Zimmermann]

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

Hi

Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Use the correct kmap variant.  We don't have a reservation here,
> so we can't use the _locked version.

I'd merge this change into patch 6. So the new functions come with a caller.

Best regards
Thomas

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index dc295b2e2b52..4aa949799446 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   	int ret;
>   
> -	ret = qxl_bo_kmap_locked(bo, map);
> +	ret = qxl_bo_kmap(bo, map);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
>   {
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   
> -	qxl_bo_kunmap_locked(bo);
> +	qxl_bo_kunmap(bo);
>   }
>   
>   int qxl_gem_prime_mmap(struct drm_gem_object *obj,
> 

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

[PATCH 08/10] drm/qxl: fix monitors object kmap

[Gerd Hoffmann]

Use the correct kmap variant.  We don't hold a reservation here,
so we can't use the _locked variant.  We can drop the pin because
qxl_bo_kmap will do that for us.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 8672385a1caf..7500560db8e4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 	}
 	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
 
-	ret = qxl_bo_pin(qdev->monitors_config_bo);
+	ret = qxl_bo_kmap(qdev->monitors_config_bo, &map);
 	if (ret)
 		return ret;
 
-	qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
-
 	qdev->monitors_config = qdev->monitors_config_bo->kptr;
 	qdev->ram_header->monitors_config =
 		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
@@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
 	qdev->monitors_config = NULL;
 	qdev->ram_header->monitors_config = 0;
 
-	qxl_bo_kunmap_locked(qdev->monitors_config_bo);
-	ret = qxl_bo_unpin(qdev->monitors_config_bo);
+	ret = qxl_bo_kunmap(qdev->monitors_config_bo);
 	if (ret)
 		return ret;
 
-- 
2.29.2

Re: [PATCH 08/10] drm/qxl: fix monitors object kmap

[Thomas Zimmermann]

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



Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Use the correct kmap variant.  We don't hold a reservation here,
> so we can't use the _locked variant.  We can drop the pin because
> qxl_bo_kmap will do that for us.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/qxl/qxl_display.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 8672385a1caf..7500560db8e4 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>   	}
>   	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
>   
> -	ret = qxl_bo_pin(qdev->monitors_config_bo);
> +	ret = qxl_bo_kmap(qdev->monitors_config_bo, &map);
>   	if (ret)
>   		return ret;
>   
> -	qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
> -
>   	qdev->monitors_config = qdev->monitors_config_bo->kptr;
>   	qdev->ram_header->monitors_config =
>   		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
> @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
>   	qdev->monitors_config = NULL;
>   	qdev->ram_header->monitors_config = 0;
>   
> -	qxl_bo_kunmap_locked(qdev->monitors_config_bo);
> -	ret = qxl_bo_unpin(qdev->monitors_config_bo);
> +	ret = qxl_bo_kunmap(qdev->monitors_config_bo);
>   	if (ret)
>   		return ret;
>   
> 

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

[PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.

[Gerd Hoffmann]

We don't have to map in atomic_update callback then,
making locking a bit less complicated.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 7500560db8e4..39b8c5116d34 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 	struct drm_gem_object *obj;
 	struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL;
 	int ret;
-	struct dma_buf_map user_map;
 	struct dma_buf_map cursor_map;
 	void *user_ptr;
 	int size = 64*64*4;
@@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		obj = fb->obj[0];
 		user_bo = gem_to_qxl_bo(obj);
 
-		/* pinning is done in the prepare/cleanup framevbuffer */
-		ret = qxl_bo_kmap_locked(user_bo, &user_map);
-		if (ret)
-			goto out_free_release;
-		user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
+		/* mapping is done in the prepare/cleanup framevbuffer */
+		user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */
 
 		ret = qxl_alloc_bo_reserved(qdev, release,
 					    sizeof(struct qxl_cursor) + size,
@@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		cursor->chunk.data_size = size;
 		memcpy(cursor->chunk.data, user_ptr, size);
 		qxl_bo_kunmap_locked(cursor_bo);
-		qxl_bo_kunmap_locked(user_bo);
 
 		cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
 		cmd->u.set.visible = 1;
@@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
 	struct drm_gem_object *obj;
 	struct qxl_bo *user_bo;
 	struct qxl_surface surf;
+	struct dma_buf_map unused;
 
 	if (!new_state->fb)
 		return 0;
@@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
 		}
 	}
 
-	return qxl_bo_pin(user_bo);
+	return qxl_bo_kmap(user_bo, &unused);
 }
 
 static void qxl_plane_cleanup_fb(struct drm_plane *plane,
@@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
 
 	obj = old_state->fb->obj[0];
 	user_bo = gem_to_qxl_bo(obj);
-	qxl_bo_unpin(user_bo);
+	qxl_bo_kunmap(user_bo);
 
 	if (old_state->fb != plane->state->fb && user_bo->shadow) {
 		qxl_bo_unpin(user_bo->shadow);
-- 
2.29.2

Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.

[Thomas Zimmermann]

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

Hi

this is a shadow-buffered plane. Did you consider using the new helpers 
for shadow-buffered planes? They will map the user BO for you and 
provide the mapping in the plane state.

 From there, you should implement your own plane state on top of struct 
drm_shadow_plane_state, and also move all the other allocations and 
vmaps into prepare_fb and cleanup_fb. Most of this is not actually 
allowed in commit tails. All we'd have to do is to export the reset, 
duplicate and destroy code; similar to what 
__drm_atomic_helper_plane_reset() does.

Best regards
Thomas


Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> We don't have to map in atomic_update callback then,
> making locking a bit less complicated.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/qxl/qxl_display.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 7500560db8e4..39b8c5116d34 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   	struct drm_gem_object *obj;
>   	struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL;
>   	int ret;
> -	struct dma_buf_map user_map;
>   	struct dma_buf_map cursor_map;
>   	void *user_ptr;
>   	int size = 64*64*4;
> @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   		obj = fb->obj[0];
>   		user_bo = gem_to_qxl_bo(obj);
>   
> -		/* pinning is done in the prepare/cleanup framevbuffer */
> -		ret = qxl_bo_kmap_locked(user_bo, &user_map);
> -		if (ret)
> -			goto out_free_release;
> -		user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
> +		/* mapping is done in the prepare/cleanup framevbuffer */
> +		user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */
>   
>   		ret = qxl_alloc_bo_reserved(qdev, release,
>   					    sizeof(struct qxl_cursor) + size,
> @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
>   		cursor->chunk.data_size = size;
>   		memcpy(cursor->chunk.data, user_ptr, size);
>   		qxl_bo_kunmap_locked(cursor_bo);
> -		qxl_bo_kunmap_locked(user_bo);
>   
>   		cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
>   		cmd->u.set.visible = 1;
> @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
>   	struct drm_gem_object *obj;
>   	struct qxl_bo *user_bo;
>   	struct qxl_surface surf;
> +	struct dma_buf_map unused;
>   
>   	if (!new_state->fb)
>   		return 0;
> @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
>   		}
>   	}
>   
> -	return qxl_bo_pin(user_bo);
> +	return qxl_bo_kmap(user_bo, &unused);
>   }
>   
>   static void qxl_plane_cleanup_fb(struct drm_plane *plane,
> @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
>   
>   	obj = old_state->fb->obj[0];
>   	user_bo = gem_to_qxl_bo(obj);
> -	qxl_bo_unpin(user_bo);
> +	qxl_bo_kunmap(user_bo);
>   
>   	if (old_state->fb != plane->state->fb && user_bo->shadow) {
>   		qxl_bo_unpin(user_bo->shadow);
> 

-- 
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 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.

[Thomas Zimmermann]

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



Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
> Hi
> 
> this is a shadow-buffered plane. Did you consider using the new helpers 
> for shadow-buffered planes? They will map the user BO for you and 
> provide the mapping in the plane state.
> 
>  From there, you should implement your own plane state on top of struct 
> drm_shadow_plane_state, and also move all the other allocations and 
> vmaps into prepare_fb and cleanup_fb. Most of this is not actually 
> allowed in commit tails. All we'd have to do is to export the reset, 
> duplicate and destroy code; similar to what 
> __drm_atomic_helper_plane_reset() does.

AFAICT the cursor_bo is used to implement double buffering for the 
cursor image.

Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of 
the vram. Then pageflip between them in atomic_update(). Resolves all 
the allocation and mapping headaches.

Best regards
Thomas

> 
> Best regards
> Thomas
> 
> 
> Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
>> We don't have to map in atomic_update callback then,
>> making locking a bit less complicated.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   drivers/gpu/drm/qxl/qxl_display.c | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
>> b/drivers/gpu/drm/qxl/qxl_display.c
>> index 7500560db8e4..39b8c5116d34 100644
>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>> @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct 
>> drm_plane *plane,
>>       struct drm_gem_object *obj;
>>       struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo 
>> = NULL;
>>       int ret;
>> -    struct dma_buf_map user_map;
>>       struct dma_buf_map cursor_map;
>>       void *user_ptr;
>>       int size = 64*64*4;
>> @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct 
>> drm_plane *plane,
>>           obj = fb->obj[0];
>>           user_bo = gem_to_qxl_bo(obj);
>> -        /* pinning is done in the prepare/cleanup framevbuffer */
>> -        ret = qxl_bo_kmap_locked(user_bo, &user_map);
>> -        if (ret)
>> -            goto out_free_release;
>> -        user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction 
>> properly */
>> +        /* mapping is done in the prepare/cleanup framevbuffer */
>> +        user_ptr = user_bo->map.vaddr; /* TODO: Use mapping 
>> abstraction properly */
>>           ret = qxl_alloc_bo_reserved(qdev, release,
>>                           sizeof(struct qxl_cursor) + size,
>> @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct 
>> drm_plane *plane,
>>           cursor->chunk.data_size = size;
>>           memcpy(cursor->chunk.data, user_ptr, size);
>>           qxl_bo_kunmap_locked(cursor_bo);
>> -        qxl_bo_kunmap_locked(user_bo);
>>           cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
>>           cmd->u.set.visible = 1;
>> @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane 
>> *plane,
>>       struct drm_gem_object *obj;
>>       struct qxl_bo *user_bo;
>>       struct qxl_surface surf;
>> +    struct dma_buf_map unused;
>>       if (!new_state->fb)
>>           return 0;
>> @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane 
>> *plane,
>>           }
>>       }
>> -    return qxl_bo_pin(user_bo);
>> +    return qxl_bo_kmap(user_bo, &unused);
>>   }
>>   static void qxl_plane_cleanup_fb(struct drm_plane *plane,
>> @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane 
>> *plane,
>>       obj = old_state->fb->obj[0];
>>       user_bo = gem_to_qxl_bo(obj);
>> -    qxl_bo_unpin(user_bo);
>> +    qxl_bo_kunmap(user_bo);
>>       if (old_state->fb != plane->state->fb && user_bo->shadow) {
>>           qxl_bo_unpin(user_bo->shadow);
>>
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.

[Gerd Hoffmann]

On Tue, Feb 16, 2021 at 02:46:21PM +0100, Thomas Zimmermann wrote:
> 
> 
> Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
> > Hi
> > 
> > this is a shadow-buffered plane. Did you consider using the new helpers
> > for shadow-buffered planes? They will map the user BO for you and
> > provide the mapping in the plane state.
> > 
> >  From there, you should implement your own plane state on top of struct
> > drm_shadow_plane_state, and also move all the other allocations and
> > vmaps into prepare_fb and cleanup_fb. Most of this is not actually
> > allowed in commit tails. All we'd have to do is to export the reset,
> > duplicate and destroy code; similar to what
> > __drm_atomic_helper_plane_reset() does.
> 
> AFAICT the cursor_bo is used to implement double buffering for the cursor
> image.
> 
> Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the
> vram. Then pageflip between them in atomic_update(). Resolves all the
> allocation and mapping headaches.

Just waded through the ast patches.

It is not that simple for qxl.  You have to send a command to the
virtualization host and take care of the host accessing that memory
when processing the command, so you can't reuse the memory until the
host signals it is fine to do so.

But, yes, it should be possible to handle cursor_bo creation in
prepare_fb without too much effort.

take care,
  Gerd

Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.

[Thomas Zimmermann]

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

Hi

Am 17.02.21 um 11:02 schrieb Gerd Hoffmann:
> On Tue, Feb 16, 2021 at 02:46:21PM +0100, Thomas Zimmermann wrote:
>>
>>
>> Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> this is a shadow-buffered plane. Did you consider using the new helpers
>>> for shadow-buffered planes? They will map the user BO for you and
>>> provide the mapping in the plane state.
>>>
>>>   From there, you should implement your own plane state on top of struct
>>> drm_shadow_plane_state, and also move all the other allocations and
>>> vmaps into prepare_fb and cleanup_fb. Most of this is not actually
>>> allowed in commit tails. All we'd have to do is to export the reset,
>>> duplicate and destroy code; similar to what
>>> __drm_atomic_helper_plane_reset() does.
>>
>> AFAICT the cursor_bo is used to implement double buffering for the cursor
>> image.
>>
>> Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the
>> vram. Then pageflip between them in atomic_update(). Resolves all the
>> allocation and mapping headaches.
> 
> Just waded through the ast patches.

I just received your ack. Thanks a lot for looking at the ast patches.

> 
> It is not that simple for qxl.  You have to send a command to the
> virtualization host and take care of the host accessing that memory
> when processing the command, so you can't reuse the memory until the
> host signals it is fine to do so.
> 
> But, yes, it should be possible to handle cursor_bo creation in
> prepare_fb without too much effort.

I've been thinking about this issue and here's an idea:

If you take the ast code as a blueprint, you'd store two cursor bo in a 
cursor-plane structure. Aditionally each of these BOs would have a 
pointer to a fence associated with it.

One idea for the fencing code would be to allocate each new fence in 
prepare_fb and store it in the cursor plane state. In atomic_update, 
pick the unused BO in the cursor plane and wait on its fence. This 
should guarantee that the BO is available. (?) Then swap the BO's fence 
with the one in the cursor plane state. Setup the new fence for 
synchronization with the host. Next time you pick this cursor BO, the 
fence will be there for synchronization. The old fence from the cursor 
BO will now be stored in the cursor-plane state and can be freed in 
cleanup_fb().

My main interest here is to move all fail-able/locking calls out of the 
atomic_update function. I might be missing some crucial corner case, but 
this should resolve the issue. (?) In any case, it's maybe worth a 
separate patchset.

Best regards
Thomas

> 
> take care,
>    Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

[PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked

[Gerd Hoffmann]

Try avoid re-introducing locking bugs.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_object.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 22748b9566af..90d5e5b7f927 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
 {
 	int r;
 
+	dma_resv_assert_held(bo->tbo.base.resv);
+
 	if (bo->kptr) {
 		bo->map_count++;
 		goto out;
@@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
 
 void qxl_bo_kunmap_locked(struct qxl_bo *bo)
 {
+	dma_resv_assert_held(bo->tbo.base.resv);
+
 	if (bo->kptr == NULL)
 		return;
 	bo->map_count--;
-- 
2.29.2

Re: [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked

[Thomas Zimmermann]

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

Hi

Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
> Try avoid re-introducing locking bugs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

IMHO this should eventually be done in the rsp TTM functions. But so 
far, I'd expect this to break some drivers.

Best regards
Thomas

> ---
>   drivers/gpu/drm/qxl/qxl_object.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 22748b9566af..90d5e5b7f927 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map)
>   {
>   	int r;
>   
> +	dma_resv_assert_held(bo->tbo.base.resv);
> +
>   	if (bo->kptr) {
>   		bo->map_count++;
>   		goto out;
> @@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>   
>   void qxl_bo_kunmap_locked(struct qxl_bo *bo)
>   {
> +	dma_resv_assert_held(bo->tbo.base.resv);
> +
>   	if (bo->kptr == NULL)
>   		return;
>   	bo->map_count--;
> 

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