[PATCH 1/4] drm/ast: cursor: Calculate checksum in helper
Jocelyn Falempe
jfalempe at redhat.com
Fri Feb 14 10:29:13 UTC 2025
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 at redhat.com>
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at 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;
More information about the dri-devel
mailing list