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@xxxxxxxxxx>
Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
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;
