[PATCH v3 0/9] Remove explicit locking and kmap arguments from GEM VRAM interface

[PATCH v3 0/9] Remove explicit locking and kmap arguments from GEM VRAM interface

[Thomas Zimmermann]

Drivers should not have to care about internal locking of GEM VRAM objects
and their memory-mapping structures. This patch set removes both from the
GEM VRAM interface.

This affects the ast and mgag200 drivers. In places where GEM objects are
being locked by the driver, the patch converts the lock operation to a pin
operation. The locking prevented the memory manager from moving the object,
so pinning is more appropriate.

For the memory mappings, all book-keeping is done by the implementation
of GEM VRAM. Explicit kmap objects are removed from the module's public
interfaces. This change mostly affects the cursor handling in ast and
mgag200, which is being simplified by this patch set.

Future directions: with these patches in place, more code in mode setting
and fbdev emulation can be shared between ast and mgag200.

The patches have been tested on ast and mgag200 hardware.

v3:
	* document PRIME pin flags
	* pin cursor BOs at current location
v2:
	* support pinning BOs at current location
	* pin PRIME buffers to current location

Thomas Zimmermann (9):
  drm/gem-vram: Support pinning buffers to current location
  drm/ast: Unpin cursor BO during cleanup
  drm/ast: Remove obsolete or unused cursor state
  drm/ast: Pin and map cursor source BO during update
  drm/ast: Pin framebuffer BO during dirty update
  drm/mgag200: Pin framebuffer BO during dirty update
  drm/mgag200: Rewrite cursor handling
  drm: Remove lock interfaces from GEM VRAM helpers
  drm: Remove functions with kmap-object argument from GEM VRAM helpers

 drivers/gpu/drm/ast/ast_drv.h            |   7 -
 drivers/gpu/drm/ast/ast_fb.c             |  33 ++---
 drivers/gpu/drm/ast/ast_mode.c           |  65 ++++----
 drivers/gpu/drm/drm_gem_vram_helper.c    | 179 +++--------------------
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 +++++++++------------
 drivers/gpu/drm/mgag200/mgag200_drv.h    |   3 -
 drivers/gpu/drm/mgag200/mgag200_fb.c     |  27 ++--
 drivers/gpu/drm/mgag200/mgag200_main.c   |   4 +-
 include/drm/drm_gem_vram_helper.h        |   9 --
 9 files changed, 155 insertions(+), 337 deletions(-)

--
2.21.0

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

[PATCH v3 1/9] drm/gem-vram: Support pinning buffers to current location

[Thomas Zimmermann]

Pinning a buffer prevents it from being moved to a different memory
location. For some operations, such as buffer updates, it is not
important where the buffer is located. Setting the pin function's
pl_flag argument to 0 will pin the buffer to whereever it is stored.

v2:
	* document pin flags in PRIME pin helper

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 42ad80888df7..f3e5803affb0 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -224,7 +224,9 @@ EXPORT_SYMBOL(drm_gem_vram_offset);
  *
  * 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.
+ * it can be pinned to another region. If the pl_flag argument is 0,
+ * the buffer is pinned at its current location (video RAM or system
+ * memory).
  *
  * Returns:
  * 0 on success, or
@@ -242,7 +244,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 	if (gbo->pin_count)
 		goto out;
 
-	drm_gem_vram_placement(gbo, pl_flag);
+	if (pl_flag)
+		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;
 
@@ -691,7 +695,15 @@ int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem)
 {
 	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
 
-	return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
+	/* Fbdev console emulation is the use case of these PRIME
+	 * helpers. This may involve updating a hardware buffer from
+	 * a shadow FB. We pin the buffer to it's current location
+	 * (either video RAM or system memory) to prevent it from
+	 * being relocated during the update operation. If you require
+	 * the buffer to be pinned to VRAM, implement a callback that
+	 * sets the flags accordingly.
+	 */
+	return drm_gem_vram_pin(gbo, 0);
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_pin);
 
@@ -723,7 +735,7 @@ void *drm_gem_vram_driver_gem_prime_vmap(struct drm_gem_object *gem)
 	int ret;
 	void *base;
 
-	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
+	ret = drm_gem_vram_pin(gbo, 0);
 	if (ret)
 		return NULL;
 	base = drm_gem_vram_kmap(gbo, true, NULL);
-- 
2.21.0

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

[PATCH v3 2/9] drm/ast: Unpin cursor BO during cleanup

[Thomas Zimmermann]

The unpin operation was missing from ast_cursor_fini(). Fixed now.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fb700d620b64..41741cd6cd15 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -959,6 +959,7 @@ static void ast_cursor_fini(struct drm_device *dev)
 	struct drm_gem_vram_object *gbo =
 		drm_gem_vram_of_gem(ast->cursor_cache);
 	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(ast->cursor_cache);
 }
 
