[PATCH 0/2] Add BO reservation to GEM VRAM pin/unpin/push_to_system

[PATCH 0/2] Add BO reservation to GEM VRAM pin/unpin/push_to_system

[Thomas Zimmermann]

A kernel test bot reported a problem with the locktorture testcase that
was triggered by the GEM VRAM helpers.

  ...
  [   10.004734] RIP: 0010:ttm_bo_validate+0x41/0x141 [ttm]
  ...
  [   10.015669]  ? kvm_sched_clock_read+0x5/0xd
  [   10.016157]  ? get_lock_stats+0x11/0x3f
  [   10.016607]  drm_gem_vram_pin+0x77/0xa2 [drm_vram_helper]
  [   10.017229]  drm_gem_vram_driver_gem_prime_vmap+0xe/0x39 [drm_vram_helper]
  [   10.018015]  drm_gem_vmap+0x36/0x43 [drm]
  [   10.018491]  drm_client_framebuffer_create+0xc6/0x1ca [drm]
  [   10.019143]  drm_fb_helper_generic_probe+0x4c/0x157 [drm_kms_helper]
  [   10.019864]  __drm_fb_helper_initial_config_and_unlock+0x307/0x442 [drm_kms_helper]
  [   10.020739]  drm_fbdev_client_hotplug+0xc8/0x115 [drm_kms_helper]
  [   10.021442]  drm_fbdev_generic_setup+0xc4/0xf1 [drm_kms_helper]
  [   10.022120]  bochs_pci_probe+0x123/0x143 [bochs_drm]
  [   10.022688]  local_pci_probe+0x34/0x75
  [   10.023127]  pci_device_probe+0xf8/0x150A
  ...

It turns out that the bochs and vbox drivers automatically reserved and
unreserved the BO from within their pin and unpin functions. The other
drivers; ast, hibmc and mgag200; performed reservation explicitly. With the
GEM VRAM conversion, automatic BO reservation within pin and unpin functions
accidentally got lost. So for bochs and vbox, ttm_bo_validate() worked on
unlocked BOs.

This patch set fixes the problem by adding automatic reservation to the
implementation of drm_gem_vram_{pin,unpin,push_to_system}() to fix bochs
and vbox. It removes explicit BO reservation around the pin, unpin and
push-to-system calls in the ast, hibmc and mgag200 drivers.

The only exception is the cursor handling of mgag200. In this case, the
mgag200 driver now calls drm_gem_vram_{pin,unpin}_reserved(), which works
with reserved BOs. The respective code should be refactored in a future
patch to work with the regular pin and unpin functions.

Thomas Zimmermann (2):
  drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
  drm: Reserve/unreserve GEM VRAM BOs from within pin/unpin functions

 drivers/gpu/drm/ast/ast_mode.c                |  24 +---
 drivers/gpu/drm/drm_gem_vram_helper.c         | 115 +++++++++++++++++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |   6 -
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c |  17 +--
 drivers/gpu/drm/mgag200/mgag200_cursor.c      |  18 +--
 drivers/gpu/drm/mgag200/mgag200_mode.c        |  19 +--
 include/drm/drm_gem_vram_helper.h             |   3 +
 7 files changed, 126 insertions(+), 76 deletions(-)

--
2.21.0

[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Thomas Zimmermann]

The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
GEM VRAM pin/unpin functions that do not reserve the BO during validation.
The mgag200 driver requires this behavior for its cursor handling. The
patch also converts the driver to use the new interfaces.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c    | 75 ++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
 include/drm/drm_gem_vram_helper.h        |  3 +
 3 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 8f142b810eb4..a002c03eaf4c 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 }
 EXPORT_SYMBOL(drm_gem_vram_pin);
 
