[PATCH v2 0/7] drm/ast: Various changes for video memory

[PATCH v2 0/7] drm/ast: Various changes for video memory

[Thomas Zimmermann]

Various changes to detecting and organizing video memory on Aspeed
hardware. Resolves code duplication and cruft.

Patches 1 to 3 do a number of cleanups to the code for detecting
the size of the video memory. Patches 4 to 7 provide helpers for
calculating framebuffer and cursor locations within the video
memory.

Tested on AST2600 hardware.

v2:
- initialize cursor-plane size

Thomas Zimmermann (7):
  drm/ast: Replace AST_VIDMEM_SIZE_ with Linux SZ_ constants
  drm/ast: Add VGACRAA register constants
  drm/ast: Add VGACR99 register constants
  drm/ast: cursor: Add helpers for computing location in video memory
  drm/ast: Add helper for computing framebuffer location in video memory
  drm/ast: Remove vram_fb_available from struct ast_device
  drm/ast: cursor: Drop page alignment

 drivers/gpu/drm/ast/ast_cursor.c | 39 +++++++++++++++++++-------------
 drivers/gpu/drm/ast/ast_drv.h    | 13 +----------
 drivers/gpu/drm/ast/ast_mm.c     | 26 ++++++++++-----------
 drivers/gpu/drm/ast/ast_mode.c   | 25 ++++++++++++++++----
 drivers/gpu/drm/ast/ast_post.c   | 24 ++++++++++----------
 drivers/gpu/drm/ast/ast_reg.h    |  2 ++
 6 files changed, 71 insertions(+), 58 deletions(-)

-- 
2.48.1

[PATCH v2 1/7] drm/ast: Replace AST_VIDMEM_SIZE_ with Linux SZ_ constants

[Thomas Zimmermann]

Ast's AST_VIDMEM_SIZE_ constants enumerate supported video-memory
sizes from 8 MiB to 128 MiB. Replace them with Linux' SZ_ constants
of the same value. When expanded, the literal values remain the same.

The size constant for 128 MiB is unused and the default size is not
necessary. Remove both of them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  8 --------
 drivers/gpu/drm/ast/ast_mm.c   |  9 ++++-----
 drivers/gpu/drm/ast/ast_post.c | 24 ++++++++++++------------
 3 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index d2c2605d2728..2c7861835cfb 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -340,14 +340,6 @@ static inline void ast_set_index_reg_mask(struct ast_device *ast, u32 base, u8 i
 	__ast_write8_i_masked(ast->ioregs, base, index, preserve_mask, val);
 }
 
-#define AST_VIDMEM_SIZE_8M    0x00800000
-#define AST_VIDMEM_SIZE_16M   0x01000000
-#define AST_VIDMEM_SIZE_32M   0x02000000
-#define AST_VIDMEM_SIZE_64M   0x04000000
-#define AST_VIDMEM_SIZE_128M  0x08000000
-
-#define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
-
 struct ast_vbios_stdtable {
 	u8 misc;
 	u8 seq[4];
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 6dfe6d9777d4..20d833632a01 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -38,20 +38,19 @@ static u32 ast_get_vram_size(struct ast_device *ast)
 	u8 jreg;
 	u32 vram_size;
 
-	vram_size = AST_VIDMEM_DEFAULT_SIZE;
 	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xaa, 0xff);
 	switch (jreg & 3) {
 	case 0:
-		vram_size = AST_VIDMEM_SIZE_8M;
+		vram_size = SZ_8M;
 		break;
 	case 1:
-		vram_size = AST_VIDMEM_SIZE_16M;
+		vram_size = SZ_16M;
 		break;
 	case 2:
-		vram_size = AST_VIDMEM_SIZE_32M;
+		vram_size = SZ_32M;
 		break;
 	case 3:
-		vram_size = AST_VIDMEM_SIZE_64M;
+		vram_size = SZ_64M;
 		break;
 	}
 
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 91e85e457bdf..37568cf3822c 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -1075,16 +1075,16 @@ static void get_ddr3_info(struct ast_device *ast, struct ast2300_dram_param *par
 
 	switch (param->vram_size) {
 	default:
-	case AST_VIDMEM_SIZE_8M:
+	case SZ_8M:
 		param->dram_config |= 0x00;
 		break;
-	case AST_VIDMEM_SIZE_16M:
+	case SZ_16M:
 		param->dram_config |= 0x04;
 		break;
-	case AST_VIDMEM_SIZE_32M:
+	case SZ_32M:
 		param->dram_config |= 0x08;
 		break;
-	case AST_VIDMEM_SIZE_64M:
+	case SZ_64M:
 		param->dram_config |= 0x0c;
 		break;
 	}
@@ -1446,16 +1446,16 @@ static void get_ddr2_info(struct ast_device *ast, struct ast2300_dram_param *par
 
 	switch (param->vram_size) {
 	default:
-	case AST_VIDMEM_SIZE_8M:
+	case SZ_8M:
 		param->dram_config |= 0x00;
 		break;
-	case AST_VIDMEM_SIZE_16M:
+	case SZ_16M:
 		param->dram_config |= 0x04;
 		break;
-	case AST_VIDMEM_SIZE_32M:
+	case SZ_32M:
 		param->dram_config |= 0x08;
 		break;
-	case AST_VIDMEM_SIZE_64M:
+	case SZ_64M:
 		param->dram_config |= 0x0c;
 		break;
 	}
@@ -1635,19 +1635,19 @@ static void ast_post_chip_2300(struct ast_device *ast)
                 switch (temp & 0x0c) {
                 default:
 		case 0x00:
-			param.vram_size = AST_VIDMEM_SIZE_8M;
+			param.vram_size = SZ_8M;
 			break;
 
 		case 0x04:
-			param.vram_size = AST_VIDMEM_SIZE_16M;
+			param.vram_size = SZ_16M;
 			break;
 
 		case 0x08:
-			param.vram_size = AST_VIDMEM_SIZE_32M;
+			param.vram_size = SZ_32M;
 			break;
 
 		case 0x0c:
-			param.vram_size = AST_VIDMEM_SIZE_64M;
+			param.vram_size = SZ_64M;
 			break;
 		}
 
-- 
2.48.1

[PATCH v2 2/7] drm/ast: Add VGACRAA register constants

[Thomas Zimmermann]

Add register constants for VGACRAA and use them when detecting the
size of the VGA memory. Aligns the code with the programming manual.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mm.c  | 5 +++--
 drivers/gpu/drm/ast/ast_reg.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 20d833632a01..8d8aac8c0814 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -37,9 +37,10 @@ static u32 ast_get_vram_size(struct ast_device *ast)
 {
 	u8 jreg;
 	u32 vram_size;
+	u8 vgacraa;
 
-	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xaa, 0xff);
-	switch (jreg & 3) {
+	vgacraa = ast_get_index_reg(ast, AST_IO_VGACRI, 0xaa);
+	switch (vgacraa & AST_IO_VGACRAA_VGAMEM_SIZE_MASK) {
 	case 0:
 		vram_size = SZ_8M;
 		break;
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index bb2cc1d8b84e..039b93bed19e 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -33,6 +33,7 @@
 #define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
 #define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
 #define AST_IO_VGACRA3_DVO_ENABLED	BIT(7)
+#define AST_IO_VGACRAA_VGAMEM_SIZE_MASK	GENMASK(1, 0)
 #define AST_IO_VGACRB6_HSYNC_OFF	BIT(0)
 #define AST_IO_VGACRB6_VSYNC_OFF	BIT(1)
 #define AST_IO_VGACRCB_HWC_16BPP	BIT(0) /* set: ARGB4444, cleared: 2bpp palette */
-- 
2.48.1

[PATCH v2 3/7] drm/ast: Add VGACR99 register constants

[Thomas Zimmermann]

Add register constants for VGACR99 and use them when detecting the
size of the VGA memory. Aligns the code with the programming manual.
Also replace literal size values with Linux' SZ_ size constants.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mm.c  | 13 ++++++-------
 drivers/gpu/drm/ast/ast_reg.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 8d8aac8c0814..3d03ef556d0a 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -35,9 +35,8 @@
 
 static u32 ast_get_vram_size(struct ast_device *ast)
 {
-	u8 jreg;
 	u32 vram_size;
-	u8 vgacraa;
+	u8 vgacr99, vgacraa;
 
 	vgacraa = ast_get_index_reg(ast, AST_IO_VGACRI, 0xaa);
 	switch (vgacraa & AST_IO_VGACRAA_VGAMEM_SIZE_MASK) {
@@ -55,16 +54,16 @@ static u32 ast_get_vram_size(struct ast_device *ast)
 		break;
 	}
 
-	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0x99, 0xff);
-	switch (jreg & 0x03) {
+	vgacr99 = ast_get_index_reg(ast, AST_IO_VGACRI, 0x99);
+	switch (vgacr99 & AST_IO_VGACR99_VGAMEM_RSRV_MASK) {
 	case 1:
-		vram_size -= 0x100000;
+		vram_size -= SZ_1M;
 		break;
 	case 2:
-		vram_size -= 0x200000;
+		vram_size -= SZ_2M;
 		break;
 	case 3:
-		vram_size -= 0x400000;
+		vram_size -= SZ_4M;
 		break;
 	}
 
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index 039b93bed19e..e15adaf3a80e 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -30,6 +30,7 @@
 
 #define AST_IO_VGACRI			(0x54)
 #define AST_IO_VGACR80_PASSWORD		(0xa8)
+#define AST_IO_VGACR99_VGAMEM_RSRV_MASK	GENMASK(1, 0)
 #define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
 #define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
 #define AST_IO_VGACRA3_DVO_ENABLED	BIT(7)
-- 
2.48.1

[PATCH v2 4/7] drm/ast: cursor: Add helpers for computing location in video memory

[Thomas Zimmermann]

The ast drivers stores the cursor image at the end of the video memory.
Add helpers to calculate the offset and size.

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

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 139ab00dee8f..05e297f30b4e 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -45,6 +45,21 @@
 #define AST_HWC_SIGNATURE_HOTSPOTX	0x14
 #define AST_HWC_SIGNATURE_HOTSPOTY	0x18
 
+static unsigned long ast_cursor_vram_size(void)
+{
+	return AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE;
+}
+
+long ast_cursor_vram_offset(struct ast_device *ast)
+{
+	unsigned long size = ast_cursor_vram_size();
+
+	if (size > ast->vram_size)
+		return -EINVAL;
+
+	return PAGE_ALIGN_DOWN(ast->vram_size - size);
+}
+
 static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, unsigned int height)
 {
 	u32 csum = 0;
@@ -276,7 +291,7 @@ int ast_cursor_plane_init(struct ast_device *ast)
 	struct drm_plane *cursor_plane = &ast_plane->base;
 	size_t size;
 	void __iomem *vaddr;
-	u64 offset;
+	long offset;
 	int ret;
 
 	/*
@@ -290,7 +305,9 @@ int ast_cursor_plane_init(struct ast_device *ast)
 		return -ENOMEM;
 
 	vaddr = ast->vram + ast->vram_fb_available - size;
-	offset = ast->vram_fb_available - size;
+	offset = ast_cursor_vram_offset(ast);
+	if (offset < 0)
+		return offset;
 
 	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
 			     0x01, &ast_cursor_plane_funcs,
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 2c7861835cfb..ec9ec77260e9 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -432,6 +432,7 @@ int ast_vga_output_init(struct ast_device *ast);
 int ast_sil164_output_init(struct ast_device *ast);
 
 /* ast_cursor.c */
+long ast_cursor_vram_offset(struct ast_device *ast);
 int ast_cursor_plane_init(struct ast_device *ast);
 
 /* ast dp501 */
-- 
2.48.1

[PATCH v2 5/7] drm/ast: Add helper for computing framebuffer location in video memory

[Thomas Zimmermann]

The ast driver stores the primary plane's image in the framebuffer
memory up to where the cursor is located. Add helpers to calculate
the offset and size.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index c3b950675485..4cac5c7f4547 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -51,6 +51,24 @@
 
 #define AST_LUT_SIZE 256
 
+static unsigned long ast_fb_vram_offset(void)
+{
+	return 0; // with shmem, the primary plane is always at offset 0
+}
+
+static unsigned long ast_fb_vram_size(struct ast_device *ast)
+{
+	struct drm_device *dev = &ast->base;
+	unsigned long offset = ast_fb_vram_offset(); // starts at offset
+	long cursor_offset = ast_cursor_vram_offset(ast); // ends at cursor offset
+
+	if (cursor_offset < 0)
+		cursor_offset = ast->vram_size; // no cursor; it's all ours
+	if (drm_WARN_ON_ONCE(dev, offset > cursor_offset))
+		return 0; // cannot legally happen; signal error
+	return cursor_offset - offset;
+}
+
 static inline void ast_load_palette_index(struct ast_device *ast,
 				     u8 index, u8 red, u8 green,
 				     u8 blue)
@@ -609,9 +627,8 @@ static int ast_primary_plane_init(struct ast_device *ast)
 	struct ast_plane *ast_primary_plane = &ast->primary_plane;
 	struct drm_plane *primary_plane = &ast_primary_plane->base;
 	void __iomem *vaddr = ast->vram;
-	u64 offset = 0; /* with shmem, the primary plane is always at offset 0 */
-	unsigned long cursor_size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
-	unsigned long size = ast->vram_fb_available - cursor_size;
+	u64 offset = ast_fb_vram_offset();
+	unsigned long size = ast_fb_vram_size(ast);
 	int ret;
 
 	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
@@ -942,7 +959,7 @@ static enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
 	struct ast_device *ast = to_ast_device(dev);
 	unsigned long fbsize, fbpages, max_fbpages;
 
-	max_fbpages = (ast->vram_fb_available) >> PAGE_SHIFT;
+	max_fbpages = ast_fb_vram_size(ast) >> PAGE_SHIFT;
 
 	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
 	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
-- 
2.48.1

[PATCH v2 6/7] drm/ast: Remove vram_fb_available from struct ast_device

[Thomas Zimmermann]

Helpers compute the offset and size of the available framebuffer
memory. Remove the obsolete field vram_fb_available from struct
ast_device. Also define the cursor-signature size next to its only
user.

v2:
- initialize plane size

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c | 18 ++++--------------
 drivers/gpu/drm/ast/ast_drv.h    |  4 ----
 drivers/gpu/drm/ast/ast_mm.c     |  1 -
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 05e297f30b4e..cb0c48d47207 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -37,6 +37,7 @@
  */
 
 /* define for signature structure */
+#define AST_HWC_SIGNATURE_SIZE		SZ_32
 #define AST_HWC_SIGNATURE_CHECKSUM	0x00
 #define AST_HWC_SIGNATURE_SizeX		0x04
 #define AST_HWC_SIGNATURE_SizeY		0x08
@@ -289,25 +290,16 @@ int ast_cursor_plane_init(struct ast_device *ast)
 	struct ast_cursor_plane *ast_cursor_plane = &ast->cursor_plane;
 	struct ast_plane *ast_plane = &ast_cursor_plane->base;
 	struct drm_plane *cursor_plane = &ast_plane->base;
-	size_t size;
+	unsigned long size;
 	void __iomem *vaddr;
 	long offset;
 	int ret;
 
-	/*
-	 * Allocate backing storage for cursors. The BOs are permanently
-	 * pinned to the top end of the VRAM.
-	 */
-
-	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
-
-	if (ast->vram_fb_available < size)
-		return -ENOMEM;
-
-	vaddr = ast->vram + ast->vram_fb_available - size;
+	size = ast_cursor_vram_size();
 	offset = ast_cursor_vram_offset(ast);
 	if (offset < 0)
 		return offset;
+	vaddr = ast->vram + offset;
 
 	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
 			     0x01, &ast_cursor_plane_funcs,
@@ -320,7 +312,5 @@ int ast_cursor_plane_init(struct ast_device *ast)
 	drm_plane_helper_add(cursor_plane, &ast_cursor_plane_helper_funcs);
 	drm_plane_enable_fb_damage_clips(cursor_plane);
 
-	ast->vram_fb_available -= size;
-
 	return 0;
 }
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index ec9ec77260e9..d9da2328d46b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -112,12 +112,9 @@ enum ast_config_mode {
 
 #define AST_MAX_HWC_WIDTH	64
 #define AST_MAX_HWC_HEIGHT	64
-
 #define AST_HWC_PITCH		(AST_MAX_HWC_WIDTH * SZ_2)
 #define AST_HWC_SIZE		(AST_MAX_HWC_HEIGHT * AST_HWC_PITCH)
 
-#define AST_HWC_SIGNATURE_SIZE	32
-
 /*
  * Planes
  */
@@ -183,7 +180,6 @@ struct ast_device {
 	void __iomem	*vram;
 	unsigned long	vram_base;
 	unsigned long	vram_size;
-	unsigned long	vram_fb_available;
 
 	struct mutex modeset_lock; /* Protects access to modeset I/O registers in ioregs */
 
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 3d03ef556d0a..0bc140319464 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -92,7 +92,6 @@ int ast_mm_init(struct ast_device *ast)
 
 	ast->vram_base = base;
 	ast->vram_size = vram_size;
-	ast->vram_fb_available = vram_size;
 
 	return 0;
 }
-- 
2.48.1

[PATCH v2 7/7] drm/ast: cursor: Drop page alignment

[Thomas Zimmermann]

The cursor scanout address requires alignment to a multiple of 8,
but does not require page alignment. Change the offset calculation
accordingly. Frees up a few more bytes for the primary framebuffer.

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

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index cb0c48d47207..5ee724bfd682 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -58,7 +58,7 @@ long ast_cursor_vram_offset(struct ast_device *ast)
 	if (size > ast->vram_size)
 		return -EINVAL;
 
-	return PAGE_ALIGN_DOWN(ast->vram_size - size);
+	return ALIGN_DOWN(ast->vram_size - size, SZ_8);
 }
 
 static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, unsigned int height)
-- 
2.48.1

Re: [PATCH v2 1/7] drm/ast: Replace AST_VIDMEM_SIZE_ with Linux SZ_ constants

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> Ast's AST_VIDMEM_SIZE_ constants enumerate supported video-memory
> sizes from 8 MiB to 128 MiB. Replace them with Linux' SZ_ constants
> of the same value. When expanded, the literal values remain the same.
> 
> The size constant for 128 MiB is unused and the default size is not
> necessary. Remove both of them.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  8 --------
>   drivers/gpu/drm/ast/ast_mm.c   |  9 ++++-----
>   drivers/gpu/drm/ast/ast_post.c | 24 ++++++++++++------------
>   3 files changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index d2c2605d2728..2c7861835cfb 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -340,14 +340,6 @@ static inline void ast_set_index_reg_mask(struct ast_device *ast, u32 base, u8 i
>   	__ast_write8_i_masked(ast->ioregs, base, index, preserve_mask, val);
>   }
>   
> -#define AST_VIDMEM_SIZE_8M    0x00800000
> -#define AST_VIDMEM_SIZE_16M   0x01000000
> -#define AST_VIDMEM_SIZE_32M   0x02000000
> -#define AST_VIDMEM_SIZE_64M   0x04000000
> -#define AST_VIDMEM_SIZE_128M  0x08000000
> -
> -#define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
> -
>   struct ast_vbios_stdtable {
>   	u8 misc;
>   	u8 seq[4];
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 6dfe6d9777d4..20d833632a01 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -38,20 +38,19 @@ static u32 ast_get_vram_size(struct ast_device *ast)
>   	u8 jreg;
>   	u32 vram_size;
>   
> -	vram_size = AST_VIDMEM_DEFAULT_SIZE;
>   	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xaa, 0xff);
>   	switch (jreg & 3) {
>   	case 0:
> -		vram_size = AST_VIDMEM_SIZE_8M;
> +		vram_size = SZ_8M;
>   		break;
>   	case 1:
> -		vram_size = AST_VIDMEM_SIZE_16M;
> +		vram_size = SZ_16M;
>   		break;
>   	case 2:
> -		vram_size = AST_VIDMEM_SIZE_32M;
> +		vram_size = SZ_32M;
>   		break;
>   	case 3:
> -		vram_size = AST_VIDMEM_SIZE_64M;
> +		vram_size = SZ_64M;
>   		break;
>   	}
>   
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 91e85e457bdf..37568cf3822c 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -1075,16 +1075,16 @@ static void get_ddr3_info(struct ast_device *ast, struct ast2300_dram_param *par
>   
>   	switch (param->vram_size) {
>   	default:
> -	case AST_VIDMEM_SIZE_8M:
> +	case SZ_8M:
>   		param->dram_config |= 0x00;
>   		break;
> -	case AST_VIDMEM_SIZE_16M:
> +	case SZ_16M:
>   		param->dram_config |= 0x04;
>   		break;
> -	case AST_VIDMEM_SIZE_32M:
> +	case SZ_32M:
>   		param->dram_config |= 0x08;
>   		break;
> -	case AST_VIDMEM_SIZE_64M:
> +	case SZ_64M:
>   		param->dram_config |= 0x0c;
>   		break;
>   	}
> @@ -1446,16 +1446,16 @@ static void get_ddr2_info(struct ast_device *ast, struct ast2300_dram_param *par
>   
>   	switch (param->vram_size) {
>   	default:
> -	case AST_VIDMEM_SIZE_8M:
> +	case SZ_8M:
>   		param->dram_config |= 0x00;
>   		break;
> -	case AST_VIDMEM_SIZE_16M:
> +	case SZ_16M:
>   		param->dram_config |= 0x04;
>   		break;
> -	case AST_VIDMEM_SIZE_32M:
> +	case SZ_32M:
>   		param->dram_config |= 0x08;
>   		break;
> -	case AST_VIDMEM_SIZE_64M:
> +	case SZ_64M:
>   		param->dram_config |= 0x0c;
>   		break;
>   	}
> @@ -1635,19 +1635,19 @@ static void ast_post_chip_2300(struct ast_device *ast)
>                   switch (temp & 0x0c) {
>                   default:
>   		case 0x00:
> -			param.vram_size = AST_VIDMEM_SIZE_8M;
> +			param.vram_size = SZ_8M;
>   			break;
>   
>   		case 0x04:
> -			param.vram_size = AST_VIDMEM_SIZE_16M;
> +			param.vram_size = SZ_16M;
>   			break;
>   
>   		case 0x08:
> -			param.vram_size = AST_VIDMEM_SIZE_32M;
> +			param.vram_size = SZ_32M;
>   			break;
>   
>   		case 0x0c:
> -			param.vram_size = AST_VIDMEM_SIZE_64M;
> +			param.vram_size = SZ_64M;
>   			break;
>   		}
>   

Re: [PATCH v2 2/7] drm/ast: Add VGACRAA register constants

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> Add register constants for VGACRAA and use them when detecting the
> size of the VGA memory. Aligns the code with the programming manual.
> 
Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mm.c  | 5 +++--
>   drivers/gpu/drm/ast/ast_reg.h | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 20d833632a01..8d8aac8c0814 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -37,9 +37,10 @@ static u32 ast_get_vram_size(struct ast_device *ast)
>   {
>   	u8 jreg;
>   	u32 vram_size;
> +	u8 vgacraa;
>   
> -	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xaa, 0xff);
> -	switch (jreg & 3) {
> +	vgacraa = ast_get_index_reg(ast, AST_IO_VGACRI, 0xaa);
> +	switch (vgacraa & AST_IO_VGACRAA_VGAMEM_SIZE_MASK) {
>   	case 0:
>   		vram_size = SZ_8M;
>   		break;
> diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
> index bb2cc1d8b84e..039b93bed19e 100644
> --- a/drivers/gpu/drm/ast/ast_reg.h
> +++ b/drivers/gpu/drm/ast/ast_reg.h
> @@ -33,6 +33,7 @@
>   #define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
>   #define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
>   #define AST_IO_VGACRA3_DVO_ENABLED	BIT(7)
> +#define AST_IO_VGACRAA_VGAMEM_SIZE_MASK	GENMASK(1, 0)
>   #define AST_IO_VGACRB6_HSYNC_OFF	BIT(0)
>   #define AST_IO_VGACRB6_VSYNC_OFF	BIT(1)
>   #define AST_IO_VGACRCB_HWC_16BPP	BIT(0) /* set: ARGB4444, cleared: 2bpp palette */

Re: [PATCH v2 3/7] drm/ast: Add VGACR99 register constants

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> Add register constants for VGACR99 and use them when detecting the
> size of the VGA memory. Aligns the code with the programming manual.
> Also replace literal size values with Linux' SZ_ size constants.
> 
Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mm.c  | 13 ++++++-------
>   drivers/gpu/drm/ast/ast_reg.h |  1 +
>   2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 8d8aac8c0814..3d03ef556d0a 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -35,9 +35,8 @@
>   
>   static u32 ast_get_vram_size(struct ast_device *ast)
>   {
> -	u8 jreg;
>   	u32 vram_size;
> -	u8 vgacraa;
> +	u8 vgacr99, vgacraa;
>   
>   	vgacraa = ast_get_index_reg(ast, AST_IO_VGACRI, 0xaa);
>   	switch (vgacraa & AST_IO_VGACRAA_VGAMEM_SIZE_MASK) {
> @@ -55,16 +54,16 @@ static u32 ast_get_vram_size(struct ast_device *ast)
>   		break;
>   	}
>   
> -	jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0x99, 0xff);
> -	switch (jreg & 0x03) {
> +	vgacr99 = ast_get_index_reg(ast, AST_IO_VGACRI, 0x99);
> +	switch (vgacr99 & AST_IO_VGACR99_VGAMEM_RSRV_MASK) {
>   	case 1:
> -		vram_size -= 0x100000;
> +		vram_size -= SZ_1M;
>   		break;
>   	case 2:
> -		vram_size -= 0x200000;
> +		vram_size -= SZ_2M;
>   		break;
>   	case 3:
> -		vram_size -= 0x400000;
> +		vram_size -= SZ_4M;
>   		break;
>   	}
>   
> diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
> index 039b93bed19e..e15adaf3a80e 100644
> --- a/drivers/gpu/drm/ast/ast_reg.h
> +++ b/drivers/gpu/drm/ast/ast_reg.h
> @@ -30,6 +30,7 @@
>   
>   #define AST_IO_VGACRI			(0x54)
>   #define AST_IO_VGACR80_PASSWORD		(0xa8)
> +#define AST_IO_VGACR99_VGAMEM_RSRV_MASK	GENMASK(1, 0)
>   #define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
>   #define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
>   #define AST_IO_VGACRA3_DVO_ENABLED	BIT(7)

Re: [PATCH v2 4/7] drm/ast: cursor: Add helpers for computing location in video memory

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> The ast drivers stores the cursor image at the end of the video memory.
> Add helpers to calculate the offset and size.
> 
Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_cursor.c | 21 +++++++++++++++++++--
>   drivers/gpu/drm/ast/ast_drv.h    |  1 +
>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> index 139ab00dee8f..05e297f30b4e 100644
> --- a/drivers/gpu/drm/ast/ast_cursor.c
> +++ b/drivers/gpu/drm/ast/ast_cursor.c
> @@ -45,6 +45,21 @@
>   #define AST_HWC_SIGNATURE_HOTSPOTX	0x14
>   #define AST_HWC_SIGNATURE_HOTSPOTY	0x18
>   
> +static unsigned long ast_cursor_vram_size(void)
> +{
> +	return AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE;
> +}
> +
> +long ast_cursor_vram_offset(struct ast_device *ast)
> +{
> +	unsigned long size = ast_cursor_vram_size();
> +
> +	if (size > ast->vram_size)
> +		return -EINVAL;
> +
> +	return PAGE_ALIGN_DOWN(ast->vram_size - size);
> +}
> +
>   static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, unsigned int height)
>   {
>   	u32 csum = 0;
> @@ -276,7 +291,7 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   	struct drm_plane *cursor_plane = &ast_plane->base;
>   	size_t size;
>   	void __iomem *vaddr;
> -	u64 offset;
> +	long offset;
>   	int ret;
>   
>   	/*
> @@ -290,7 +305,9 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   		return -ENOMEM;
>   
>   	vaddr = ast->vram + ast->vram_fb_available - size;
> -	offset = ast->vram_fb_available - size;
> +	offset = ast_cursor_vram_offset(ast);
> +	if (offset < 0)
> +		return offset;
>   
>   	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
>   			     0x01, &ast_cursor_plane_funcs,
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 2c7861835cfb..ec9ec77260e9 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -432,6 +432,7 @@ int ast_vga_output_init(struct ast_device *ast);
>   int ast_sil164_output_init(struct ast_device *ast);
>   
>   /* ast_cursor.c */
> +long ast_cursor_vram_offset(struct ast_device *ast);
>   int ast_cursor_plane_init(struct ast_device *ast);
>   
>   /* ast dp501 */

Re: [PATCH v2 5/7] drm/ast: Add helper for computing framebuffer location in video memory

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> The ast driver stores the primary plane's image in the framebuffer
> memory up to where the cursor is located. Add helpers to calculate
> the offset and size.
> 
Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index c3b950675485..4cac5c7f4547 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -51,6 +51,24 @@
>   
>   #define AST_LUT_SIZE 256
>   
> +static unsigned long ast_fb_vram_offset(void)
> +{
> +	return 0; // with shmem, the primary plane is always at offset 0
> +}
> +
> +static unsigned long ast_fb_vram_size(struct ast_device *ast)
> +{
> +	struct drm_device *dev = &ast->base;
> +	unsigned long offset = ast_fb_vram_offset(); // starts at offset
> +	long cursor_offset = ast_cursor_vram_offset(ast); // ends at cursor offset
> +
> +	if (cursor_offset < 0)
> +		cursor_offset = ast->vram_size; // no cursor; it's all ours
> +	if (drm_WARN_ON_ONCE(dev, offset > cursor_offset))
> +		return 0; // cannot legally happen; signal error
> +	return cursor_offset - offset;
> +}
> +
>   static inline void ast_load_palette_index(struct ast_device *ast,
>   				     u8 index, u8 red, u8 green,
>   				     u8 blue)
> @@ -609,9 +627,8 @@ static int ast_primary_plane_init(struct ast_device *ast)
>   	struct ast_plane *ast_primary_plane = &ast->primary_plane;
>   	struct drm_plane *primary_plane = &ast_primary_plane->base;
>   	void __iomem *vaddr = ast->vram;
> -	u64 offset = 0; /* with shmem, the primary plane is always at offset 0 */
> -	unsigned long cursor_size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
> -	unsigned long size = ast->vram_fb_available - cursor_size;
> +	u64 offset = ast_fb_vram_offset();
> +	unsigned long size = ast_fb_vram_size(ast);
>   	int ret;
>   
>   	ret = ast_plane_init(dev, ast_primary_plane, vaddr, offset, size,
> @@ -942,7 +959,7 @@ static enum drm_mode_status ast_mode_config_mode_valid(struct drm_device *dev,
>   	struct ast_device *ast = to_ast_device(dev);
>   	unsigned long fbsize, fbpages, max_fbpages;
>   
> -	max_fbpages = (ast->vram_fb_available) >> PAGE_SHIFT;
> +	max_fbpages = ast_fb_vram_size(ast) >> PAGE_SHIFT;
>   
>   	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
>   	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);

Re: [PATCH v2 6/7] drm/ast: Remove vram_fb_available from struct ast_device

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> Helpers compute the offset and size of the available framebuffer
> memory. Remove the obsolete field vram_fb_available from struct
> ast_device. Also define the cursor-signature size next to its only
> user.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> v2:
> - initialize plane size
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_cursor.c | 18 ++++--------------
>   drivers/gpu/drm/ast/ast_drv.h    |  4 ----
>   drivers/gpu/drm/ast/ast_mm.c     |  1 -
>   3 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> index 05e297f30b4e..cb0c48d47207 100644
> --- a/drivers/gpu/drm/ast/ast_cursor.c
> +++ b/drivers/gpu/drm/ast/ast_cursor.c
> @@ -37,6 +37,7 @@
>    */
>   
>   /* define for signature structure */
> +#define AST_HWC_SIGNATURE_SIZE		SZ_32
>   #define AST_HWC_SIGNATURE_CHECKSUM	0x00
>   #define AST_HWC_SIGNATURE_SizeX		0x04
>   #define AST_HWC_SIGNATURE_SizeY		0x08
> @@ -289,25 +290,16 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   	struct ast_cursor_plane *ast_cursor_plane = &ast->cursor_plane;
>   	struct ast_plane *ast_plane = &ast_cursor_plane->base;
>   	struct drm_plane *cursor_plane = &ast_plane->base;
> -	size_t size;
> +	unsigned long size;
>   	void __iomem *vaddr;
>   	long offset;
>   	int ret;
>   
> -	/*
> -	 * Allocate backing storage for cursors. The BOs are permanently
> -	 * pinned to the top end of the VRAM.
> -	 */
> -
> -	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
> -
> -	if (ast->vram_fb_available < size)
> -		return -ENOMEM;
> -
> -	vaddr = ast->vram + ast->vram_fb_available - size;
> +	size = ast_cursor_vram_size();
>   	offset = ast_cursor_vram_offset(ast);
>   	if (offset < 0)
>   		return offset;
> +	vaddr = ast->vram + offset;
>   
>   	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
>   			     0x01, &ast_cursor_plane_funcs,
> @@ -320,7 +312,5 @@ int ast_cursor_plane_init(struct ast_device *ast)
>   	drm_plane_helper_add(cursor_plane, &ast_cursor_plane_helper_funcs);
>   	drm_plane_enable_fb_damage_clips(cursor_plane);
>   
> -	ast->vram_fb_available -= size;
> -
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index ec9ec77260e9..d9da2328d46b 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -112,12 +112,9 @@ enum ast_config_mode {
>   
>   #define AST_MAX_HWC_WIDTH	64
>   #define AST_MAX_HWC_HEIGHT	64
> -
>   #define AST_HWC_PITCH		(AST_MAX_HWC_WIDTH * SZ_2)
>   #define AST_HWC_SIZE		(AST_MAX_HWC_HEIGHT * AST_HWC_PITCH)
>   
> -#define AST_HWC_SIGNATURE_SIZE	32
> -
>   /*
>    * Planes
>    */
> @@ -183,7 +180,6 @@ struct ast_device {
>   	void __iomem	*vram;
>   	unsigned long	vram_base;
>   	unsigned long	vram_size;
> -	unsigned long	vram_fb_available;
>   
>   	struct mutex modeset_lock; /* Protects access to modeset I/O registers in ioregs */
>   
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 3d03ef556d0a..0bc140319464 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -92,7 +92,6 @@ int ast_mm_init(struct ast_device *ast)
>   
>   	ast->vram_base = base;
>   	ast->vram_size = vram_size;
> -	ast->vram_fb_available = vram_size;
>   
>   	return 0;
>   }

Re: [PATCH v2 7/7] drm/ast: cursor: Drop page alignment

[Jocelyn Falempe]

On 05/03/2025 17:30, Thomas Zimmermann wrote:
> The cursor scanout address requires alignment to a multiple of 8,
> but does not require page alignment. Change the offset calculation
> accordingly. Frees up a few more bytes for the primary framebuffer.
> 
The framebuffer is page aligned, so I'm not sure you can use the extra 
bytes.
Otherwise, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_cursor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> index cb0c48d47207..5ee724bfd682 100644
> --- a/drivers/gpu/drm/ast/ast_cursor.c
> +++ b/drivers/gpu/drm/ast/ast_cursor.c
> @@ -58,7 +58,7 @@ long ast_cursor_vram_offset(struct ast_device *ast)
>   	if (size > ast->vram_size)
>   		return -EINVAL;
>   
> -	return PAGE_ALIGN_DOWN(ast->vram_size - size);
> +	return ALIGN_DOWN(ast->vram_size - size, SZ_8);
>   }
>   
>   static u32 ast_cursor_calculate_checksum(const void *src, unsigned int width, unsigned int height)

Re: [PATCH v2 7/7] drm/ast: cursor: Drop page alignment

[Thomas Zimmermann]

Hi

Am 11.03.25 um 14:10 schrieb Jocelyn Falempe:
> On 05/03/2025 17:30, Thomas Zimmermann wrote:
>> The cursor scanout address requires alignment to a multiple of 8,
>> but does not require page alignment. Change the offset calculation
>> accordingly. Frees up a few more bytes for the primary framebuffer.
>>
> The framebuffer is page aligned, so I'm not sure you can use the extra 
> bytes.

The GEM buffer located in system ram is page aligned, so that mmap works 
as expected. But the scanout buffer in VRAM doesn't have to be as we 
don't ever mmap that to user space. The cursor scanout address only 
requires an alignment of 8. The damage handling only copies visible 
bytes, hence we don't accidentally overwrite cursor bytes when we copy 
the primary plane. So this should all be fine.

The driver likely requires an improved version of 
ast_mode_config_mode_valid() to really use these extra bytes. I've been 
working on this a bit.

I think this might not make much of a difference on most machines, but 
there are lowest-of-the-low-end systems with only 4 MiB of VRAM 
available. Maybe there's a case where those additional bytes will be useful.

> Otherwise, it looks good to me.
>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

Thanks for reviewing, BTW

Best regards
Thomas

>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_cursor.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_cursor.c 
>> b/drivers/gpu/drm/ast/ast_cursor.c
>> index cb0c48d47207..5ee724bfd682 100644
>> --- a/drivers/gpu/drm/ast/ast_cursor.c
>> +++ b/drivers/gpu/drm/ast/ast_cursor.c
>> @@ -58,7 +58,7 @@ long ast_cursor_vram_offset(struct ast_device *ast)
>>       if (size > ast->vram_size)
>>           return -EINVAL;
>>   -    return PAGE_ALIGN_DOWN(ast->vram_size - size);
>> +    return ALIGN_DOWN(ast->vram_size - size, SZ_8);
>>   }
>>     static u32 ast_cursor_calculate_checksum(const void *src, 
>> unsigned int width, unsigned int height)
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)