diff mbox series

[1/4] drm/ast: cursor: Calculate checksum in helper

Message ID 20250213163845.118207-2-tzimmermann@suse.de (mailing list archive)
State New
Headers show
Series drm/ast: cursor: Improve format handling | expand

Commit Message

Thomas Zimmermann Feb. 13, 2025, 4:25 p.m. UTC 
Setting the cursor image requires a 32-bit checksum of the cursor
image data. The current cursor code converts the image to ARGB4444
format and computes the checksum in a single step. Moving the
checksum calculation into a separate helepr will allow to move the
format conversion into a shared helper.

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

Comments

Jocelyn Falempe Feb. 14, 2025, 10:29 a.m. UTC | #1 
On 13/02/2025 17:25, Thomas Zimmermann wrote:
> Setting the cursor image requires a 32-bit checksum of the cursor
> image data. The current cursor code converts the image to ARGB4444
> format and computes the checksum in a single step. Moving the
> checksum calculation into a separate helepr will allow to move the
typo: helper
> format conversion into a shared helper.


Thanks, I've only one remark below, but you can keep it as-is if you prefer.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  | 13 +++++++-
>   drivers/gpu/drm/ast/ast_mode.c | 57 +++++++++++++++++++++++++---------
>   2 files changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 022a8c070c1b..d3115b31b032 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -142,6 +142,17 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
>   	return container_of(plane, struct ast_plane, base);
>   }
>   
> +struct ast_cursor_plane {
> +	struct ast_plane base;
> +
> +	u8 argb4444[AST_HWC_SIZE];
> +};
> +
> +static inline struct ast_cursor_plane *to_ast_cursor_plane(struct drm_plane *plane)
> +{
> +	return container_of(to_ast_plane(plane), struct ast_cursor_plane, base);
> +}
> +
>   /*
>    * Connector
>    */
> @@ -186,7 +197,7 @@ struct ast_device {
>   	enum ast_tx_chip tx_chip;
>   
>   	struct ast_plane primary_plane;
> -	struct ast_plane cursor_plane;
> +	struct ast_cursor_plane cursor_plane;
>   	struct drm_crtc crtc;
>   	union {
>   		struct {
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index bd781293b6d9..974f4eb46bc3 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -632,7 +632,32 @@ static int ast_primary_plane_init(struct ast_device *ast)
>    * Cursor plane
>    */
>   
> -static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int height)
> +static u32 ast_cursor_calculate_checksum(const u8 *src, unsigned int width, unsigned int height)
> +{
> +	u32 csum = 0;
> +	unsigned int i, j;
> +	unsigned int one_pixel_copy = width & BIT(0);
> +	unsigned int two_pixel_copy = width - one_pixel_copy;
> +	unsigned int trailing_bytes = (AST_MAX_HWC_WIDTH - width) * sizeof(u16);
> +
> +	for (j = 0; j < height; j++) {
> +		for (i = 0; i < two_pixel_copy; i += 2, src += SZ_4) {
> +			const u32 *src32 = (const u32 *)src;
> +
> +			csum += *src32;
> +		}
> +		for (i = 0; i < one_pixel_copy; i++, src += SZ_2) {
This for loop can run only 0 or 1 time, so I would prefer an "if" instead.

> +			const u16 *src16 = (const u16 *)src;
> +
> +			csum += *src16;
> +		}
> +		src += trailing_bytes;
> +	}
> +
> +	return csum;
> +}
> +
> +static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, u8 *tmp, int width, int height)
>   {
>   	union {
>   		u32 ul;
> @@ -644,7 +669,7 @@ static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
>   	} data16;
>   	u32 csum = 0;
>   	s32 alpha_dst_delta, last_alpha_dst_delta;
> -	u8 __iomem *dstxor;
> +	u8 *dstxor;
>   	const u8 *srcxor;
>   	int i, j;
>   	u32 per_pixel_copy, two_pixel_copy;
> @@ -653,7 +678,7 @@ static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
>   	last_alpha_dst_delta = alpha_dst_delta - (width << 1);
>   
>   	srcxor = src;
> -	dstxor = (u8 *)dst + last_alpha_dst_delta + (AST_MAX_HWC_HEIGHT - height) * alpha_dst_delta;
> +	dstxor = tmp + last_alpha_dst_delta + (AST_MAX_HWC_HEIGHT - height) * alpha_dst_delta;
>   	per_pixel_copy = width & 1;
>   	two_pixel_copy = width >> 1;
>   
> @@ -665,21 +690,17 @@ static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
>   			data32.b[1] = srcdata32[0].b[3] | (srcdata32[0].b[2] >> 4);
>   			data32.b[2] = srcdata32[1].b[1] | (srcdata32[1].b[0] >> 4);
>   			data32.b[3] = srcdata32[1].b[3] | (srcdata32[1].b[2] >> 4);
> -
> -			writel(data32.ul, dstxor);
> -			csum += data32.ul;
> +			memcpy(dstxor, &data32, 4);
>   
>   			dstxor += 4;
>   			srcxor += 8;
> -
>   		}
>   
>   		for (i = 0; i < per_pixel_copy; i++) {
>   			srcdata32[0].ul = *((u32 *)srcxor) & 0xf0f0f0f0;
>   			data16.b[0] = srcdata32[0].b[1] | (srcdata32[0].b[0] >> 4);
>   			data16.b[1] = srcdata32[0].b[3] | (srcdata32[0].b[2] >> 4);
> -			writew(data16.us, dstxor);
> -			csum += (u32)data16.us;
> +			memcpy(dstxor, &data16, 2);
>   
>   			dstxor += 2;
>   			srcxor += 4;
> @@ -687,6 +708,11 @@ static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
>   		dstxor += last_alpha_dst_delta;
>   	}
>   
> +	csum = ast_cursor_calculate_checksum(tmp, width, height);
> +
> +	/* write pixel data */
> +	memcpy_toio(dst, tmp, AST_HWC_SIZE);
> +
>   	/* write checksum + signature */
>   	dst += AST_HWC_SIZE;
>   	writel(csum, dst);
> @@ -767,6 +793,7 @@ static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
>   static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>   						  struct drm_atomic_state *state)
>   {
> +	struct ast_cursor_plane *ast_cursor_plane = to_ast_cursor_plane(plane);
>   	struct ast_plane *ast_plane = to_ast_plane(plane);
>   	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> @@ -789,7 +816,8 @@ static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>   	 */
>   
>   	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &damage)) {
> -		ast_update_cursor_image(dst, src, fb->width, fb->height);
> +		ast_update_cursor_image(dst, src, ast_cursor_plane->argb4444,
> +					fb->width, fb->height);
>   		ast_set_cursor_base(ast, dst_off);
>   	}
>   
> @@ -849,8 +877,9 @@ static const struct drm_plane_funcs ast_cursor_plane_funcs = {
>   static int ast_cursor_plane_init(struct ast_device *ast)
>   {
>   	struct drm_device *dev = &ast->base;
> -	struct ast_plane *ast_cursor_plane = &ast->cursor_plane;
> -	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
> +	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;
>   	void __iomem *vaddr;
>   	u64 offset;
> @@ -869,7 +898,7 @@ static int ast_cursor_plane_init(struct ast_device *ast)
>   	vaddr = ast->vram + ast->vram_fb_available - size;
>   	offset = ast->vram_fb_available - size;
>   
> -	ret = ast_plane_init(dev, ast_cursor_plane, vaddr, offset, size,
> +	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
>   			     0x01, &ast_cursor_plane_funcs,
>   			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
>   			     NULL, DRM_PLANE_TYPE_CURSOR);
> @@ -1156,7 +1185,7 @@ static int ast_crtc_init(struct ast_device *ast)
>   	int ret;
>   
>   	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane.base,
> -					&ast->cursor_plane.base, &ast_crtc_funcs,
> +					&ast->cursor_plane.base.base, &ast_crtc_funcs,
>   					NULL);
>   	if (ret)
>   		return ret;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 022a8c070c1b..d3115b31b032 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -142,6 +142,17 @@  static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 	return container_of(plane, struct ast_plane, base);
 }
 
+struct ast_cursor_plane {
+	struct ast_plane base;
+
+	u8 argb4444[AST_HWC_SIZE];
+};
+
+static inline struct ast_cursor_plane *to_ast_cursor_plane(struct drm_plane *plane)
+{
+	return container_of(to_ast_plane(plane), struct ast_cursor_plane, base);
+}
+
 /*
  * Connector
  */
@@ -186,7 +197,7 @@  struct ast_device {
 	enum ast_tx_chip tx_chip;
 
 	struct ast_plane primary_plane;
-	struct ast_plane cursor_plane;
+	struct ast_cursor_plane cursor_plane;
 	struct drm_crtc crtc;
 	union {
 		struct {
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index bd781293b6d9..974f4eb46bc3 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -632,7 +632,32 @@  static int ast_primary_plane_init(struct ast_device *ast)
  * Cursor plane
  */
 
-static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int height)
+static u32 ast_cursor_calculate_checksum(const u8 *src, unsigned int width, unsigned int height)
+{
+	u32 csum = 0;
+	unsigned int i, j;
+	unsigned int one_pixel_copy = width & BIT(0);
+	unsigned int two_pixel_copy = width - one_pixel_copy;
+	unsigned int trailing_bytes = (AST_MAX_HWC_WIDTH - width) * sizeof(u16);
+
+	for (j = 0; j < height; j++) {
+		for (i = 0; i < two_pixel_copy; i += 2, src += SZ_4) {
+			const u32 *src32 = (const u32 *)src;
+
+			csum += *src32;
+		}
+		for (i = 0; i < one_pixel_copy; i++, src += SZ_2) {
+			const u16 *src16 = (const u16 *)src;
+
+			csum += *src16;
+		}
+		src += trailing_bytes;
+	}
+
+	return csum;
+}
+
+static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, u8 *tmp, int width, int height)
 {
 	union {
 		u32 ul;
@@ -644,7 +669,7 @@  static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
 	} data16;
 	u32 csum = 0;
 	s32 alpha_dst_delta, last_alpha_dst_delta;
-	u8 __iomem *dstxor;
+	u8 *dstxor;
 	const u8 *srcxor;
 	int i, j;
 	u32 per_pixel_copy, two_pixel_copy;
@@ -653,7 +678,7 @@  static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
 	last_alpha_dst_delta = alpha_dst_delta - (width << 1);
 
 	srcxor = src;
-	dstxor = (u8 *)dst + last_alpha_dst_delta + (AST_MAX_HWC_HEIGHT - height) * alpha_dst_delta;
+	dstxor = tmp + last_alpha_dst_delta + (AST_MAX_HWC_HEIGHT - height) * alpha_dst_delta;
 	per_pixel_copy = width & 1;
 	two_pixel_copy = width >> 1;
 
@@ -665,21 +690,17 @@  static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
 			data32.b[1] = srcdata32[0].b[3] | (srcdata32[0].b[2] >> 4);
 			data32.b[2] = srcdata32[1].b[1] | (srcdata32[1].b[0] >> 4);
 			data32.b[3] = srcdata32[1].b[3] | (srcdata32[1].b[2] >> 4);
-
-			writel(data32.ul, dstxor);
-			csum += data32.ul;
+			memcpy(dstxor, &data32, 4);
 
 			dstxor += 4;
 			srcxor += 8;
-
 		}
 
 		for (i = 0; i < per_pixel_copy; i++) {
 			srcdata32[0].ul = *((u32 *)srcxor) & 0xf0f0f0f0;
 			data16.b[0] = srcdata32[0].b[1] | (srcdata32[0].b[0] >> 4);
 			data16.b[1] = srcdata32[0].b[3] | (srcdata32[0].b[2] >> 4);
-			writew(data16.us, dstxor);
-			csum += (u32)data16.us;
+			memcpy(dstxor, &data16, 2);
 
 			dstxor += 2;
 			srcxor += 4;
@@ -687,6 +708,11 @@  static void ast_update_cursor_image(u8 __iomem *dst, const u8 *src, int width, i
 		dstxor += last_alpha_dst_delta;
 	}
 
+	csum = ast_cursor_calculate_checksum(tmp, width, height);
+
+	/* write pixel data */
+	memcpy_toio(dst, tmp, AST_HWC_SIZE);
+
 	/* write checksum + signature */
 	dst += AST_HWC_SIZE;
 	writel(csum, dst);
@@ -767,6 +793,7 @@  static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
 static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 						  struct drm_atomic_state *state)
 {
+	struct ast_cursor_plane *ast_cursor_plane = to_ast_cursor_plane(plane);
 	struct ast_plane *ast_plane = to_ast_plane(plane);
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
@@ -789,7 +816,8 @@  static void ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 	 */
 
 	if (drm_atomic_helper_damage_merged(old_plane_state, plane_state, &damage)) {
-		ast_update_cursor_image(dst, src, fb->width, fb->height);
+		ast_update_cursor_image(dst, src, ast_cursor_plane->argb4444,
+					fb->width, fb->height);
 		ast_set_cursor_base(ast, dst_off);
 	}
 
@@ -849,8 +877,9 @@  static const struct drm_plane_funcs ast_cursor_plane_funcs = {
 static int ast_cursor_plane_init(struct ast_device *ast)
 {
 	struct drm_device *dev = &ast->base;
-	struct ast_plane *ast_cursor_plane = &ast->cursor_plane;
-	struct drm_plane *cursor_plane = &ast_cursor_plane->base;
+	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;
 	void __iomem *vaddr;
 	u64 offset;
@@ -869,7 +898,7 @@  static int ast_cursor_plane_init(struct ast_device *ast)
 	vaddr = ast->vram + ast->vram_fb_available - size;
 	offset = ast->vram_fb_available - size;
 
-	ret = ast_plane_init(dev, ast_cursor_plane, vaddr, offset, size,
+	ret = ast_plane_init(dev, ast_plane, vaddr, offset, size,
 			     0x01, &ast_cursor_plane_funcs,
 			     ast_cursor_plane_formats, ARRAY_SIZE(ast_cursor_plane_formats),
 			     NULL, DRM_PLANE_TYPE_CURSOR);
@@ -1156,7 +1185,7 @@  static int ast_crtc_init(struct ast_device *ast)
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane.base,
-					&ast->cursor_plane.base, &ast_crtc_funcs,
+					&ast->cursor_plane.base.base, &ast_crtc_funcs,
 					NULL);
 	if (ret)
 		return ret;