+/**
+ * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
+ * @gbo:	the GEM VRAM object
+ * @pl_flag:	a bitmask of possible memory regions
+ *
+ * Pinning a buffer object ensures that it is not evicted from
+ * a memory region. A pinned buffer object has to be unpinned before
+ * it can be pinned to another region.
+ *
+ * This function pins a GEM VRAM object that has already been
+ * reserved. Use drm_gem_vram_pin() if possible.
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise.
+ */
+int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
+			      unsigned long pl_flag)
+{
+	int i, ret;
+	struct ttm_operation_ctx ctx = { false, false };
+
+	if (gbo->pin_count) {
+		++gbo->pin_count;
+		return 0;
+	}
+
+	drm_gem_vram_placement(gbo, pl_flag);
+	for (i = 0; i < gbo->placement.num_placement; ++i)
+		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
+
+	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
+	if (ret < 0)
+		return ret;
+
+	gbo->pin_count = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
+
 /**
  * drm_gem_vram_unpin() - Unpins a GEM VRAM object
  * @gbo:	the GEM VRAM object
@@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
+/**
+ * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
+ * @gbo:	the GEM VRAM object
+ *
+ * This function unpins a GEM VRAM object that has already been
+ * reserved. Use drm_gem_vram_unpin() if possible.
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise.
+ */
+int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
+{
+	int i, ret;
+	struct ttm_operation_ctx ctx = { false, false };
+
+	if (WARN_ON_ONCE(!gbo->pin_count))
+		return 0;
+
+	--gbo->pin_count;
+	if (gbo->pin_count)
+		return 0;
+
+	for (i = 0; i < gbo->placement.num_placement ; ++i)
+		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
+
+	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
+
 /**
  * drm_gem_vram_push_to_system() - \
 	Unpins a GEM VRAM object and moves it to system memory
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index 6c1a9d724d85..1c4fc85315a0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
 	WREG8(MGA_CURPOSXL, 0);
 	WREG8(MGA_CURPOSXH, 0);
 	if (mdev->cursor.pixels_1->pin_count)
-		drm_gem_vram_unpin(mdev->cursor.pixels_1);
+		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
 	if (mdev->cursor.pixels_2->pin_count)
-		drm_gem_vram_unpin(mdev->cursor.pixels_2);
+		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
 }
 
 int mga_crtc_cursor_set(struct drm_crtc *crtc,
@@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 
 	/* Move cursor buffers into VRAM if they aren't already */
 	if (!pixels_1->pin_count) {
-		ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
+		ret = drm_gem_vram_pin_reserved(pixels_1,
+						DRM_GEM_VRAM_PL_FLAG_VRAM);
 		if (ret)
 			goto out1;
 		gpu_addr = drm_gem_vram_offset(pixels_1);
 		if (gpu_addr < 0) {
-			drm_gem_vram_unpin(pixels_1);
+			drm_gem_vram_unpin_reserved(pixels_1);
 			goto out1;
 		}
 		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
 	}
 	if (!pixels_2->pin_count) {
-		ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
+		ret = drm_gem_vram_pin_reserved(pixels_2,
+						DRM_GEM_VRAM_PL_FLAG_VRAM);
 		if (ret) {
-			drm_gem_vram_unpin(pixels_1);
+			drm_gem_vram_unpin_reserved(pixels_1);
 			goto out1;
 		}
 		gpu_addr = drm_gem_vram_offset(pixels_2);
 		if (gpu_addr < 0) {
-			drm_gem_vram_unpin(pixels_1);
-			drm_gem_vram_unpin(pixels_2);
+			drm_gem_vram_unpin_reserved(pixels_1);
+			drm_gem_vram_unpin_reserved(pixels_2);
 			goto out1;
 		}
 		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index b056f189ba62..ff1a81761543 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
 u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
 s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
 int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
+int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
+			      unsigned long pl_flag);
 int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
 int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
 void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
 			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
-- 
2.21.0

[PATCH 2/2] drm: Reserve/unreserve GEM VRAM BOs from within pin/unpin functions

[Thomas Zimmermann]

The original bochs and vbox implementations of pin and unpin functions
automatically reserved BOs during validation. This functionality got lost
while converting the code to a generic implementation. This may result
in validating unlocked TTM BOs.

Adding the reserve and unreserve operations to GEM VRAM's pin and unpin
functions fixes the bochs and vbox drivers. Additionally the patch changes
the mgag200, ast and hibmc drivers to not reserve BOs by themselves.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: a3232987fdbf0bede92a9d7c7e2db99a5084d31b ("drm/bochs: Convert [...]")
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/ast/ast_mode.c                | 24 +--------
 drivers/gpu/drm/drm_gem_vram_helper.c         | 54 ++++++++++++++-----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |  6 ---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 17 +-----
 drivers/gpu/drm/mgag200/mgag200_mode.c        | 19 +------
 5 files changed, 45 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 3475591a22c3..9aca9135a5cc 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -539,24 +539,16 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 		ast_fb = to_ast_framebuffer(fb);
 		obj = ast_fb->obj;
 		gbo = drm_gem_vram_of_gem(obj);
-		ret = drm_gem_vram_reserve(gbo, false);
-		if (ret)
-			return ret;
 		drm_gem_vram_push_to_system(gbo);
-		drm_gem_vram_unreserve(gbo);
 	}
 
 	ast_fb = to_ast_framebuffer(crtc->primary->fb);
 	obj = ast_fb->obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	ret = drm_gem_vram_reserve(gbo, false);
-	if (ret)
-		return ret;
-
 	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
 	if (ret)
-		goto err_drm_gem_vram_unreserve;
+		return ret;
 	gpu_addr = drm_gem_vram_offset(gbo);
 	if (gpu_addr < 0) {
 		ret = (int)gpu_addr;
@@ -573,7 +565,6 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 			ast_fbdev_set_base(ast, gpu_addr);
 		}
 	}
-	drm_gem_vram_unreserve(gbo);
 
 	ast_set_offset_reg(crtc);
 	ast_set_start_address_crt1(crtc, (u32)gpu_addr);
@@ -582,8 +573,6 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 
 err_drm_gem_vram_unpin:
 	drm_gem_vram_unpin(gbo);
-err_drm_gem_vram_unreserve:
-	drm_gem_vram_unreserve(gbo);
 	return ret;
 }
 
@@ -630,8 +619,6 @@ static int ast_crtc_mode_set(struct drm_crtc *crtc,
 
 static void ast_crtc_disable(struct drm_crtc *crtc)
 {
-	int ret;
-
 	DRM_DEBUG_KMS("\n");
 	ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
@@ -639,11 +626,7 @@ static void ast_crtc_disable(struct drm_crtc *crtc)
 		struct drm_gem_object *obj = ast_fb->obj;
 		struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj);
 
-		ret = drm_gem_vram_reserve(gbo, false);
-		if (ret)
-			return;
 		drm_gem_vram_push_to_system(gbo);
-		drm_gem_vram_unreserve(gbo);
 	}
 	crtc->primary->fb = NULL;
 }
@@ -939,12 +922,7 @@ static int ast_cursor_init(struct drm_device *dev)
 	if (ret)
 		return ret;
 	gbo = drm_gem_vram_of_gem(obj);
-	ret = drm_gem_vram_reserve(gbo, false);
-	if (unlikely(ret != 0))
-		goto fail;
-
 	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
-	drm_gem_vram_unreserve(gbo);
 	if (ret)
 		goto fail;
 	gpu_addr = drm_gem_vram_offset(gbo);
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index a002c03eaf4c..bde8237e8021 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -235,10 +235,12 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
 
-	if (gbo->pin_count) {
-		++gbo->pin_count;
-		return 0;
-	}
+	ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
+	if (ret < 0)
+		return ret;
+
+	if (gbo->pin_count)
+		goto out;
 
 	drm_gem_vram_placement(gbo, pl_flag);
 	for (i = 0; i < gbo->placement.num_placement; ++i)
@@ -246,11 +248,17 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 
 	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
 	if (ret < 0)
-		return ret;
+		goto err_ttm_bo_unreserve;
 
-	gbo->pin_count = 1;
+out:
+	++gbo->pin_count;
+	ttm_bo_unreserve(&gbo->bo);
 
 	return 0;
+
+err_ttm_bo_unreserve:
+	ttm_bo_unreserve(&gbo->bo);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_vram_pin);
 
@@ -308,21 +316,32 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
 
+	ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
+	if (ret < 0)
+		return ret;
+
 	if (WARN_ON_ONCE(!gbo->pin_count))
-		return 0;
+		goto out;
 
 	--gbo->pin_count;
 	if (gbo->pin_count)
-		return 0;
+		goto out;
 
 	for (i = 0; i < gbo->placement.num_placement ; ++i)
 		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
 
 	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
 	if (ret < 0)
-		return ret;
+		goto err_ttm_bo_unreserve;
+
+out:
+	ttm_bo_unreserve(&gbo->bo);
 
 	return 0;
+
+err_ttm_bo_unreserve:
+	ttm_bo_unreserve(&gbo->bo);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
@@ -377,12 +396,16 @@ int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
 
+	ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
+	if (ret < 0)
+		return ret;
+
 	if (WARN_ON_ONCE(!gbo->pin_count))
-		return 0;
+		goto out;
 
 	--gbo->pin_count;
 	if (gbo->pin_count)
-		return 0;
+		goto out;
 
 	if (gbo->kmap.virtual)
 		ttm_bo_kunmap(&gbo->kmap);
@@ -393,9 +416,16 @@ int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
 
 	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
 	if (ret)
-		return ret;
+		goto err_ttm_bo_unreserve;
+
+out:
+	ttm_bo_unreserve(&gbo->bo);
 
 	return 0;
+
+err_ttm_bo_unreserve:
+	ttm_bo_unreserve(&gbo->bo);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_vram_push_to_system);
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index db0dfa57844e..fbdf495779e0 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -107,14 +107,8 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 
 	hibmc_fb = to_hibmc_framebuffer(state->fb);
 	gbo = drm_gem_vram_of_gem(hibmc_fb->obj);
-	ret = drm_gem_vram_reserve(gbo, false);
-	if (ret) {
-		DRM_ERROR("failed to reserve BO: %d", ret);
-		return;
-	}
 
 	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
-	drm_gem_vram_unreserve(gbo);
 	if (ret) {
 		DRM_ERROR("failed to pin bo: %d", ret);
 		return;
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
index 9d2025fa16f8..bd5fbb23973a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
@@ -63,7 +63,6 @@ static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
 	struct drm_mode_fb_cmd2 mode_cmd;
 	struct drm_gem_object *gobj = NULL;
 	int ret = 0;
-	int ret1;
 	size_t size;
 	unsigned int bytes_per_pixel;
 	struct drm_gem_vram_object *gbo = NULL;
@@ -91,16 +90,10 @@ static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
 
 	gbo = drm_gem_vram_of_gem(gobj);
 
-	ret = drm_gem_vram_reserve(gbo, false);
-	if (ret) {
-		DRM_ERROR("failed to reserve bo: %d\n", ret);
-		goto out_unref_gem;
-	}
-
 	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
 	if (ret) {
 		DRM_ERROR("failed to pin fbcon: %d\n", ret);
-		goto out_unreserve_ttm_bo;
+		goto out_unref_gem;
 	}
 
 	base = drm_gem_vram_kmap(gbo, true, NULL);
@@ -109,7 +102,6 @@ static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
 		DRM_ERROR("failed to kmap fbcon: %d\n", ret);
 		goto out_unpin_bo;
 	}
-	drm_gem_vram_unreserve(gbo);
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
@@ -141,16 +133,9 @@ static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
 	return 0;
 
 out_release_fbi:
-	ret1 = drm_gem_vram_reserve(gbo, false);
-	if (ret1) {
-		DRM_ERROR("failed to rsv ttm_bo when release fbi: %d\n", ret1);
-		goto out_unref_gem;
-	}
 	drm_gem_vram_kunmap(gbo);
 out_unpin_bo:
 	drm_gem_vram_unpin(gbo);
-out_unreserve_ttm_bo:
-	drm_gem_vram_unreserve(gbo);
 out_unref_gem:
 	drm_gem_object_put_unlocked(gobj);
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3098bf5c1744..e79872c968bf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -877,24 +877,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 		mga_fb = to_mga_framebuffer(fb);
 		obj = mga_fb->obj;
 		gbo = drm_gem_vram_of_gem(obj);
-		ret = drm_gem_vram_reserve(gbo, false);
-		if (ret)
-			return ret;
 		drm_gem_vram_push_to_system(gbo);
-		drm_gem_vram_unreserve(gbo);
 	}
 
 	mga_fb = to_mga_framebuffer(crtc->primary->fb);
 	obj = mga_fb->obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	ret = drm_gem_vram_reserve(gbo, false);
-	if (ret)
-		return ret;
-
 	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
 	if (ret)
-		goto err_drm_gem_vram_unreserve;
+		return ret;
 	gpu_addr = drm_gem_vram_offset(gbo);
 	if (gpu_addr < 0) {
 		ret = (int)gpu_addr;
@@ -910,16 +902,12 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 		}
 	}
 
-	drm_gem_vram_unreserve(gbo);
-
 	mga_set_start_address(crtc, (u32)gpu_addr);
 
 	return 0;
 
 err_drm_gem_vram_unpin:
 	drm_gem_vram_unpin(gbo);
-err_drm_gem_vram_unreserve:
-	drm_gem_vram_unreserve(gbo);
 	return ret;
 }
 
@@ -1434,7 +1422,6 @@ static void mga_crtc_destroy(struct drm_crtc *crtc)
 
 static void mga_crtc_disable(struct drm_crtc *crtc)
 {
-	int ret;
 	DRM_DEBUG_KMS("\n");
 	mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
@@ -1442,11 +1429,7 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
 		struct drm_gem_object *obj = mga_fb->obj;
 		struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj);
 
-		ret = drm_gem_vram_reserve(gbo, false);
-		if (ret)
-			return;
 		drm_gem_vram_push_to_system(gbo);
-		drm_gem_vram_unreserve(gbo);
 	}
 	crtc->primary->fb = NULL;
 }
-- 
2.21.0

Re: [PATCH 0/2] Add BO reservation to GEM VRAM pin/unpin/push_to_system

[Gerd Hoffmann]

  Hi,

> It turns out that the bochs and vbox drivers automatically reserved and
> unreserved the BO from within their pin and unpin functions. The other
> drivers; ast, hibmc and mgag200; performed reservation explicitly. With the
> GEM VRAM conversion, automatic BO reservation within pin and unpin functions
> accidentally got lost. So for bochs and vbox, ttm_bo_validate() worked on
> unlocked BOs.
> 
> This patch set fixes the problem by adding automatic reservation to the
> implementation of drm_gem_vram_{pin,unpin,push_to_system}() to fix bochs
> and vbox. It removes explicit BO reservation around the pin, unpin and
> push-to-system calls in the ast, hibmc and mgag200 drivers.
> 
> The only exception is the cursor handling of mgag200. In this case, the
> mgag200 driver now calls drm_gem_vram_{pin,unpin}_reserved(), which works
> with reserved BOs. The respective code should be refactored in a future
> patch to work with the regular pin and unpin functions.

