All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] drm/qxl: a collection of fixes
@ 2021-02-16 11:37 Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                   ` (9 more replies)
  0 siblings, 10 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel; +Cc: Gerd Hoffmann

Mostly around locking.

Gerd Hoffmann (10):
  drm/qxl: properly handle device init failures
  drm/qxl: more fence wait rework
  drm/qxl: use ttm bo priorities
  drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
  drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
  drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
  drm/qxl: fix prime kmap
  drm/qxl: fix monitors object kmap
  drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked

 drivers/gpu/drm/qxl/qxl_object.h  |  5 ++-
 drivers/gpu/drm/qxl/qxl_cmd.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 34 +++++++++-----------
 drivers/gpu/drm/qxl/qxl_draw.c    |  8 ++---
 drivers/gpu/drm/qxl/qxl_gem.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_image.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_kms.c     |  4 +++
 drivers/gpu/drm/qxl/qxl_object.c  | 53 +++++++++++++++++++++++++++----
 drivers/gpu/drm/qxl/qxl_release.c | 41 +++++++++++++++---------
 9 files changed, 103 insertions(+), 48 deletions(-)

-- 
2.29.2


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* [PATCH 01/10] drm/qxl: properly handle device init failures
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Tong Zhang, Dave Airlie, David Airlie,
	Daniel Vetter, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 01/10] drm/qxl: properly handle device init failures
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, Tong Zhang, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 01/10] drm/qxl: properly handle device init failures
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, Tong Zhang, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 02/10] drm/qxl: more fence wait rework
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	Thomas Zimmermann, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 02/10] drm/qxl: more fence wait rework
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Thomas Zimmermann, David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 02/10] drm/qxl: more fence wait rework
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Thomas Zimmermann, David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 03/10] drm/qxl: use ttm bo priorities
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 03/10] drm/qxl: use ttm bo priorities
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 03/10] drm/qxl: use ttm bo priorities
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	Thomas Zimmermann, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Thomas Zimmermann, David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Thomas Zimmermann, David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 07/10] drm/qxl: fix prime kmap
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 07/10] drm/qxl: fix prime kmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 07/10] drm/qxl: fix prime kmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 08/10] drm/qxl: fix monitors object kmap
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 08/10] drm/qxl: fix monitors object kmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 08/10] drm/qxl: fix monitors object kmap
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
  2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
@ 2021-02-16 11:37   ` Gerd Hoffmann
  2021-02-16 11:37   ` Gerd Hoffmann
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

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


^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Daniel Vetter,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
@ 2021-02-16 11:37   ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-16 11:37 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Gerd Hoffmann,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 61+ messages in thread

* Re: [PATCH 01/10] drm/qxl: properly handle device init failures
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:08     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:08 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, Tong Zhang, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 01/10] drm/qxl: properly handle device init failures
@ 2021-02-16 13:08     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:08 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, Tong Zhang, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 01/10] drm/qxl: properly handle device init failures
@ 2021-02-16 13:08     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:08 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, Tong Zhang, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:14     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:14 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
@ 2021-02-16 13:14     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:14 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
@ 2021-02-16 13:14     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:14 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 07/10] drm/qxl: fix prime kmap
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:16     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:16 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 07/10] drm/qxl: fix prime kmap
@ 2021-02-16 13:16     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:16 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 07/10] drm/qxl: fix prime kmap
@ 2021-02-16 13:16     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:16 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:17     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:17 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
@ 2021-02-16 13:17     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:17 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
@ 2021-02-16 13:17     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:17 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 08/10] drm/qxl: fix monitors object kmap
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:18     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:18 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 08/10] drm/qxl: fix monitors object kmap
@ 2021-02-16 13:18     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:18 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 08/10] drm/qxl: fix monitors object kmap
@ 2021-02-16 13:18     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:18 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:27     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:27 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 13:27     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:27 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 13:27     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:27 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
  2021-02-16 11:37   ` Gerd Hoffmann
  (?)
@ 2021-02-16 13:30     ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:30 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
@ 2021-02-16 13:30     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:30 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
@ 2021-02-16 13:30     ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:30 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  2021-02-16 13:27     ` Thomas Zimmermann
  (?)
@ 2021-02-16 13:46       ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:46 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	Dave Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 13:46       ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:46 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-16 13:46       ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-16 13:46 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: David Airlie, open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  2021-02-16 13:46       ` Thomas Zimmermann
  (?)
@ 2021-02-17 10:02         ` Gerd Hoffmann
  -1 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-17 10:02 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: dri-devel, David Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU

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


^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-17 10:02         ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-17 10:02 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-17 10:02         ` Gerd Hoffmann
  0 siblings, 0 replies; 61+ messages in thread
From: Gerd Hoffmann @ 2021-02-17 10:02 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
  2021-02-17 10:02         ` Gerd Hoffmann
  (?)
@ 2021-02-17 10:23           ` Thomas Zimmermann
  -1 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-17 10:23 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU


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

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-17 10:23           ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-17 10:23 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 61+ messages in thread

* Re: [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
@ 2021-02-17 10:23           ` Thomas Zimmermann
  0 siblings, 0 replies; 61+ messages in thread
From: Thomas Zimmermann @ 2021-02-17 10:23 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie


[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 61+ messages in thread

end of thread, other threads:[~2021-02-17 10:24 UTC | newest]

Thread overview: 61+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16 11:37 [PATCH 00/10] drm/qxl: a collection of fixes Gerd Hoffmann
2021-02-16 11:37 ` [PATCH 01/10] drm/qxl: properly handle device init failures Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:08   ` Thomas Zimmermann
2021-02-16 13:08     ` Thomas Zimmermann
2021-02-16 13:08     ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 02/10] drm/qxl: more fence wait rework Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37 ` [PATCH 03/10] drm/qxl: use ttm bo priorities Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37 ` [PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37 ` [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:14   ` Thomas Zimmermann
2021-02-16 13:14     ` Thomas Zimmermann
2021-02-16 13:14     ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:17   ` Thomas Zimmermann
2021-02-16 13:17     ` Thomas Zimmermann
2021-02-16 13:17     ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 07/10] drm/qxl: fix prime kmap Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:16   ` Thomas Zimmermann
2021-02-16 13:16     ` Thomas Zimmermann
2021-02-16 13:16     ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 08/10] drm/qxl: fix monitors object kmap Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:18   ` Thomas Zimmermann
2021-02-16 13:18     ` Thomas Zimmermann
2021-02-16 13:18     ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:27   ` Thomas Zimmermann
2021-02-16 13:27     ` Thomas Zimmermann
2021-02-16 13:27     ` Thomas Zimmermann
2021-02-16 13:46     ` Thomas Zimmermann
2021-02-16 13:46       ` Thomas Zimmermann
2021-02-16 13:46       ` Thomas Zimmermann
2021-02-17 10:02       ` Gerd Hoffmann
2021-02-17 10:02         ` Gerd Hoffmann
2021-02-17 10:02         ` Gerd Hoffmann
2021-02-17 10:23         ` Thomas Zimmermann
2021-02-17 10:23           ` Thomas Zimmermann
2021-02-17 10:23           ` Thomas Zimmermann
2021-02-16 11:37 ` [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 11:37   ` Gerd Hoffmann
2021-02-16 13:30   ` Thomas Zimmermann
2021-02-16 13:30     ` Thomas Zimmermann
2021-02-16 13:30     ` Thomas Zimmermann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.