-- 
2.21.0

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

[PATCH v3 3/9] drm/ast: Remove obsolete or unused cursor state

[Thomas Zimmermann]

The ast driver's data structures store unused or uncecessary cursor
state. Most of the cursor state is already stored elsewhere and can
be retrieved when necessary. Remove the obsolete fields and adapt
users accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  7 -------
 drivers/gpu/drm/ast/ast_mode.c | 31 +++++++++++++++++--------------
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index b6cac9511796..684e15e64a62 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -101,10 +101,6 @@ struct ast_private {
 	int fb_mtrr;
 
 	struct drm_gem_object *cursor_cache;
-	uint64_t cursor_cache_gpu_addr;
-	/* Acces to this cache is protected by the crtc->mutex of the only crtc
-	 * we have. */
-	struct ttm_bo_kmap_obj cache_kmap;
 	int next_cursor;
 	bool support_wide_screen;
 	enum {
@@ -236,9 +232,6 @@ struct ast_connector {
 
 struct ast_crtc {
 	struct drm_crtc base;
-	struct drm_gem_object *cursor_bo;
-	uint64_t cursor_addr;
-	int cursor_width, cursor_height;
 	u8 offset_x, offset_y;
 };
 
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 41741cd6cd15..cb6e8249a7db 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -939,15 +939,13 @@ static int ast_cursor_init(struct drm_device *dev)
 	}
 
 	/* kmap the object */
-	base = drm_gem_vram_kmap_at(gbo, true, NULL, &ast->cache_kmap);
+	base = drm_gem_vram_kmap(gbo, true, NULL);
 	if (IS_ERR(base)) {
 		ret = PTR_ERR(base);
 		goto fail;
 	}
 
 	ast->cursor_cache = obj;
-	ast->cursor_cache_gpu_addr = gpu_addr;
-	DRM_DEBUG_KMS("pinned cursor cache at %llx\n", ast->cursor_cache_gpu_addr);
 	return 0;
 fail:
 	return ret;
@@ -958,7 +956,7 @@ static void ast_cursor_fini(struct drm_device *dev)
 	struct ast_private *ast = dev->dev_private;
 	struct drm_gem_vram_object *gbo =
 		drm_gem_vram_of_gem(ast->cursor_cache);
-	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
+	drm_gem_vram_kunmap(gbo);
 	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(ast->cursor_cache);
 }
@@ -1181,7 +1179,8 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
-	s64 gpu_addr;
+	s64 dst_gpu;
+	u64 gpu_addr;
 	u32 csum;
 	int ret;
 	struct ttm_bo_kmap_obj uobj_map;
@@ -1215,14 +1214,19 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	if (src_isiomem == true)
 		DRM_ERROR("src cursor bo should be in main memory\n");
 
-	dst = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-				   false, &dst_isiomem, &ast->cache_kmap);
+	dst = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
+				false, &dst_isiomem);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
 		goto fail_unlock;
 	}
 	if (dst_isiomem == false)
 		DRM_ERROR("dst bo should be in VRAM\n");
+	dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache));
+	if (dst_gpu < 0) {
+		ret = (int)dst_gpu;
+		goto fail_unlock;
+	}
 
 	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
 
@@ -1234,8 +1238,9 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 
 	/* write checksum + signature */
 	{
-		u8 *dst = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-					       false, NULL, &ast->cache_kmap);
+		struct drm_gem_vram_object *dst_gbo =
+			drm_gem_vram_of_gem(ast->cursor_cache);
+		u8 *dst = drm_gem_vram_kmap(dst_gbo, false, NULL);
 		dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor + AST_HWC_SIZE;
 		writel(csum, dst);
 		writel(width, dst + AST_HWC_SIGNATURE_SizeX);
@@ -1244,15 +1249,13 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 		writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY);
 
 		/* set pattern offset */
-		gpu_addr = ast->cursor_cache_gpu_addr;
+		gpu_addr = (u64)dst_gpu;
 		gpu_addr += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
 		gpu_addr >>= 3;
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc8, gpu_addr & 0xff);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc9, (gpu_addr >> 8) & 0xff);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, (gpu_addr >> 16) & 0xff);
 	}
-	ast_crtc->cursor_width = width;
-	ast_crtc->cursor_height = height;
 	ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width;
 	ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height;
 
@@ -1278,8 +1281,8 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 	int x_offset, y_offset;
 	u8 *sig;
 
-	sig = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-				   false, NULL, &ast->cache_kmap);
+	sig = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
+				   false, NULL);
 	sig += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor + AST_HWC_SIZE;
 	writel(x, sig + AST_HWC_SIGNATURE_X);
 	writel(y, sig + AST_HWC_SIGNATURE_Y);
-- 
2.21.0

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

[PATCH v3 4/9] drm/ast: Pin and map cursor source BO during update

[Thomas Zimmermann]

The ast driver used to lock the cursor source BO during updates. Locking
should be done internally by the BO's implementation, so we pin it instead
to system memory. The mapping information is also stored in the BO. No
need to have an extra argument to the kmap function.

v2:
	* pin cursor BOs to current location

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 37 +++++++++++++++-------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cb6e8249a7db..6649d370b2f1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1183,9 +1183,8 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	u64 gpu_addr;
 	u32 csum;
 	int ret;
-	struct ttm_bo_kmap_obj uobj_map;
 	u8 *src, *dst;
-	bool src_isiomem, dst_isiomem;
+
 	if (!handle) {
 		ast_hide_cursor(crtc);
 		return 0;
@@ -1201,31 +1200,25 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	}
 	gbo = drm_gem_vram_of_gem(obj);
 
-	ret = drm_gem_vram_lock(gbo, false);
+	ret = drm_gem_vram_pin(gbo, 0);
 	if (ret)
-		goto fail;
-
-	memset(&uobj_map, 0, sizeof(uobj_map));
-	src = drm_gem_vram_kmap_at(gbo, true, &src_isiomem, &uobj_map);
+		goto err_drm_gem_object_put_unlocked;
+	src = drm_gem_vram_kmap(gbo, true, NULL);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		goto fail_unlock;
+		goto err_drm_gem_vram_unpin;
 	}
-	if (src_isiomem == true)
-		DRM_ERROR("src cursor bo should be in main memory\n");
 
 	dst = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
-				false, &dst_isiomem);
+				false, NULL);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
-		goto fail_unlock;
+		goto err_drm_gem_vram_kunmap;
 	}
-	if (dst_isiomem == false)
-		DRM_ERROR("dst bo should be in VRAM\n");
 	dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache));
 	if (dst_gpu < 0) {
 		ret = (int)dst_gpu;
-		goto fail_unlock;
+		goto err_drm_gem_vram_kunmap;
 	}
 
 	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
@@ -1233,9 +1226,6 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	/* do data transfer to cursor cache */
 	csum = copy_cursor_image(src, dst, width, height);
 
-	drm_gem_vram_kunmap_at(gbo, &uobj_map);
-	drm_gem_vram_unlock(gbo);
-
 	/* write checksum + signature */
 	{
 		struct drm_gem_vram_object *dst_gbo =
@@ -1263,12 +1253,17 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 
 	ast_show_cursor(crtc);
 
+	drm_gem_vram_kunmap(gbo);
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(obj);
+
 	return 0;
 
-fail_unlock:
-	drm_gem_vram_unlock(gbo);
-fail:
+err_drm_gem_vram_kunmap:
+	drm_gem_vram_kunmap(gbo);
+err_drm_gem_vram_unpin:
+	drm_gem_vram_unpin(gbo);
+err_drm_gem_object_put_unlocked:
 	drm_gem_object_put_unlocked(obj);
 	return ret;
 }
-- 
2.21.0

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

[PATCH v3 5/9] drm/ast: Pin framebuffer BO during dirty update

[Thomas Zimmermann]

Another explicit lock operation of a GEM VRAM BO is located in AST's
framebuffer update code. Instead of locking the BO, we pin it to wherever
it is.