Looks good, pushed to drm-misc-next.

thanks,
  Gerd

Re: [PATCH 0/2] Add BO reservation to GEM VRAM pin/unpin/push_to_system

[Daniel Vetter]

On Fri, May 17, 2019 at 01:17:03PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > It turns out that the bochs and vbox drivers automatically reserved and
> > unreserved the BO from within their pin and unpin functions. The other
> > drivers; ast, hibmc and mgag200; performed reservation explicitly. With the
> > GEM VRAM conversion, automatic BO reservation within pin and unpin functions
> > accidentally got lost. So for bochs and vbox, ttm_bo_validate() worked on
> > unlocked BOs.
> > 
> > This patch set fixes the problem by adding automatic reservation to the
> > implementation of drm_gem_vram_{pin,unpin,push_to_system}() to fix bochs
> > and vbox. It removes explicit BO reservation around the pin, unpin and
> > push-to-system calls in the ast, hibmc and mgag200 drivers.
> > 
> > The only exception is the cursor handling of mgag200. In this case, the
> > mgag200 driver now calls drm_gem_vram_{pin,unpin}_reserved(), which works
> > with reserved BOs. The respective code should be refactored in a future
> > patch to work with the regular pin and unpin functions.
> 
> Looks good, pushed to drm-misc-next.

I have a bit of design review (replied to patch 1), would be great if
either of you could address that as a follow up.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Daniel Vetter]

On Thu, May 16, 2019 at 06:27:45PM +0200, Thomas Zimmermann wrote:
> The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
> GEM VRAM pin/unpin functions that do not reserve the BO during validation.
> The mgag200 driver requires this behavior for its cursor handling. The
> patch also converts the driver to use the new interfaces.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 75 ++++++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
>  include/drm/drm_gem_vram_helper.h        |  3 +
>  3 files changed, 88 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 8f142b810eb4..a002c03eaf4c 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_pin);
>  
> +/**
> + * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
> + * @gbo:	the GEM VRAM object
> + * @pl_flag:	a bitmask of possible memory regions
> + *
> + * Pinning a buffer object ensures that it is not evicted from
> + * a memory region. A pinned buffer object has to be unpinned before
> + * it can be pinned to another region.
> + *
> + * This function pins a GEM VRAM object that has already been
> + * reserved. Use drm_gem_vram_pin() if possible.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> +			      unsigned long pl_flag)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };

I think would be good to have a lockdep_assert_held here for the ww_mutex.

Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
we call the structure tracking the fences+lock the "reservation", but the
naming scheme used is _lock/_unlock.

I think would be good to be consistent with that, and use _locked here.
Especially for a very simplified vram helper like this one I expect that's
going to lead to less wtf moments by driver writers :-)

Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
more with what we now have in dma-buf.

Cheers, Daniel

> +
> +	if (gbo->pin_count) {
> +		++gbo->pin_count;
> +		return 0;
> +	}
> +
> +	drm_gem_vram_placement(gbo, pl_flag);
> +	for (i = 0; i < gbo->placement.num_placement; ++i)
> +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> +
> +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	gbo->pin_count = 1;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
> +
>  /**
>   * drm_gem_vram_unpin() - Unpins a GEM VRAM object
>   * @gbo:	the GEM VRAM object
> @@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_unpin);
>  
> +/**
> + * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
> + * @gbo:	the GEM VRAM object
> + *
> + * This function unpins a GEM VRAM object that has already been
> + * reserved. Use drm_gem_vram_unpin() if possible.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (WARN_ON_ONCE(!gbo->pin_count))
> +		return 0;
> +
> +	--gbo->pin_count;
> +	if (gbo->pin_count)
> +		return 0;
> +
> +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> +		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> +
> +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
> +
>  /**
>   * drm_gem_vram_push_to_system() - \
>  	Unpins a GEM VRAM object and moves it to system memory
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index 6c1a9d724d85..1c4fc85315a0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
>  	WREG8(MGA_CURPOSXL, 0);
>  	WREG8(MGA_CURPOSXH, 0);
>  	if (mdev->cursor.pixels_1->pin_count)
> -		drm_gem_vram_unpin(mdev->cursor.pixels_1);
> +		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
>  	if (mdev->cursor.pixels_2->pin_count)
> -		drm_gem_vram_unpin(mdev->cursor.pixels_2);
> +		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
>  }
>  
>  int mga_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	/* Move cursor buffers into VRAM if they aren't already */
>  	if (!pixels_1->pin_count) {
> -		ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
> +		ret = drm_gem_vram_pin_reserved(pixels_1,
> +						DRM_GEM_VRAM_PL_FLAG_VRAM);
>  		if (ret)
>  			goto out1;
>  		gpu_addr = drm_gem_vram_offset(pixels_1);
>  		if (gpu_addr < 0) {
> -			drm_gem_vram_unpin(pixels_1);
> +			drm_gem_vram_unpin_reserved(pixels_1);
>  			goto out1;
>  		}
>  		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
>  	}
>  	if (!pixels_2->pin_count) {
> -		ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
> +		ret = drm_gem_vram_pin_reserved(pixels_2,
> +						DRM_GEM_VRAM_PL_FLAG_VRAM);
>  		if (ret) {
> -			drm_gem_vram_unpin(pixels_1);
> +			drm_gem_vram_unpin_reserved(pixels_1);
>  			goto out1;
>  		}
>  		gpu_addr = drm_gem_vram_offset(pixels_2);
>  		if (gpu_addr < 0) {
> -			drm_gem_vram_unpin(pixels_1);
> -			drm_gem_vram_unpin(pixels_2);
> +			drm_gem_vram_unpin_reserved(pixels_1);
> +			drm_gem_vram_unpin_reserved(pixels_2);
>  			goto out1;
>  		}
>  		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index b056f189ba62..ff1a81761543 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
>  u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
>  s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
>  int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> +			      unsigned long pl_flag);
>  int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
>  int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
>  void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
>  			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Daniel Vetter]

On Mon, May 20, 2019 at 06:19:00PM +0200, Daniel Vetter wrote:
> On Thu, May 16, 2019 at 06:27:45PM +0200, Thomas Zimmermann wrote:
> > The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
> > GEM VRAM pin/unpin functions that do not reserve the BO during validation.
> > The mgag200 driver requires this behavior for its cursor handling. The
> > patch also converts the driver to use the new interfaces.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/gpu/drm/drm_gem_vram_helper.c    | 75 ++++++++++++++++++++++++
> >  drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
> >  include/drm/drm_gem_vram_helper.h        |  3 +
> >  3 files changed, 88 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> > index 8f142b810eb4..a002c03eaf4c 100644
> > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > @@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
> >  }
> >  EXPORT_SYMBOL(drm_gem_vram_pin);
> >  
> > +/**
> > + * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
> > + * @gbo:	the GEM VRAM object
> > + * @pl_flag:	a bitmask of possible memory regions
> > + *
> > + * Pinning a buffer object ensures that it is not evicted from
> > + * a memory region. A pinned buffer object has to be unpinned before
> > + * it can be pinned to another region.
> > + *
> > + * This function pins a GEM VRAM object that has already been
> > + * reserved. Use drm_gem_vram_pin() if possible.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> > +			      unsigned long pl_flag)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> 
> I think would be good to have a lockdep_assert_held here for the ww_mutex.
> 
> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
> we call the structure tracking the fences+lock the "reservation", but the
> naming scheme used is _lock/_unlock.
> 
> I think would be good to be consistent with that, and use _locked here.
> Especially for a very simplified vram helper like this one I expect that's
> going to lead to less wtf moments by driver writers :-)
> 
> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
> more with what we now have in dma-buf.

More aside:

Could be a good move to demidlayer this an essentially remove
ttm_bo_reserve as a wrapper around the linux/reservation.h functions. Not
sure whether that aligns with Christian's plans. TODO.rst patch might be a
good step to get that discussion started.
-Daniel

> 
> Cheers, Daniel
> 
> > +
> > +	if (gbo->pin_count) {
> > +		++gbo->pin_count;
> > +		return 0;
> > +	}
> > +
> > +	drm_gem_vram_placement(gbo, pl_flag);
> > +	for (i = 0; i < gbo->placement.num_placement; ++i)
> > +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	gbo->pin_count = 1;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
> > +
> >  /**
> >   * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> >   * @gbo:	the GEM VRAM object
> > @@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> >  }
> >  EXPORT_SYMBOL(drm_gem_vram_unpin);
> >  
> > +/**
> > + * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * This function unpins a GEM VRAM object that has already been
> > + * reserved. Use drm_gem_vram_unpin() if possible.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (WARN_ON_ONCE(!gbo->pin_count))
> > +		return 0;
> > +
> > +	--gbo->pin_count;
> > +	if (gbo->pin_count)
> > +		return 0;
> > +
> > +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> > +		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
> > +
> >  /**
> >   * drm_gem_vram_push_to_system() - \
> >  	Unpins a GEM VRAM object and moves it to system memory
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > index 6c1a9d724d85..1c4fc85315a0 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > @@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
> >  	WREG8(MGA_CURPOSXL, 0);
> >  	WREG8(MGA_CURPOSXH, 0);
> >  	if (mdev->cursor.pixels_1->pin_count)
> > -		drm_gem_vram_unpin(mdev->cursor.pixels_1);
> > +		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
> >  	if (mdev->cursor.pixels_2->pin_count)
> > -		drm_gem_vram_unpin(mdev->cursor.pixels_2);
> > +		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
> >  }
> >  
> >  int mga_crtc_cursor_set(struct drm_crtc *crtc,
> > @@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
> >  
> >  	/* Move cursor buffers into VRAM if they aren't already */
> >  	if (!pixels_1->pin_count) {
> > -		ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
> > +		ret = drm_gem_vram_pin_reserved(pixels_1,
> > +						DRM_GEM_VRAM_PL_FLAG_VRAM);
> >  		if (ret)
> >  			goto out1;
> >  		gpu_addr = drm_gem_vram_offset(pixels_1);
> >  		if (gpu_addr < 0) {
> > -			drm_gem_vram_unpin(pixels_1);
> > +			drm_gem_vram_unpin_reserved(pixels_1);
> >  			goto out1;
> >  		}
> >  		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
> >  	}
> >  	if (!pixels_2->pin_count) {
> > -		ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
> > +		ret = drm_gem_vram_pin_reserved(pixels_2,
> > +						DRM_GEM_VRAM_PL_FLAG_VRAM);
> >  		if (ret) {
> > -			drm_gem_vram_unpin(pixels_1);
> > +			drm_gem_vram_unpin_reserved(pixels_1);
> >  			goto out1;
> >  		}
> >  		gpu_addr = drm_gem_vram_offset(pixels_2);
> >  		if (gpu_addr < 0) {
> > -			drm_gem_vram_unpin(pixels_1);
> > -			drm_gem_vram_unpin(pixels_2);
> > +			drm_gem_vram_unpin_reserved(pixels_1);
> > +			drm_gem_vram_unpin_reserved(pixels_2);
> >  			goto out1;
> >  		}
> >  		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
> > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > index b056f189ba62..ff1a81761543 100644
> > --- a/include/drm/drm_gem_vram_helper.h
> > +++ b/include/drm/drm_gem_vram_helper.h
> > @@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
> >  u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
> >  s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
> >  int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
> > +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> > +			      unsigned long pl_flag);
> >  int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
> > +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
> >  int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
> >  void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
> >  			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Koenig, Christian]

Am 20.05.19 um 18:26 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Mon, May 20, 2019 at 06:19:00PM +0200, Daniel Vetter wrote:
>> On Thu, May 16, 2019 at 06:27:45PM +0200, Thomas Zimmermann wrote:
>>> The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
>>> GEM VRAM pin/unpin functions that do not reserve the BO during validation.
>>> The mgag200 driver requires this behavior for its cursor handling. The
>>> patch also converts the driver to use the new interfaces.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/drm_gem_vram_helper.c    | 75 ++++++++++++++++++++++++
>>>   drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
>>>   include/drm/drm_gem_vram_helper.h        |  3 +
>>>   3 files changed, 88 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> index 8f142b810eb4..a002c03eaf4c 100644
>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> @@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_vram_pin);
>>>
>>> +/**
>>> + * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
>>> + * @gbo:   the GEM VRAM object
>>> + * @pl_flag:       a bitmask of possible memory regions
>>> + *
>>> + * Pinning a buffer object ensures that it is not evicted from
>>> + * a memory region. A pinned buffer object has to be unpinned before
>>> + * it can be pinned to another region.
>>> + *
>>> + * This function pins a GEM VRAM object that has already been
>>> + * reserved. Use drm_gem_vram_pin() if possible.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or
>>> + * a negative error code otherwise.
>>> + */
>>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
>>> +                         unsigned long pl_flag)
>>> +{
>>> +   int i, ret;
>>> +   struct ttm_operation_ctx ctx = { false, false };
>> I think would be good to have a lockdep_assert_held here for the ww_mutex.
>>
>> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
>> we call the structure tracking the fences+lock the "reservation", but the
>> naming scheme used is _lock/_unlock.
>>
>> I think would be good to be consistent with that, and use _locked here.
>> Especially for a very simplified vram helper like this one I expect that's
>> going to lead to less wtf moments by driver writers :-)
>>
>> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
>> more with what we now have in dma-buf.
> More aside:
>
> Could be a good move to demidlayer this an essentially remove
> ttm_bo_reserve as a wrapper around the linux/reservation.h functions. Not
> sure whether that aligns with Christian's plans. TODO.rst patch might be a
> good step to get that discussion started.

Well what ttm_bo_reserve()/_unreserve() does is a) lock/unlock the 
reservation object and b) remove the BO from the LRU.

Since I'm desperately trying to get rid of the LRU removal right now we 
sooner or later should be able to remove ttmo_bo_reserve()/_unreserve() 
as well (or at least replace them with tiny ttm_bo_lock() wrappers.

Christian.

> -Daniel
>
>> Cheers, Daniel
>>
>>> +
>>> +   if (gbo->pin_count) {
>>> +           ++gbo->pin_count;
>>> +           return 0;
>>> +   }
>>> +
>>> +   drm_gem_vram_placement(gbo, pl_flag);
>>> +   for (i = 0; i < gbo->placement.num_placement; ++i)
>>> +           gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>> +
>>> +   ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
>>> +   if (ret < 0)
>>> +           return ret;
>>> +
>>> +   gbo->pin_count = 1;
>>> +
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
>>> +
>>>   /**
>>>    * drm_gem_vram_unpin() - Unpins a GEM VRAM object
>>>    * @gbo:   the GEM VRAM object
>>> @@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_vram_unpin);
>>>
>>> +/**
>>> + * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
>>> + * @gbo:   the GEM VRAM object
>>> + *
>>> + * This function unpins a GEM VRAM object that has already been
>>> + * reserved. Use drm_gem_vram_unpin() if possible.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or
>>> + * a negative error code otherwise.
>>> + */
>>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
>>> +{
>>> +   int i, ret;
>>> +   struct ttm_operation_ctx ctx = { false, false };
>>> +
>>> +   if (WARN_ON_ONCE(!gbo->pin_count))
>>> +           return 0;
>>> +
>>> +   --gbo->pin_count;
>>> +   if (gbo->pin_count)
>>> +           return 0;
>>> +
>>> +   for (i = 0; i < gbo->placement.num_placement ; ++i)
>>> +           gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
>>> +
>>> +   ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
>>> +   if (ret < 0)
>>> +           return ret;
>>> +
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
>>> +
>>>   /**
>>>    * drm_gem_vram_push_to_system() - \
>>>      Unpins a GEM VRAM object and moves it to system memory
>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> index 6c1a9d724d85..1c4fc85315a0 100644
>>> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> @@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
>>>      WREG8(MGA_CURPOSXL, 0);
>>>      WREG8(MGA_CURPOSXH, 0);
>>>      if (mdev->cursor.pixels_1->pin_count)
>>> -           drm_gem_vram_unpin(mdev->cursor.pixels_1);
>>> +           drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
>>>      if (mdev->cursor.pixels_2->pin_count)
>>> -           drm_gem_vram_unpin(mdev->cursor.pixels_2);
>>> +           drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
>>>   }
>>>
>>>   int mga_crtc_cursor_set(struct drm_crtc *crtc,
>>> @@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>>>
>>>      /* Move cursor buffers into VRAM if they aren't already */
>>>      if (!pixels_1->pin_count) {
>>> -           ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> +           ret = drm_gem_vram_pin_reserved(pixels_1,
>>> +                                           DRM_GEM_VRAM_PL_FLAG_VRAM);
>>>              if (ret)
>>>                      goto out1;
>>>              gpu_addr = drm_gem_vram_offset(pixels_1);
>>>              if (gpu_addr < 0) {
>>> -                   drm_gem_vram_unpin(pixels_1);
>>> +                   drm_gem_vram_unpin_reserved(pixels_1);
>>>                      goto out1;
>>>              }
>>>              mdev->cursor.pixels_1_gpu_addr = gpu_addr;
>>>      }
>>>      if (!pixels_2->pin_count) {
>>> -           ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> +           ret = drm_gem_vram_pin_reserved(pixels_2,
>>> +                                           DRM_GEM_VRAM_PL_FLAG_VRAM);
>>>              if (ret) {
>>> -                   drm_gem_vram_unpin(pixels_1);
>>> +                   drm_gem_vram_unpin_reserved(pixels_1);
>>>                      goto out1;
>>>              }
>>>              gpu_addr = drm_gem_vram_offset(pixels_2);
>>>              if (gpu_addr < 0) {
>>> -                   drm_gem_vram_unpin(pixels_1);
>>> -                   drm_gem_vram_unpin(pixels_2);
>>> +                   drm_gem_vram_unpin_reserved(pixels_1);
>>> +                   drm_gem_vram_unpin_reserved(pixels_2);
>>>                      goto out1;
>>>              }
>>>              mdev->cursor.pixels_2_gpu_addr = gpu_addr;
>>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>>> index b056f189ba62..ff1a81761543 100644
>>> --- a/include/drm/drm_gem_vram_helper.h
>>> +++ b/include/drm/drm_gem_vram_helper.h
>>> @@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
>>>   u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
>>>   s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
>>>   int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
>>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
>>> +                         unsigned long pl_flag);
>>>   int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
>>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
>>>   int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
>>>   void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
>>>                         bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
>>> --
>>> 2.21.0
>>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Daniel Vetter]

On Mon, May 20, 2019 at 9:24 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 20.05.19 um 18:26 schrieb Daniel Vetter:
> > [CAUTION: External Email]
> >
> > On Mon, May 20, 2019 at 06:19:00PM +0200, Daniel Vetter wrote:
> >> On Thu, May 16, 2019 at 06:27:45PM +0200, Thomas Zimmermann wrote:
> >>> The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
> >>> GEM VRAM pin/unpin functions that do not reserve the BO during validation.
> >>> The mgag200 driver requires this behavior for its cursor handling. The
> >>> patch also converts the driver to use the new interfaces.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> ---
> >>>   drivers/gpu/drm/drm_gem_vram_helper.c    | 75 ++++++++++++++++++++++++
> >>>   drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
> >>>   include/drm/drm_gem_vram_helper.h        |  3 +
> >>>   3 files changed, 88 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> >>> index 8f142b810eb4..a002c03eaf4c 100644
> >>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> >>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> >>> @@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
> >>>   }
> >>>   EXPORT_SYMBOL(drm_gem_vram_pin);
> >>>
> >>> +/**
> >>> + * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
> >>> + * @gbo:   the GEM VRAM object
> >>> + * @pl_flag:       a bitmask of possible memory regions
> >>> + *
> >>> + * Pinning a buffer object ensures that it is not evicted from
> >>> + * a memory region. A pinned buffer object has to be unpinned before
> >>> + * it can be pinned to another region.
> >>> + *
> >>> + * This function pins a GEM VRAM object that has already been
> >>> + * reserved. Use drm_gem_vram_pin() if possible.
> >>> + *
> >>> + * Returns:
> >>> + * 0 on success, or
> >>> + * a negative error code otherwise.
> >>> + */
> >>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> >>> +                         unsigned long pl_flag)
> >>> +{
> >>> +   int i, ret;
> >>> +   struct ttm_operation_ctx ctx = { false, false };
> >> I think would be good to have a lockdep_assert_held here for the ww_mutex.
> >>
> >> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
> >> we call the structure tracking the fences+lock the "reservation", but the
> >> naming scheme used is _lock/_unlock.
> >>
> >> I think would be good to be consistent with that, and use _locked here.
> >> Especially for a very simplified vram helper like this one I expect that's
> >> going to lead to less wtf moments by driver writers :-)
> >>
> >> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
> >> more with what we now have in dma-buf.
> > More aside:
> >
> > Could be a good move to demidlayer this an essentially remove
> > ttm_bo_reserve as a wrapper around the linux/reservation.h functions. Not
> > sure whether that aligns with Christian's plans. TODO.rst patch might be a
> > good step to get that discussion started.
>
> Well what ttm_bo_reserve()/_unreserve() does is a) lock/unlock the
> reservation object and b) remove the BO from the LRU.
>
> Since I'm desperately trying to get rid of the LRU removal right now we
> sooner or later should be able to remove ttmo_bo_reserve()/_unreserve()
> as well (or at least replace them with tiny ttm_bo_lock() wrappers.

Yeah that's what I meant, would be nice to ditch that bit of
midlayering, at least from drivers.

I guess within ttm we'd still need ttm_bo_lock/unlock, since
interrupteble/timedout/lock modes in general is passed around
throughout the entire stack as parameters.
-Daniel

>
> Christian.
>
> > -Daniel
> >
> >> Cheers, Daniel
> >>
> >>> +
> >>> +   if (gbo->pin_count) {
> >>> +           ++gbo->pin_count;
> >>> +           return 0;
> >>> +   }
> >>> +
> >>> +   drm_gem_vram_placement(gbo, pl_flag);
> >>> +   for (i = 0; i < gbo->placement.num_placement; ++i)
> >>> +           gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> >>> +
> >>> +   ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> >>> +   if (ret < 0)
> >>> +           return ret;
> >>> +
> >>> +   gbo->pin_count = 1;
> >>> +
> >>> +   return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
> >>> +
> >>>   /**
> >>>    * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> >>>    * @gbo:   the GEM VRAM object
> >>> @@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> >>>   }
> >>>   EXPORT_SYMBOL(drm_gem_vram_unpin);
> >>>
> >>> +/**
> >>> + * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
> >>> + * @gbo:   the GEM VRAM object
> >>> + *
> >>> + * This function unpins a GEM VRAM object that has already been
> >>> + * reserved. Use drm_gem_vram_unpin() if possible.
> >>> + *
> >>> + * Returns:
> >>> + * 0 on success, or
> >>> + * a negative error code otherwise.
> >>> + */
> >>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
> >>> +{
> >>> +   int i, ret;
> >>> +   struct ttm_operation_ctx ctx = { false, false };
> >>> +
> >>> +   if (WARN_ON_ONCE(!gbo->pin_count))
> >>> +           return 0;
> >>> +
> >>> +   --gbo->pin_count;
> >>> +   if (gbo->pin_count)
> >>> +           return 0;
> >>> +
> >>> +   for (i = 0; i < gbo->placement.num_placement ; ++i)
> >>> +           gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> >>> +
> >>> +   ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> >>> +   if (ret < 0)
> >>> +           return ret;
> >>> +
> >>> +   return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
> >>> +
> >>>   /**
> >>>    * drm_gem_vram_push_to_system() - \
> >>>      Unpins a GEM VRAM object and moves it to system memory
> >>> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> >>> index 6c1a9d724d85..1c4fc85315a0 100644
> >>> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> >>> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> >>> @@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
> >>>      WREG8(MGA_CURPOSXL, 0);
> >>>      WREG8(MGA_CURPOSXH, 0);
> >>>      if (mdev->cursor.pixels_1->pin_count)
> >>> -           drm_gem_vram_unpin(mdev->cursor.pixels_1);
> >>> +           drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
> >>>      if (mdev->cursor.pixels_2->pin_count)
> >>> -           drm_gem_vram_unpin(mdev->cursor.pixels_2);
> >>> +           drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
> >>>   }
> >>>
> >>>   int mga_crtc_cursor_set(struct drm_crtc *crtc,
> >>> @@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
> >>>
> >>>      /* Move cursor buffers into VRAM if they aren't already */
> >>>      if (!pixels_1->pin_count) {
> >>> -           ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
> >>> +           ret = drm_gem_vram_pin_reserved(pixels_1,
> >>> +                                           DRM_GEM_VRAM_PL_FLAG_VRAM);
> >>>              if (ret)
> >>>                      goto out1;
> >>>              gpu_addr = drm_gem_vram_offset(pixels_1);
> >>>              if (gpu_addr < 0) {
> >>> -                   drm_gem_vram_unpin(pixels_1);
> >>> +                   drm_gem_vram_unpin_reserved(pixels_1);
> >>>                      goto out1;
> >>>              }
> >>>              mdev->cursor.pixels_1_gpu_addr = gpu_addr;
> >>>      }
> >>>      if (!pixels_2->pin_count) {
> >>> -           ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
> >>> +           ret = drm_gem_vram_pin_reserved(pixels_2,
> >>> +                                           DRM_GEM_VRAM_PL_FLAG_VRAM);
> >>>              if (ret) {
> >>> -                   drm_gem_vram_unpin(pixels_1);
> >>> +                   drm_gem_vram_unpin_reserved(pixels_1);
> >>>                      goto out1;
> >>>              }
> >>>              gpu_addr = drm_gem_vram_offset(pixels_2);
> >>>              if (gpu_addr < 0) {
> >>> -                   drm_gem_vram_unpin(pixels_1);
> >>> -                   drm_gem_vram_unpin(pixels_2);
> >>> +                   drm_gem_vram_unpin_reserved(pixels_1);
> >>> +                   drm_gem_vram_unpin_reserved(pixels_2);
> >>>                      goto out1;
> >>>              }
> >>>              mdev->cursor.pixels_2_gpu_addr = gpu_addr;
> >>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> >>> index b056f189ba62..ff1a81761543 100644
> >>> --- a/include/drm/drm_gem_vram_helper.h
> >>> +++ b/include/drm/drm_gem_vram_helper.h
> >>> @@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
> >>>   u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
> >>>   s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
> >>>   int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
> >>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
> >>> +                         unsigned long pl_flag);
> >>>   int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
> >>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
> >>>   int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
> >>>   void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
> >>>                         bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
> >>> --
> >>> 2.21.0
> >>>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Gerd Hoffmann]

  Hi,

> I think would be good to have a lockdep_assert_held here for the ww_mutex.
> 
> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
> we call the structure tracking the fences+lock the "reservation", but the
> naming scheme used is _lock/_unlock.
> 
> I think would be good to be consistent with that, and use _locked here.
> Especially for a very simplified vram helper like this one I expect that's
> going to lead to less wtf moments by driver writers :-)
> 
> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
> more with what we now have in dma-buf.

Given that mgag200 is the only user I think the best way forward is to
improve the mgag200 cursor handling so we can just drop the _reserved
variants ...

When looking at mga_crtc_cursor_set() I suspect the easierst way to do
that would be to simply pin the cursor bo's at driver_load time, then we
don't have to bother with pinning in mga_crtc_cursor_set() at all.

Thomas, as you have test hardware, can you look into this?

thanks,
  Gerd

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1856 bytes --]

Hi

Am 21.05.19 um 12:35 schrieb Gerd Hoffmann:
>   Hi,
> 
>> I think would be good to have a lockdep_assert_held here for the ww_mutex.
>>
>> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
>> we call the structure tracking the fences+lock the "reservation", but the
>> naming scheme used is _lock/_unlock.
>>
>> I think would be good to be consistent with that, and use _locked here.
>> Especially for a very simplified vram helper like this one I expect that's
>> going to lead to less wtf moments by driver writers :-)
>>
>> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
>> more with what we now have in dma-buf.
> 
> Given that mgag200 is the only user I think the best way forward is to
> improve the mgag200 cursor handling so we can just drop the _reserved
> variants ...
> 
> When looking at mga_crtc_cursor_set() I suspect the easierst way to do
> that would be to simply pin the cursor bo's at driver_load time, then we
> don't have to bother with pinning in mga_crtc_cursor_set() at all.
> 
> Thomas, as you have test hardware, can you look into this?

Sure, that's the plan. May take a few days, though.

I'd like to see all of the locking/reservation gone from the helper API.
By using bochs' PRIME helpers, the generic console could probably be
used in mgag200 and ast. That would remove the remaining calls from
mgag200 and ast.

Best regards
Thomas


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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

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

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

Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2189 bytes --]

Hi

Am 21.05.19 um 12:35 schrieb Gerd Hoffmann:
>   Hi,
> 
>> I think would be good to have a lockdep_assert_held here for the ww_mutex.
>>
>> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
>> we call the structure tracking the fences+lock the "reservation", but the
>> naming scheme used is _lock/_unlock.
>>
>> I think would be good to be consistent with that, and use _locked here.
>> Especially for a very simplified vram helper like this one I expect that's
>> going to lead to less wtf moments by driver writers :-)
>>
>> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
>> more with what we now have in dma-buf.
> 
> Given that mgag200 is the only user I think the best way forward is to
> improve the mgag200 cursor handling so we can just drop the _reserved
> variants ...
> 
> When looking at mga_crtc_cursor_set() I suspect the easierst way to do
> that would be to simply pin the cursor bo's at driver_load time, then we
> don't have to bother with pinning in mga_crtc_cursor_set() at all.

I've been thinking about converting ast and mgag200 to atomic mode
setting. For that, universal planes would be required. But this seems
incompatible with HW cursors.

With universal planes, cursor planes would probably have to be exported
as RGBA8 framebuffer, but the HW uses MMIO region and 16-color palette.
So there's got to be a translation step and a way of notifying userspace
if the palette overflows. In the current driver code,
drm_crtc_funcs.set_cursor() does this.

How is this done with universal planes? I know that there's
drm_framebuffer_funcs.dirty(), but it doesn't seem to be used much.

Best regards
Thomas


> Thomas, as you have test hardware, can you look into this?
> 
> thanks,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

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

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