[PATCH v3 0/5] drm/ast: Place cursor BOs at VRAM high-end

[PATCH v3 0/5] drm/ast: Place cursor BOs at VRAM high-end

[Thomas Zimmermann]

(was: drm/ast/mgag200: Place cursor BOs at VRAM high-end)

This patchset cleans up the memory management of HW cursors in ast. It
further moves the allocated cursor BOs to the of the video RAM to reduce
memory fragmentation.

The ast driver manages cursor memory in a dedicated GEM VRAM buffer
object. It uses a double-buffering scheme of alternating between offsets
within the GEM BO. The code is convoluted and can lead to memory
fragmentation if the BO is stored the middle of VRAM. This is especially
a problem as ast devices only have a small amount of video memory (e.g.,
8 MiB).

With this patchset, the cursor handling in ast is first split up into
separate functions for copying cursor images, managing buffer objects,
setting scanout addresses, and moving and hiding the cursor. Furthermore,
the driver dedicates a few KiB at the high end of the device's video
memory to storing the cursor's buffer objects. This prevents memory
fragmentation.

The patchset has been tested on ast hardware.

v3:
	* split-off ast patches into separate series
	* move around ast_{show,hide}_cursor in a separate patch
	* fix space-before-tab error near AST_HWC_SIGNATURE_CHECKSUM
v2:
	* remove VRAM buffers in favor of GEM BOs
	* manage BO placement with pin flag


Thomas Zimmermann (5):
  drm/ast: Don't call ast_show_cursor() from ast_cursor_move()
  drm/ast: Move ast_{show,hide}_cursor() within source file
  drm/ast: Move cursor update code to ast_show_cursor()
  drm/ast: Move cursor offset swapping into ast_show_cursor()
  drm/ast: Allocate cursor BOs at high end of video memory

 drivers/gpu/drm/ast/ast_drv.h  |  43 +++---
 drivers/gpu/drm/ast/ast_mode.c | 235 +++++++++++++++++++--------------
 2 files changed, 158 insertions(+), 120 deletions(-)

--
2.23.0

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

[PATCH v3 1/5] drm/ast: Don't call ast_show_cursor() from ast_cursor_move()

[Thomas Zimmermann]

Separating the cursor's move() function from the show() function in
preparation of further rework of the cursor update code.

'Showing' the cursor from within the move() function is required to
update the cursor position.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6caa6ebfeaa8..a4cbf2d5ee0a 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1236,6 +1236,7 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 	struct ast_private *ast = crtc->dev->dev_private;
 	int x_offset, y_offset;
 	u8 *sig;
+	u8 jreg;
 
 	sig = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
 				false, NULL);
@@ -1262,7 +1263,9 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc7, ((y >> 8) & 0x07));
 
 	/* dummy write to fire HWC */
-	ast_show_cursor(crtc);
+	jreg = 0x02 |
+	       0x01; /* enable ARGB4444 cursor */
+	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
 
 	return 0;
 }
-- 
2.23.0

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

[PATCH v3 2/5] drm/ast: Move ast_{show, hide}_cursor() within source file

[Thomas Zimmermann]

This patch only moves around code for easier review of later patches. No
functional cahnges are made.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a4cbf2d5ee0a..5a9e6a87ea5b 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1064,23 +1064,6 @@ static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
 	kfree(i2c);
 }
 