v2:
	* update with pin flag of 0

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_fb.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 05f45222b702..b404b51c2c55 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -48,32 +48,31 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 			     int x, int y, int width, int height)
 {
 	int i;
-	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
 	int src_offset, dst_offset;
 	int bpp = afbdev->afb.base.format->cpp[0];
-	int ret = -EBUSY;
+	int ret;
 	u8 *dst;
 	bool unmap = false;
 	bool store_for_later = false;
 	int x2, y2;
 	unsigned long flags;
 
-	obj = afbdev->afb.obj;
-	gbo = drm_gem_vram_of_gem(obj);
-
-	/* Try to lock the BO. If we fail with -EBUSY then
-	 * the BO is being moved and we should store up the
-	 * damage until later.
-	 */
-	if (drm_can_sleep())
-		ret = drm_gem_vram_lock(gbo, true);
-	if (ret) {
-		if (ret != -EBUSY)
-			return;
-
+	gbo = drm_gem_vram_of_gem(afbdev->afb.obj);
+
+	if (drm_can_sleep()) {
+		/* We pin the BO so it won't be moved during the
+		 * update. The actual location, video RAM or system
+		 * memory, is not important.
+		 */
+		ret = drm_gem_vram_pin(gbo, 0);
+		if (ret) {
+			if (ret != -EBUSY)
+				return;
+			store_for_later = true;
+		}
+	} else
 		store_for_later = true;
-	}
 
 	x2 = x + width - 1;
 	y2 = y + height - 1;
@@ -126,7 +125,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unlock(gbo);
+	drm_gem_vram_unpin(gbo);
 }
 
 static void ast_fillrect(struct fb_info *info,
-- 
2.21.0

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

[PATCH v3 6/9] drm/mgag200: Pin framebuffer BO during dirty update

[Thomas Zimmermann]

Another explicit lock operation of a GEM VRAM BO is located in mgag200's
framebuffer update code. Instead of locking the BO, we pin it to wherever
it is.

v2:
	* update with pin flag of 0

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 37e8b8cf37ea..207dbee4938e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -23,7 +23,7 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 	struct drm_gem_vram_object *gbo;
 	int src_offset, dst_offset;
 	int bpp = mfbdev->mfb.base.format->cpp[0];
-	int ret = -EBUSY;
+	int ret;
 	u8 *dst;
 	bool unmap = false;
 	bool store_for_later = false;
@@ -33,18 +33,19 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 	obj = mfbdev->mfb.obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	/* Try to lock the BO. If we fail with -EBUSY then
-	 * the BO is being moved and we should store up the
-	 * damage until later.
-	 */
-	if (drm_can_sleep())
-		ret = drm_gem_vram_lock(gbo, true);
-	if (ret) {
-		if (ret != -EBUSY)
-			return;
-
+	if (drm_can_sleep()) {
+		/* We pin the BO so it won't be moved during the
+		 * update. The actual location, video RAM or system
+		 * memory, is not important.
+		 */
+		ret = drm_gem_vram_pin(gbo, 0);
+		if (ret) {
+			if (ret != -EBUSY)
+				return;
+			store_for_later = true;
+		}
+	} else
 		store_for_later = true;
-	}
 
 	x2 = x + width - 1;
 	y2 = y + height - 1;
@@ -97,7 +98,7 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unlock(gbo);
+	drm_gem_vram_unpin(gbo);
 }
 
 static void mga_fillrect(struct fb_info *info,
-- 
2.21.0

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

[PATCH v3 7/9] drm/mgag200: Rewrite cursor handling

[Thomas Zimmermann]

The cursor handling in mgag200 is complicated to understand. It touches a
number of different BOs, but doesn't really use all of them.

Rewriting the cursor update reduces the amount of cursor state. There are
two BOs for double-buffered HW updates. The source BO updates the one that
is currently not displayed and then switches buffers. Explicit BO locking
has been removed from the code. BOs are simply pinned and unpinned in video
RAM.

v2:
	* pin cursor BOs to current location

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 ++++++++++-------------
 drivers/gpu/drm/mgag200/mgag200_drv.h    |   3 -
 drivers/gpu/drm/mgag200/mgag200_main.c   |   4 +-
 3 files changed, 69 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index de94a650077b..f0c61a92351c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -19,10 +19,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_locked(mdev->cursor.pixels_1);
-	if (mdev->cursor.pixels_2->pin_count)
-		drm_gem_vram_unpin_locked(mdev->cursor.pixels_2);
+	if (mdev->cursor.pixels_current)
+		drm_gem_vram_unpin(mdev->cursor.pixels_current);
+	mdev->cursor.pixels_current = NULL;
 }
 
 int mga_crtc_cursor_set(struct drm_crtc *crtc,
@@ -36,7 +35,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	struct drm_gem_vram_object *pixels_1 = mdev->cursor.pixels_1;
 	struct drm_gem_vram_object *pixels_2 = mdev->cursor.pixels_2;
 	struct drm_gem_vram_object *pixels_current = mdev->cursor.pixels_current;
-	struct drm_gem_vram_object *pixels_prev = mdev->cursor.pixels_prev;
+	struct drm_gem_vram_object *pixels_next;
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo = NULL;
 	int ret = 0;
@@ -49,6 +48,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	bool found = false;
 	int colour_count = 0;
 	s64 gpu_addr;
+	u64 dst_gpu;
 	u8 reg_index;
 	u8 this_row[48];
 
@@ -58,80 +58,66 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 		return -ENOTSUPP; /* Didn't allocate space for cursors */
 	}
 
-	if ((width != 64 || height != 64) && handle) {
-		WREG8(MGA_CURPOSXL, 0);
-		WREG8(MGA_CURPOSXH, 0);
-		return -EINVAL;
+	if (WARN_ON(pixels_current &&
+		    pixels_1 != pixels_current &&
+		    pixels_2 != pixels_current)) {
+		return -ENOTSUPP; /* inconsistent state */
 	}
 
-	BUG_ON(pixels_1 != pixels_current && pixels_1 != pixels_prev);
-	BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev);
-	BUG_ON(pixels_current == pixels_prev);
-
 	if (!handle || !file_priv) {
 		mga_hide_cursor(mdev);
 		return 0;
 	}
 
-	obj = drm_gem_object_lookup(file_priv, handle);
-	if (!obj)
-		return -ENOENT;
-
-	ret = drm_gem_vram_lock(pixels_1, true);
-	if (ret) {
+	if (width != 64 || height != 64) {
 		WREG8(MGA_CURPOSXL, 0);
 		WREG8(MGA_CURPOSXH, 0);
-		goto out_unref;
-	}
-	ret = drm_gem_vram_lock(pixels_2, true);
-	if (ret) {
-		WREG8(MGA_CURPOSXL, 0);
-		WREG8(MGA_CURPOSXH, 0);
-		drm_gem_vram_unlock(pixels_1);
-		goto out_unlock1;
+		return -EINVAL;
 	}
 
-	/* Move cursor buffers into VRAM if they aren't already */
-	if (!pixels_1->pin_count) {
-		ret = drm_gem_vram_pin_locked(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_locked(pixels_1);
-			goto out1;
-		}
-		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
-	}
-	if (!pixels_2->pin_count) {
-		ret = drm_gem_vram_pin_locked(pixels_2,
-					      DRM_GEM_VRAM_PL_FLAG_VRAM);
-		if (ret) {
-			drm_gem_vram_unpin_locked(pixels_1);
-			goto out1;
-		}
-		gpu_addr = drm_gem_vram_offset(pixels_2);
-		if (gpu_addr < 0) {
-			drm_gem_vram_unpin_locked(pixels_1);
-			drm_gem_vram_unpin_locked(pixels_2);
-			goto out1;
-		}
-		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
-	}
+	if (pixels_current == pixels_1)
+		pixels_next = pixels_2;
+	else
+		pixels_next = pixels_1;
 
+	obj = drm_gem_object_lookup(file_priv, handle);
+	if (!obj)
+		return -ENOENT;
 	gbo = drm_gem_vram_of_gem(obj);
-	ret = drm_gem_vram_lock(gbo, true);
+	ret = drm_gem_vram_pin(gbo, 0);
 	if (ret) {
 		dev_err(&dev->pdev->dev, "failed to lock user bo\n");
-		goto out1;
+		goto err_drm_gem_object_put_unlocked;
 	}
 	src = drm_gem_vram_kmap(gbo, true, NULL);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		dev_err(&dev->pdev->dev, "failed to kmap user buffer updates\n");
-		goto out2;
+		dev_err(&dev->pdev->dev,
+			"failed to kmap user buffer updates\n");
+		goto err_drm_gem_vram_unpin_src;
+	}
+
+	/* Pin and map up-coming buffer to write colour indices */
+	ret = drm_gem_vram_pin(pixels_next, 0);
+	if (ret)
+		dev_err(&dev->pdev->dev,
+			"failed to pin cursor buffer: %d\n", ret);
+		goto err_drm_gem_vram_kunmap_src;
+	dst = drm_gem_vram_kmap(pixels_next, true, NULL);
+	if (IS_ERR(dst)) {
+		ret = PTR_ERR(dst);
+		dev_err(&dev->pdev->dev,
+			"failed to kmap cursor updates: %d\n", ret);
+		goto err_drm_gem_vram_unpin_dst;
+	}
+	gpu_addr = drm_gem_vram_offset(pixels_2);
+	if (gpu_addr < 0) {
+		ret = (int)gpu_addr;
+		dev_err(&dev->pdev->dev,
+			"failed to get cursor scanout address: %d\n", ret);
+		goto err_drm_gem_vram_kunmap_dst;
 	}
+	dst_gpu = (u64)gpu_addr;
 
 	memset(&colour_set[0], 0, sizeof(uint32_t)*16);
 	/* width*height*4 = 16384 */
@@ -146,7 +132,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 				warn_transparent = false; /* Only tell the user once. */
 			}
 			ret = -EINVAL;
-			goto out3;
+			goto err_drm_gem_vram_kunmap_dst;
 		}
 		/* Don't need to store transparent pixels as colours */
 		if (this_colour>>24 == 0x0)
@@ -168,7 +154,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 				warn_palette = false; /* Only tell the user once. */
 			}
 			ret = -EINVAL;
-			goto out3;
+			goto err_drm_gem_vram_kunmap_dst;
 		}
 		*next_space = this_colour;
 		next_space++;
@@ -187,14 +173,6 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 		BUG_ON((colour_set[i]>>24 & 0xff) != 0xff);
 	}
 
-	/* Map up-coming buffer to write colour indices */
-	dst = drm_gem_vram_kmap(pixels_prev, true, NULL);
-	if (IS_ERR(dst)) {
-		ret = PTR_ERR(dst);
-		dev_err(&dev->pdev->dev, "failed to kmap cursor updates\n");
-		goto out3;
-	}
-
 	/* now write colour indices into hardware cursor buffer */
 	for (row = 0; row < 64; row++) {
 		memset(&this_row[0], 0, 48);
@@ -221,42 +199,35 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	}
 
 	/* Program gpu address of cursor buffer */
-	if (pixels_prev == pixels_1)
-		gpu_addr = mdev->cursor.pixels_1_gpu_addr;
-	else
-		gpu_addr = mdev->cursor.pixels_2_gpu_addr;
-	WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((gpu_addr>>10) & 0xff));
-	WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((gpu_addr>>18) & 0x3f));
+	WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((dst_gpu>>10) & 0xff));
+	WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((dst_gpu>>18) & 0x3f));
 
 	/* Adjust cursor control register to turn on the cursor */
 	WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
 
-	/* Now swap internal buffer pointers */
-	if (mdev->cursor.pixels_1 == mdev->cursor.pixels_prev) {
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_2;
-		mdev->cursor.pixels_current = mdev->cursor.pixels_1;
-	} else if (mdev->cursor.pixels_1 == mdev->cursor.pixels_current) {
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_1;
-		mdev->cursor.pixels_current = mdev->cursor.pixels_2;
-	} else {
-		BUG();
-	}
-	ret = 0;
+	/* Now update internal buffer pointers */
+	if (pixels_current)
+		drm_gem_vram_unpin(pixels_current);
+	mdev->cursor.pixels_current = pixels_next;
 
-	drm_gem_vram_kunmap(pixels_prev);
- out3:
+	drm_gem_vram_kunmap(pixels_next);
+	drm_gem_vram_unpin(pixels_next);
 	drm_gem_vram_kunmap(gbo);
- out2:
-	drm_gem_vram_unlock(gbo);
- out1:
-	if (ret)
-		mga_hide_cursor(mdev);
-	drm_gem_vram_unlock(pixels_1);
-out_unlock1:
-	drm_gem_vram_unlock(pixels_2);
-out_unref:
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(obj);
 
+	return 0;
+
+err_drm_gem_vram_kunmap_dst:
+	drm_gem_vram_kunmap(pixels_next);
+err_drm_gem_vram_unpin_dst:
+	drm_gem_vram_unpin(pixels_next);
+err_drm_gem_vram_kunmap_src:
+	drm_gem_vram_kunmap(gbo);
+err_drm_gem_vram_unpin_src:
+	drm_gem_vram_unpin(gbo);
+err_drm_gem_object_put_unlocked:
+	drm_gem_object_put_unlocked(obj);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index b8f66de0d797..c47671ce6c48 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -155,11 +155,8 @@ struct mga_cursor {
 	*/
 	struct drm_gem_vram_object *pixels_1;
 	struct drm_gem_vram_object *pixels_2;
-	u64 pixels_1_gpu_addr, pixels_2_gpu_addr;
 	/* The currently displayed icon, this points to one of pixels_1, or pixels_2 */
 	struct drm_gem_vram_object *pixels_current;
-	/* The previously displayed icon */
-	struct drm_gem_vram_object *pixels_prev;
 };
 
 struct mga_mc {
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index bc95c3a18824..dd61ccc5af5c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -238,10 +238,8 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 		mdev->cursor.pixels_2 = NULL;
 		dev_warn(&dev->pdev->dev,
 			"Could not allocate space for cursors. Not doing hardware cursors.\n");
-	} else {
-		mdev->cursor.pixels_current = mdev->cursor.pixels_1;
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_2;
 	}
+	mdev->cursor.pixels_current = NULL;
 
 	return 0;
 
-- 
2.21.0

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

[PATCH v3 8/9] drm: Remove lock interfaces from GEM VRAM helpers

[Thomas Zimmermann]

The lock functions and the locked-pin/unpin functions of GEM VRAM are not
requried any longer. Remove them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 109 --------------------------
 include/drm/drm_gem_vram_helper.h     |   5 --
 2 files changed, 114 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index f3e5803affb0..4cae52054e61 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -151,36 +151,6 @@ void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_put);
 
-/**
- * drm_gem_vram_lock() - Locks a VRAM-backed GEM object
- * @gbo:	the GEM VRAM object
- * @no_wait:	don't wait for buffer object to become available
- *
- * See ttm_bo_reserve() for more information.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise
- */
-int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait)
-{
-	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
-}
-EXPORT_SYMBOL(drm_gem_vram_lock);
-
-/**
- * drm_gem_vram_unlock() - \
-	Release a reservation acquired by drm_gem_vram_lock()
- * @gbo:	the GEM VRAM object
- *
- * See ttm_bo_unreserve() for more information.
- */
-void drm_gem_vram_unlock(struct drm_gem_vram_object *gbo)
-{
-	ttm_bo_unreserve(&gbo->bo);
-}
-EXPORT_SYMBOL(drm_gem_vram_unlock);
-
 /**
  * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
  * @gbo:	the GEM VRAM object
@@ -266,49 +236,6 @@ 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_locked() - 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
- * locked. Use drm_gem_vram_pin() if possible.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo,
-			    unsigned long pl_flag)
-{
-	int i, ret;
-	struct ttm_operation_ctx ctx = { false, false };
-
-	lockdep_assert_held(&gbo->bo.resv->lock.base);
-
-	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_locked);
-
 /**
  * drm_gem_vram_unpin() - Unpins a GEM VRAM object
  * @gbo:	the GEM VRAM object
@@ -351,42 +278,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
-/**
- * drm_gem_vram_unpin_locked() - Unpins a GEM VRAM object
- * @gbo:	the GEM VRAM object
- *
- * This function unpins a GEM VRAM object that has already been
- * locked. Use drm_gem_vram_unpin() if possible.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
-{
-	int i, ret;
-	struct ttm_operation_ctx ctx = { false, false };
-
-	lockdep_assert_held(&gbo->bo.resv->lock.base);
-
-	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_locked);
-
 /**
  * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
  * @gbo:	the GEM VRAM object
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 4d1d2c1bf32b..1d4aa87f8dfa 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -77,15 +77,10 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 						unsigned long pg_align,
 						bool interruptible);
 void drm_gem_vram_put(struct drm_gem_vram_object *gbo);
-int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait);
-void drm_gem_vram_unlock(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_locked(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_locked(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);
 void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
-- 
2.21.0

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

[PATCH v3 9/9] drm: Remove functions with kmap-object argument from GEM VRAM helpers

[Thomas Zimmermann]

The GEM VRAM functions with kmap-object argument are not requried any
longer. Remove them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 50 +++++----------------------
 include/drm/drm_gem_vram_helper.h     |  4 ---
 2 files changed, 8 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 4cae52054e61..4de782ca26b2 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -279,12 +279,11 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
 /**
- * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
+ * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
  * @gbo:	the GEM VRAM object
  * @map:	establish a mapping if necessary
  * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
 	otherwise; can be NULL
- * @kmap:	the mapping's kmap object
  *
  * This function maps the buffer object into the kernel's address space
  * or returns the current mapping. If the parameter map is false, the
@@ -296,10 +295,11 @@ EXPORT_SYMBOL(drm_gem_vram_unpin);
  * NULL if not mapped, or
  * an ERR_PTR()-encoded error code otherwise.
  */
-void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
-			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap)
+void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
+			bool *is_iomem)
 {
 	int ret;
+	struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
 
 	if (kmap->virtual || !map)
 		goto out;
@@ -317,56 +317,22 @@ void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
 	}
 	return ttm_kmap_obj_virtual(kmap, is_iomem);
 }
-EXPORT_SYMBOL(drm_gem_vram_kmap_at);
-
-/**
- * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
- * @gbo:	the GEM VRAM object
- * @map:	establish a mapping if necessary
- * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
-	otherwise; can be NULL
- *
- * This function maps the buffer object into the kernel's address space
- * or returns the current mapping. If the parameter map is false, the
- * function only queries the current mapping, but does not establish a
- * new one.
- *
- * Returns:
- * The buffers virtual address if mapped, or
- * NULL if not mapped, or
- * an ERR_PTR()-encoded error code otherwise.
- */
-void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
-			bool *is_iomem)
-{
-	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
-}
 EXPORT_SYMBOL(drm_gem_vram_kmap);
 
 /**
- * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
+ * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
  * @gbo:	the GEM VRAM object
- * @kmap:	the mapping's kmap object
  */
-void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
-			    struct ttm_bo_kmap_obj *kmap)
+void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
 {
+	struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
+
 	if (!kmap->virtual)
 		return;
 
 	ttm_bo_kunmap(kmap);
 	kmap->virtual = NULL;
 }
-EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
-
-/**
- * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
- * @gbo:	the GEM VRAM object
- */
-void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
-{
-	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
-}
 EXPORT_SYMBOL(drm_gem_vram_kunmap);
 
 /**
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 1d4aa87f8dfa..9581ea0a4f7e 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -81,12 +81,8 @@ 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_unpin(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);
 void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
 			bool *is_iomem);
-void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
-			    struct ttm_bo_kmap_obj *kmap);
 void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo);
 
 int drm_gem_vram_fill_create_dumb(struct drm_file *file,
-- 
2.21.0

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

Re: [PATCH v3 0/9] Remove explicit locking and kmap arguments from GEM VRAM interface

[Gerd Hoffmann]

On Thu, Jun 13, 2019 at 09:30:32AM +0200, Thomas Zimmermann wrote:
> Drivers should not have to care about internal locking of GEM VRAM objects
> and their memory-mapping structures. This patch set removes both from the
> GEM VRAM interface.
> 
> This affects the ast and mgag200 drivers. In places where GEM objects are
> being locked by the driver, the patch converts the lock operation to a pin
> operation. The locking prevented the memory manager from moving the object,
> so pinning is more appropriate.
> 
> For the memory mappings, all book-keeping is done by the implementation
> of GEM VRAM. Explicit kmap objects are removed from the module's public
> interfaces. This change mostly affects the cursor handling in ast and
> mgag200, which is being simplified by this patch set.
> 
> Future directions: with these patches in place, more code in mode setting
> and fbdev emulation can be shared between ast and mgag200.
> 
> The patches have been tested on ast and mgag200 hardware.
> 
> v3:
> 	* document PRIME pin flags
> 	* pin cursor BOs at current location
> v2:
> 	* support pinning BOs at current location
> 	* pin PRIME buffers to current locationA

series:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

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

Re: [PATCH v3 8/9] drm: Remove lock interfaces from GEM VRAM helpers

[Daniel Vetter]

On Thu, Jun 13, 2019 at 09:30:40AM +0200, Thomas Zimmermann wrote:
> The lock functions and the locked-pin/unpin functions of GEM VRAM are not
> requried any longer. Remove them.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

btw a neat thing we could do for these helpers would be to integrate them
more tighly with the reservation/fence helpers in drm_gem.h. All those
require is that gem_obj->resv is set correctly, which duplicates ttm's own
ttm_buffer_object->resv.

It hink we could also remove ttm_buffer_object->ttm_resv and require that
you always pass one in when calling ttm_bo_init, and gem drivers would
just use the gem_object->resv one.

One complication is that for dma-buf expect we need to make sure we pick
the right one. There's kinda no neat way to solve that, except by just
making ttm_buffer_object a full subclass of drm_gem_object, which probably
wouldn't go down too well I think.

Just an aside, I'm digging around in this area in radeon/amdgpu/nouveau
code right now.
-Daniel

> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 109 --------------------------
>  include/drm/drm_gem_vram_helper.h     |   5 --
>  2 files changed, 114 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index f3e5803affb0..4cae52054e61 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -151,36 +151,6 @@ void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_put);
>  
> -/**
> - * drm_gem_vram_lock() - Locks a VRAM-backed GEM object
> - * @gbo:	the GEM VRAM object
> - * @no_wait:	don't wait for buffer object to become available
> - *
> - * See ttm_bo_reserve() for more information.
> - *
> - * Returns:
> - * 0 on success, or
> - * a negative error code otherwise
> - */
> -int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait)
> -{
> -	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
> -}
> -EXPORT_SYMBOL(drm_gem_vram_lock);
> -
> -/**
> - * drm_gem_vram_unlock() - \
> -	Release a reservation acquired by drm_gem_vram_lock()
> - * @gbo:	the GEM VRAM object
> - *
> - * See ttm_bo_unreserve() for more information.
> - */
> -void drm_gem_vram_unlock(struct drm_gem_vram_object *gbo)
> -{
> -	ttm_bo_unreserve(&gbo->bo);
> -}
> -EXPORT_SYMBOL(drm_gem_vram_unlock);
> -
>  /**
>   * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
>   * @gbo:	the GEM VRAM object
> @@ -266,49 +236,6 @@ 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_locked() - 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
> - * locked. Use drm_gem_vram_pin() if possible.
> - *
> - * Returns:
> - * 0 on success, or
> - * a negative error code otherwise.
> - */
> -int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo,
> -			    unsigned long pl_flag)
> -{
> -	int i, ret;
> -	struct ttm_operation_ctx ctx = { false, false };
> -
> -	lockdep_assert_held(&gbo->bo.resv->lock.base);
> -
> -	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_locked);
> -
>  /**
>   * drm_gem_vram_unpin() - Unpins a GEM VRAM object
>   * @gbo:	the GEM VRAM object
> @@ -351,42 +278,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_unpin);
>  
> -/**
> - * drm_gem_vram_unpin_locked() - Unpins a GEM VRAM object
> - * @gbo:	the GEM VRAM object
> - *
> - * This function unpins a GEM VRAM object that has already been
> - * locked. Use drm_gem_vram_unpin() if possible.
> - *
> - * Returns:
> - * 0 on success, or
> - * a negative error code otherwise.
> - */
> -int drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
> -{
> -	int i, ret;
> -	struct ttm_operation_ctx ctx = { false, false };
> -
> -	lockdep_assert_held(&gbo->bo.resv->lock.base);
> -
> -	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_locked);
> -
>  /**
>   * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
>   * @gbo:	the GEM VRAM object
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 4d1d2c1bf32b..1d4aa87f8dfa 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -77,15 +77,10 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
>  						unsigned long pg_align,
>  						bool interruptible);
>  void drm_gem_vram_put(struct drm_gem_vram_object *gbo);
> -int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait);
> -void drm_gem_vram_unlock(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_locked(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_locked(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);
>  void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel