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

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

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           |  59 ++++----
 drivers/gpu/drm/drm_gem_vram_helper.c    | 171 +++--------------------
 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, 146 insertions(+), 332 deletions(-)

--
2.21.0

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

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

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++----
 1 file changed, 8 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..214f54b8920b 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,7 @@ 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);
+	return drm_gem_vram_pin(gbo, 0);
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_pin);
 
@@ -723,7 +727,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 v2 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 v2 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 v2 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.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cb6e8249a7db..be3d91d7fde5 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1183,7 +1183,6 @@ 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) {
@@ -1201,15 +1200,13 @@ 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, DRM_GEM_VRAM_PL_FLAG_SYSTEM);
 	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, &src_isiomem);
 	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");
@@ -1218,14 +1215,14 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 				false, &dst_isiomem);
 	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 +1230,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 +1257,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 v2 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 v2 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 v2 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.

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..5a22ef825588 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, DRM_GEM_VRAM_PL_FLAG_SYSTEM);
 	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, DRM_GEM_VRAM_PL_FLAG_VRAM);
+	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 v2 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 214f54b8920b..f300e6a04948 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 v2 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 f300e6a04948..937774787160 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 v2 5/9] drm/ast: Pin framebuffer BO during dirty update

[Sam Ravnborg]

Hi Thomas.

On Tue, Jun 11, 2019 at 03:03:40PM +0200, Thomas Zimmermann wrote:
> 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()) {

While touching this code, anyway we could get rid of drm_can_sleep()?
I only ask because Daniel V. said that we should not use it.
This was some months ago during some ehader file clean-up, so nothing
in particular related to the ast driver.

	Sam

> +		/* 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

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

[Thomas Zimmermann]

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

Hi

Am 11.06.19 um 22:29 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Tue, Jun 11, 2019 at 03:03:40PM +0200, Thomas Zimmermann wrote:
>> 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()) {
> 
> While touching this code, anyway we could get rid of drm_can_sleep()?
> I only ask because Daniel V. said that we should not use it.
> This was some months ago during some ehader file clean-up, so nothing
> in particular related to the ast driver.

I'm aware of what is commented in the header and the todo file. Do you
have a link to this discussion?

In any case, I'd prefer to fix this in a separate patch set. I have
patches that replace the ast and mgag200 fbdev consoles with generic
code. That might be the right place.

Best regards
Thomas

> 
> 	Sam
> 
>> +		/* 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

-- 
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: 159 bytes --]

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

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

[Sam Ravnborg]

Hi Thomas.

> > 
> > While touching this code, anyway we could get rid of drm_can_sleep()?
> > I only ask because Daniel V. said that we should not use it.
> > This was some months ago during some ehader file clean-up, so nothing
> > in particular related to the ast driver.
> 
> I'm aware of what is commented in the header and the todo file. Do you
> have a link to this discussion?
I managed to dig up this link:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1887389.html
But that does not provide any additional technical details.

> In any case, I'd prefer to fix this in a separate patch set. I have
> patches that replace the ast and mgag200 fbdev consoles with generic
> code. That might be the right place.
Using generic code, and thus deleting the old code seems like a good
plan.

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

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

[Gerd Hoffmann]

On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote:
> 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.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++----
>  1 file changed, 8 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..214f54b8920b 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,7 @@ 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);
> +	return drm_gem_vram_pin(gbo, 0);

Not sure this is a good idea here.  If the bo happens to be in sysram
it can't be displayed any more.

> -	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
> +	ret = drm_gem_vram_pin(gbo, 0);

Likewise.

cheers,
  Gerd

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

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

[Gerd Hoffmann]

On Tue, Jun 11, 2019 at 03:03:39PM +0200, Thomas Zimmermann wrote:
> 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.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index cb6e8249a7db..be3d91d7fde5 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1183,7 +1183,6 @@ 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) {
> @@ -1201,15 +1200,13 @@ 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, DRM_GEM_VRAM_PL_FLAG_SYSTEM);

For a temporary pin like this one using pl_flag = 0 should be fine.

cheers,
  Gerd

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

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

[Gerd Hoffmann]

On Tue, Jun 11, 2019 at 03:03:42PM +0200, Thomas Zimmermann wrote:
> 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.
> 
> 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..5a22ef825588 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, DRM_GEM_VRAM_PL_FLAG_SYSTEM);

pl_flag = 0 should be fine here too.

cheers,
  Gerd

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

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

[Thomas Zimmermann]

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

Hi

Am 12.06.19 um 10:13 schrieb Gerd Hoffmann:
> On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote:
>> 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.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++----
>>  1 file changed, 8 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..214f54b8920b 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,7 @@ 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);
>> +	return drm_gem_vram_pin(gbo, 0);

The only use case for these Prime helpers is fbdev console emulation. I
have another patch set that replaces the ast and mgag200 consoles with
generic code. During the console updates it temporarily pins the BO via
this Prime funcation, which might move the BO into scarce VRAM
unnecessarily. Can we leave it like this and add a comment explaining
the decision?

Best regards
Thomas

> Not sure this is a good idea here.  If the bo happens to be in sysram
> it can't be displayed any more.
> 
>> -	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
>> +	ret = drm_gem_vram_pin(gbo, 0);
> 
> Likewise.
> 
> cheers,
>   Gerd
> 

-- 
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: 159 bytes --]

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

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

[Gerd Hoffmann]

On Wed, Jun 12, 2019 at 10:29:29AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.06.19 um 10:13 schrieb Gerd Hoffmann:
> > On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote:
> >> 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.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++----
> >>  1 file changed, 8 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..214f54b8920b 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,7 @@ 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);
> >> +	return drm_gem_vram_pin(gbo, 0);
> 
> The only use case for these Prime helpers is fbdev console emulation. I
> have another patch set that replaces the ast and mgag200 consoles with
> generic code. During the console updates it temporarily pins the BO via
> this Prime funcation, which might move the BO into scarce VRAM
> unnecessarily.

Ok, if the pin is temporary only for the update this should be fine.

> Can we leave it like this and add a comment explaining
> the decision?

Yes, a comment is a good idea.

cheers,
  Gerd

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