-static void ast_show_cursor(struct drm_crtc *crtc)
-{
-	struct ast_private *ast = crtc->dev->dev_private;
-	u8 jreg;
-
-	jreg = 0x2;
-	/* enable ARGB cursor */
-	jreg |= 1;
-	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
-}
-
-static void ast_hide_cursor(struct drm_crtc *crtc)
-{
-	struct ast_private *ast = crtc->dev->dev_private;
-	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00);
-}
-
 static u32 copy_cursor_image(u8 *src, u8 *dst, int width, int height)
 {
 	union {
@@ -1137,6 +1120,23 @@ static u32 copy_cursor_image(u8 *src, u8 *dst, int width, int height)
 	return csum;
 }
 
+static void ast_show_cursor(struct drm_crtc *crtc)
+{
+	struct ast_private *ast = crtc->dev->dev_private;
+	u8 jreg;
+
+	jreg = 0x2;
+	/* enable ARGB cursor */
+	jreg |= 1;
+	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
+}
+
+static void ast_hide_cursor(struct drm_crtc *crtc)
+{
+	struct ast_private *ast = crtc->dev->dev_private;
+	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00);
+}
+
 static int ast_cursor_set(struct drm_crtc *crtc,
 			  struct drm_file *file_priv,
 			  uint32_t handle,
-- 
2.23.0

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

[PATCH v3 3/5] drm/ast: Move cursor update code to ast_show_cursor()

[Thomas Zimmermann]

A call to ast's show-cursor function now receives the cursor image
and updates the buffer. The change splits off image update and
base-address update into separate functions.

v3:
	* move ast_{show,hide}_cursor() in a previous patch

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 5a9e6a87ea5b..1294f0612fd5 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1120,20 +1120,69 @@ static u32 copy_cursor_image(u8 *src, u8 *dst, int width, int height)
 	return csum;
 }
 
-static void ast_show_cursor(struct drm_crtc *crtc)
+static int ast_cursor_update(void *dst, void *src, unsigned int width,
+			     unsigned int height)
+{
+	u32 csum;
+
+	/* do data transfer to cursor cache */
+	csum = copy_cursor_image(src, dst, width, height);
+
+	/* write checksum + signature */
+	dst += AST_HWC_SIZE;
+	writel(csum, dst);
+	writel(width, dst + AST_HWC_SIGNATURE_SizeX);
+	writel(height, dst + AST_HWC_SIGNATURE_SizeY);
+	writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTX);
+	writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY);
+
+	return 0;
+}
+
+static void ast_cursor_set_base(struct ast_private *ast, u64 address)
+{
+	u8 addr0 = (address >> 3) & 0xff;
+	u8 addr1 = (address >> 11) & 0xff;
+	u8 addr2 = (address >> 19) & 0xff;
+
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc8, addr0);
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc9, addr1);
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, addr2);
+}
+
+static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src,
+			   unsigned int width, unsigned int height,
+			   u64 dst_gpu)
 {
 	struct ast_private *ast = crtc->dev->dev_private;
+	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
+	int ret;
 	u8 jreg;
 
+	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
+
+	ret = ast_cursor_update(dst, src, width, height);
+	if (ret)
+		return ret;
+	ast_cursor_set_base(ast, dst_gpu);
+
+	ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM;
+
+	ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width;
+	ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height;
+
 	jreg = 0x2;
 	/* enable ARGB cursor */
 	jreg |= 1;
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
+
+	return 0;
 }
 
 static void ast_hide_cursor(struct drm_crtc *crtc)
 {
 	struct ast_private *ast = crtc->dev->dev_private;
+
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00);
 }
 
@@ -1144,12 +1193,9 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 			  uint32_t height)
 {
 	struct ast_private *ast = crtc->dev->dev_private;
-	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
 	s64 dst_gpu;
-	u64 gpu_addr;
-	u32 csum;
 	int ret;
 	u8 *src, *dst;
 
@@ -1185,37 +1231,9 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 		goto err_drm_gem_vram_vunmap;
 	}
 
-	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
-
-	/* do data transfer to cursor cache */
-	csum = copy_cursor_image(src, dst, width, height);
-
-	/* write checksum + signature */
-	{
-		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);
-		writel(height, dst + AST_HWC_SIGNATURE_SizeY);
-		writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTX);
-		writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY);
-
-		/* set pattern offset */
-		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->offset_x = AST_MAX_HWC_WIDTH - width;
-	ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height;
-
-	ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM;
-
-	ast_show_cursor(crtc);
+	ret = ast_show_cursor(crtc, dst, src, width, height, dst_gpu);
+	if (ret)
+		goto err_drm_gem_vram_kunmap;
 
 	drm_gem_vram_vunmap(gbo, src);
 	drm_gem_object_put_unlocked(obj);
-- 
2.23.0

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

[PATCH v3 4/5] drm/ast: Move cursor offset swapping into ast_show_cursor()

[Thomas Zimmermann]

Selecting the correct offset for the new cursor image is not relevant
outside of ast_show_cursor(). Let the function do the work.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 1294f0612fd5..6a517ffb1c5c 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1150,23 +1150,34 @@ static void ast_cursor_set_base(struct ast_private *ast, u64 address)
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, addr2);
 }
 
-static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src,
-			   unsigned int width, unsigned int height,
-			   u64 dst_gpu)
+static int ast_show_cursor(struct drm_crtc *crtc, void *src,
+			   unsigned int width, unsigned int height)
 {
 	struct ast_private *ast = crtc->dev->dev_private;
 	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
+	struct drm_gem_vram_object *gbo;
+	u8 *dst, *dst_next;
+	s64 off;
 	int ret;
 	u8 jreg;
 
-	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
+	gbo = drm_gem_vram_of_gem(ast->cursor_cache);
+	dst = drm_gem_vram_vmap(gbo);
+	if (IS_ERR(dst))
+		return PTR_ERR(dst);
+	off = drm_gem_vram_offset(gbo);
+	if (off < 0) {
+		ret = (int)off;
+		goto err_drm_gem_vram_vunmap;
+	}
 
-	ret = ast_cursor_update(dst, src, width, height);
-	if (ret)
-		return ret;
-	ast_cursor_set_base(ast, dst_gpu);
+	dst_next = dst + (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) *
+		   ast->next_cursor;
 
-	ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM;
+	ret = ast_cursor_update(dst_next, src, width, height);
+	if (ret)
+		goto err_drm_gem_vram_vunmap;
+	ast_cursor_set_base(ast, off);
 
 	ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width;
 	ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height;
@@ -1176,7 +1187,15 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src,
 	jreg |= 1;
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
 
+	ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM;
+
+	drm_gem_vram_vunmap(gbo, dst);
+
 	return 0;
+
+err_drm_gem_vram_vunmap:
+	drm_gem_vram_vunmap(gbo, dst);
+	return ret;
 }
 
 static void ast_hide_cursor(struct drm_crtc *crtc)
@@ -1192,12 +1211,10 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 			  uint32_t width,
 			  uint32_t height)
 {
-	struct ast_private *ast = crtc->dev->dev_private;
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
-	s64 dst_gpu;
+	u8 *src;
 	int ret;
-	u8 *src, *dst;
 
 	if (!handle) {
 		ast_hide_cursor(crtc);
@@ -1219,21 +1236,9 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 		goto err_drm_gem_object_put_unlocked;
 	}
 
-	dst = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
-				false, NULL);
-	if (IS_ERR(dst)) {
-		ret = PTR_ERR(dst);
-		goto err_drm_gem_vram_vunmap;
-	}
-	dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache));
-	if (dst_gpu < 0) {
-		ret = (int)dst_gpu;
-		goto err_drm_gem_vram_vunmap;
-	}
-
-	ret = ast_show_cursor(crtc, dst, src, width, height, dst_gpu);
+	ret = ast_show_cursor(crtc, src, width, height);
 	if (ret)
-		goto err_drm_gem_vram_kunmap;
+		goto err_drm_gem_vram_vunmap;
 
 	drm_gem_vram_vunmap(gbo, src);
 	drm_gem_object_put_unlocked(obj);
-- 
2.23.0

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

[PATCH v3 5/5] drm/ast: Allocate cursor BOs at high end of video memory

[Thomas Zimmermann]

By putting cursor BOs at the high end of the video memory, we can avoid
memory fragmentation. Starting at the low end, contiguous video memory is
available for framebuffers.

The patch also simplifies the buffer swapping by splitting
struct ast_private.cursor_cache BO into two separate boffer objects. Cursor
images alternate between these buffers instead of offsets within cursor_cache.

v3:
	* fixes space-before-tab error near AST_HWC_SIGNATURE_CHECKSUM

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 244cc7c382af..ff161bd622f3 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -82,6 +82,25 @@ enum ast_tx_chip {
 #define AST_DRAM_4Gx16   7
 #define AST_DRAM_8Gx16   8
 
+
+#define AST_MAX_HWC_WIDTH	64
+#define AST_MAX_HWC_HEIGHT	64
+
+#define AST_HWC_SIZE		(AST_MAX_HWC_WIDTH * AST_MAX_HWC_HEIGHT * 2)
+#define AST_HWC_SIGNATURE_SIZE	32
+
+#define AST_DEFAULT_HWC_NUM	2
+
+/* define for signature structure */
+#define AST_HWC_SIGNATURE_CHECKSUM	0x00
+#define AST_HWC_SIGNATURE_SizeX		0x04
+#define AST_HWC_SIGNATURE_SizeY		0x08
+#define AST_HWC_SIGNATURE_X		0x0C
+#define AST_HWC_SIGNATURE_Y		0x10
+#define AST_HWC_SIGNATURE_HOTSPOTX	0x14
+#define AST_HWC_SIGNATURE_HOTSPOTY	0x18
+
+
 struct ast_private {
 	struct drm_device *dev;
 
@@ -97,8 +116,11 @@ struct ast_private {
 
 	int fb_mtrr;
 
-	struct drm_gem_object *cursor_cache;
-	int next_cursor;
+	struct {
+		struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM];
+		unsigned int next_index;
+	} cursor;
+
 	bool support_wide_screen;
 	enum {
 		ast_use_p2a,
@@ -199,23 +221,6 @@ static inline void ast_open_key(struct ast_private *ast)
 
 #define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
 
-#define AST_MAX_HWC_WIDTH 64
-#define AST_MAX_HWC_HEIGHT 64
-
-#define AST_HWC_SIZE                (AST_MAX_HWC_WIDTH*AST_MAX_HWC_HEIGHT*2)
-#define AST_HWC_SIGNATURE_SIZE      32
-
-#define AST_DEFAULT_HWC_NUM 2
-/* define for signature structure */
-#define AST_HWC_SIGNATURE_CHECKSUM  0x00
-#define AST_HWC_SIGNATURE_SizeX     0x04
-#define AST_HWC_SIGNATURE_SizeY     0x08
-#define AST_HWC_SIGNATURE_X         0x0C
-#define AST_HWC_SIGNATURE_Y         0x10
-#define AST_HWC_SIGNATURE_HOTSPOTX  0x14
-#define AST_HWC_SIGNATURE_HOTSPOTY  0x18
-
-
 struct ast_i2c_chan {
 	struct i2c_adapter adapter;
 	struct drm_device *dev;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6a517ffb1c5c..b13eaa2619ab 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -883,50 +883,53 @@ static int ast_connector_init(struct drm_device *dev)
 static int ast_cursor_init(struct drm_device *dev)
 {
 	struct ast_private *ast = dev->dev_private;
-	int size;
-	int ret;
-	struct drm_gem_object *obj;
+	size_t size, i;
 	struct drm_gem_vram_object *gbo;
-	s64 gpu_addr;
-	void *base;
+	int ret;
 
-	size = (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) * AST_DEFAULT_HWC_NUM;
+	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
 
-	ret = ast_gem_create(dev, size, true, &obj);
-	if (ret)
-		return ret;
-	gbo = drm_gem_vram_of_gem(obj);
-	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
-	if (ret)
-		goto fail;
-	gpu_addr = drm_gem_vram_offset(gbo);
-	if (gpu_addr < 0) {
-		drm_gem_vram_unpin(gbo);
-		ret = (int)gpu_addr;
-		goto fail;
-	}
+	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
+		gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev,
+					  size, 0, false);
+		if (IS_ERR(gbo)) {
+			ret = PTR_ERR(gbo);
+			goto err_drm_gem_vram_put;
+		}
+		ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
+					    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
+		if (ret) {
+			drm_gem_vram_put(gbo);
+			goto err_drm_gem_vram_put;
+		}
 
-	/* kmap the object */
-	base = drm_gem_vram_kmap(gbo, true, NULL);
-	if (IS_ERR(base)) {
-		ret = PTR_ERR(base);
-		goto fail;
+		ast->cursor.gbo[i] = gbo;
 	}
 
-	ast->cursor_cache = obj;
 	return 0;
-fail:
+
+err_drm_gem_vram_put:
+	while (i) {
+		--i;
+		gbo = ast->cursor.gbo[i];
+		drm_gem_vram_unpin(gbo);
+		drm_gem_vram_put(gbo);
+		ast->cursor.gbo[i] = NULL;
+	}
 	return ret;
 }
 
 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(gbo);
-	drm_gem_vram_unpin(gbo);
-	drm_gem_object_put_unlocked(ast->cursor_cache);
+	size_t i;
+	struct drm_gem_vram_object *gbo;
+
+	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
+		gbo = ast->cursor.gbo[i];
+		drm_gem_vram_unpin(gbo);
+		drm_gem_vram_put(gbo);
+	}
 }
 
 int ast_mode_init(struct drm_device *dev)
@@ -1156,12 +1159,12 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src,
 	struct ast_private *ast = crtc->dev->dev_private;
 	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
 	struct drm_gem_vram_object *gbo;
-	u8 *dst, *dst_next;
+	void *dst;
 	s64 off;
 	int ret;
 	u8 jreg;
 
-	gbo = drm_gem_vram_of_gem(ast->cursor_cache);
+	gbo = ast->cursor.gbo[ast->cursor.next_index];
 	dst = drm_gem_vram_vmap(gbo);
 	if (IS_ERR(dst))
 		return PTR_ERR(dst);
@@ -1171,10 +1174,7 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src,
 		goto err_drm_gem_vram_vunmap;
 	}
 
-	dst_next = dst + (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) *
-		   ast->next_cursor;
-
-	ret = ast_cursor_update(dst_next, src, width, height);
+	ret = ast_cursor_update(dst, src, width, height);
 	if (ret)
 		goto err_drm_gem_vram_vunmap;
 	ast_cursor_set_base(ast, off);
@@ -1187,7 +1187,8 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src,
 	jreg |= 1;
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
 
-	ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM;
+	++ast->cursor.next_index;
+	ast->cursor.next_index %= ARRAY_SIZE(ast->cursor.gbo);
 
 	drm_gem_vram_vunmap(gbo, dst);
 
@@ -1257,13 +1258,17 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 {
 	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
 	struct ast_private *ast = crtc->dev->dev_private;
+	struct drm_gem_vram_object *gbo;
 	int x_offset, y_offset;
-	u8 *sig;
+	u8 *dst, *sig;
 	u8 jreg;
 
-	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;
+	gbo = ast->cursor.gbo[ast->cursor.next_index];
+	dst = drm_gem_vram_vmap(gbo);
+	if (IS_ERR(dst))
+		return PTR_ERR(dst);
+
+	sig = dst + AST_HWC_SIZE;
 	writel(x, sig + AST_HWC_SIGNATURE_X);
 	writel(y, sig + AST_HWC_SIGNATURE_Y);
 
@@ -1290,5 +1295,7 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 	       0x01; /* enable ARGB4444 cursor */
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
 
+	drm_gem_vram_vunmap(gbo, dst);
+
 	return 0;
 }
-- 
2.23.0

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

Re: [PATCH v3 0/5] drm/ast: Place cursor BOs at VRAM high-end

[Gerd Hoffmann]

On Fri, Sep 27, 2019 at 11:03:04AM +0200, Thomas Zimmermann wrote:
> (was: drm/ast/mgag200: Place cursor BOs at VRAM high-end)
> 
> This patchset cleans up the memory management of HW cursors in ast. It
> further moves the allocated cursor BOs to the of the video RAM to reduce
> memory fragmentation.
> 
> The ast driver manages cursor memory in a dedicated GEM VRAM buffer
> object. It uses a double-buffering scheme of alternating between offsets
> within the GEM BO. The code is convoluted and can lead to memory
> fragmentation if the BO is stored the middle of VRAM. This is especially
> a problem as ast devices only have a small amount of video memory (e.g.,
> 8 MiB).
> 
> With this patchset, the cursor handling in ast is first split up into
> separate functions for copying cursor images, managing buffer objects,
> setting scanout addresses, and moving and hiding the cursor. Furthermore,
> the driver dedicates a few KiB at the high end of the device's video
> memory to storing the cursor's buffer objects. This prevents memory
> fragmentation.
> 
> The patchset has been tested on ast hardware.
> 
> v3:
> 	* split-off ast patches into separate series
> 	* move around ast_{show,hide}_cursor in a separate patch
> 	* fix space-before-tab error near AST_HWC_SIGNATURE_CHECKSUM
> v2:
> 	* remove VRAM buffers in favor of GEM BOs
> 	* manage BO placement with pin flag

Looks all sane to me, series is
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

> 
> 
> Thomas Zimmermann (5):
>   drm/ast: Don't call ast_show_cursor() from ast_cursor_move()
>   drm/ast: Move ast_{show,hide}_cursor() within source file
>   drm/ast: Move cursor update code to ast_show_cursor()
>   drm/ast: Move cursor offset swapping into ast_show_cursor()
>   drm/ast: Allocate cursor BOs at high end of video memory
> 
>  drivers/gpu/drm/ast/ast_drv.h  |  43 +++---
>  drivers/gpu/drm/ast/ast_mode.c | 235 +++++++++++++++++++--------------
>  2 files changed, 158 insertions(+), 120 deletions(-)
> 
> --
> 2.23.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/5] drm/ast: Place cursor BOs at VRAM high-end

[Thomas Zimmermann]

Hi

Am 02.10.19 um 10:59 schrieb Gerd Hoffmann:
> On Fri, Sep 27, 2019 at 11:03:04AM +0200, Thomas Zimmermann wrote:
>> (was: drm/ast/mgag200: Place cursor BOs at VRAM high-end)
>>
>> This patchset cleans up the memory management of HW cursors in ast. It
>> further moves the allocated cursor BOs to the of the video RAM to reduce
>> memory fragmentation.
>>
>> The ast driver manages cursor memory in a dedicated GEM VRAM buffer
>> object. It uses a double-buffering scheme of alternating between offsets
>> within the GEM BO. The code is convoluted and can lead to memory
>> fragmentation if the BO is stored the middle of VRAM. This is especially
>> a problem as ast devices only have a small amount of video memory (e.g.,
>> 8 MiB).
>>
>> With this patchset, the cursor handling in ast is first split up into
>> separate functions for copying cursor images, managing buffer objects,
>> setting scanout addresses, and moving and hiding the cursor. Furthermore,
>> the driver dedicates a few KiB at the high end of the device's video
>> memory to storing the cursor's buffer objects. This prevents memory
>> fragmentation.
>>
>> The patchset has been tested on ast hardware.
>>
>> v3:
>> 	* split-off ast patches into separate series
>> 	* move around ast_{show,hide}_cursor in a separate patch
>> 	* fix space-before-tab error near AST_HWC_SIGNATURE_CHECKSUM
>> v2:
>> 	* remove VRAM buffers in favor of GEM BOs
>> 	* manage BO placement with pin flag
> 
> Looks all sane to me, series is
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Thanks for taking the time to review patches for this fairly obscure code.

Best regards
Thomas

> 
>>
>>
>> Thomas Zimmermann (5):
>>    drm/ast: Don't call ast_show_cursor() from ast_cursor_move()
>>    drm/ast: Move ast_{show,hide}_cursor() within source file
>>    drm/ast: Move cursor update code to ast_show_cursor()
>>    drm/ast: Move cursor offset swapping into ast_show_cursor()
>>    drm/ast: Allocate cursor BOs at high end of video memory
>>
>>   drivers/gpu/drm/ast/ast_drv.h  |  43 +++---
>>   drivers/gpu/drm/ast/ast_mode.c | 235 +++++++++++++++++++--------------
>>   2 files changed, 158 insertions(+), 120 deletions(-)
>>
>> --
>> 2.23.0
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel