[PATCH 2/4] drm/ast: cursor: Move format conversion to shared helper
Thomas Zimmermann
tzimmermann at suse.de
Fri Feb 14 10:45:44 UTC 2025
Hi
Am 14.02.25 um 11:35 schrieb Jocelyn Falempe:
> On 13/02/2025 17:25, Thomas Zimmermann wrote:
>> User-space cursor-image data is encoded in ARBG8888, while hardware
>> supports ARGB4444. Implement the format conversion as part of the
>> format-helper framework, so that other drivers can benefit.
>>
>> This allows to respect the damage area of the cursor update. In
>> previous code, all cursor image data had to be converted on each
>> update. Now, only the changed areas require an update. The hardware
>> image is always updated completely, as it is required for the
>> checksum update.
>
> Hum, for a 64x64 cursor size, I don't see much benefit to handle
> damage area (And I'm not sure userspace actually change only parts of
> the cursor).
Of course. I could imagine a user space that implements fully generic
plane handling and treats all planes as equal; hence doing damage
handling on cursors. But current user space usually treats cursor planes
separately from primary planes and always replaces the cursor image with
a different one. But we get it for free, so why not?
>
>>
>> The format-conversion helper still contains the old implementation's
>> optimization of writing 2 output pixels at the same time.
>
> Thanks, it looks good to me.
>
> Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
Thanks for reviewing.
Best regards
Thomas
>
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/ast/ast_drv.h | 4 +-
>> drivers/gpu/drm/ast/ast_mode.c | 71 +++++++----------------------
>> drivers/gpu/drm/drm_format_helper.c | 69 ++++++++++++++++++++++++++++
>> include/drm/drm_format_helper.h | 3 ++
>> 4 files changed, 92 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index d3115b31b032..973abd0cbd42 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -113,7 +113,9 @@ enum ast_config_mode {
>> #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_PITCH (AST_MAX_HWC_WIDTH * SZ_2)
>> +#define AST_HWC_SIZE (AST_MAX_HWC_HEIGHT * AST_HWC_PITCH)
>> +
>> #define AST_HWC_SIGNATURE_SIZE 32
>> /* define for signature structure */
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index 974f4eb46bc3..ed00275d6418 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -657,61 +657,16 @@ static u32 ast_cursor_calculate_checksum(const
>> u8 *src, unsigned int width, unsi
>> return csum;
>> }
>> -static void ast_update_cursor_image(u8 __iomem *dst, const u8
>> *src, u8 *tmp, int width, int height)
>> +static void ast_set_cursor_image(struct ast_device *ast, const u8 *src,
>> + unsigned int width, unsigned int height)
>> {
>> - union {
>> - u32 ul;
>> - u8 b[4];
>> - } srcdata32[2], data32;
>> - union {
>> - u16 us;
>> - u8 b[2];
>> - } data16;
>> + u8 __iomem *dst = ast->cursor_plane.base.vaddr;
>> u32 csum = 0;
>> - s32 alpha_dst_delta, last_alpha_dst_delta;
>> - u8 *dstxor;
>> - const u8 *srcxor;
>> - int i, j;
>> - u32 per_pixel_copy, two_pixel_copy;
>> - alpha_dst_delta = AST_MAX_HWC_WIDTH << 1;
>> - last_alpha_dst_delta = alpha_dst_delta - (width << 1);
>> -
>> - srcxor = src;
>> - dstxor = tmp + last_alpha_dst_delta + (AST_MAX_HWC_HEIGHT -
>> height) * alpha_dst_delta;
>> - per_pixel_copy = width & 1;
>> - two_pixel_copy = width >> 1;
>> -
>> - for (j = 0; j < height; j++) {
>> - for (i = 0; i < two_pixel_copy; i++) {
>> - srcdata32[0].ul = *((u32 *)srcxor) & 0xf0f0f0f0;
>> - srcdata32[1].ul = *((u32 *)(srcxor + 4)) & 0xf0f0f0f0;
>> - data32.b[0] = srcdata32[0].b[1] | (srcdata32[0].b[0] >> 4);
>> - 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);
>> - 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);
>> - memcpy(dstxor, &data16, 2);
>> -
>> - dstxor += 2;
>> - srcxor += 4;
>> - }
>> - dstxor += last_alpha_dst_delta;
>> - }
>> -
>> - csum = ast_cursor_calculate_checksum(tmp, width, height);
>> + csum = ast_cursor_calculate_checksum(src, width, height);
>> /* write pixel data */
>> - memcpy_toio(dst, tmp, AST_HWC_SIZE);
>> + memcpy_toio(dst, src, AST_HWC_SIZE);
>> /* write checksum + signature */
>> dst += AST_HWC_SIZE;
>> @@ -800,9 +755,7 @@ static void
>> ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
>> struct drm_framebuffer *fb = plane_state->fb;
>> struct drm_plane_state *old_plane_state =
>> drm_atomic_get_old_plane_state(state, plane);
>> struct ast_device *ast = to_ast_device(plane->dev);
>> - struct iosys_map src_map = shadow_plane_state->data[0];
>> struct drm_rect damage;
>> - const u8 *src = src_map.vaddr; /* TODO: Use mapping abstraction
>> properly */
>> u64 dst_off = ast_plane->offset;
>> u8 __iomem *dst = ast_plane->vaddr; /* TODO: Use mapping
>> abstraction properly */
>> u8 __iomem *sig = dst + AST_HWC_SIZE; /* TODO: Use mapping
>> abstraction properly */
>> @@ -816,8 +769,18 @@ 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, ast_cursor_plane->argb4444,
>> - fb->width, fb->height);
>> + u8 *argb4444 = ast_cursor_plane->argb4444;
>> + struct iosys_map argb4444_dst[DRM_FORMAT_MAX_PLANES] = {
>> + IOSYS_MAP_INIT_VADDR(argb4444),
>> + };
>> + unsigned int argb4444_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
>> + AST_HWC_PITCH,
>> + };
>> +
>> + drm_fb_argb8888_to_argb4444(argb4444_dst, argb4444_dst_pitch,
>> + shadow_plane_state->data, fb, &damage,
>> + &shadow_plane_state->fmtcnv_state);
>> + ast_set_cursor_image(ast, argb4444, fb->width, fb->height);
>> ast_set_cursor_base(ast, dst_off);
>> }
>> diff --git a/drivers/gpu/drm/drm_format_helper.c
>> b/drivers/gpu/drm/drm_format_helper.c
>> index b1be458ed4dd..ecb278b63e8c 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -978,6 +978,75 @@ void drm_fb_xrgb8888_to_gray8(struct iosys_map
>> *dst, const unsigned int *dst_pit
>> }
>> EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
>> +static void drm_fb_argb8888_to_argb4444_line(void *dbuf, const
>> void *sbuf, unsigned int pixels)
>> +{
>> + unsigned int pixels2 = pixels & ~GENMASK_ULL(0, 0);
>> + __le32 *dbuf32 = dbuf;
>> + __le16 *dbuf16 = dbuf + pixels2 * sizeof(*dbuf16);
>> + const __le32 *sbuf32 = sbuf;
>> + unsigned int x;
>> + u32 val32;
>> + u16 val16;
>> + u32 pix[2];
>> +
>> + for (x = 0; x < pixels2; x += 2, ++dbuf32) {
>> + pix[0] = le32_to_cpu(sbuf32[x]);
>> + pix[1] = le32_to_cpu(sbuf32[x + 1]);
>> + val32 = ((pix[0] & 0xf0000000) >> 16) |
>> + ((pix[0] & 0x00f00000) >> 12) |
>> + ((pix[0] & 0x0000f000) >> 8) |
>> + ((pix[0] & 0x000000f0) >> 4) |
>> + ((pix[1] & 0xf0000000) >> 0) |
>> + ((pix[1] & 0x00f00000) << 4) |
>> + ((pix[1] & 0x0000f000) << 8) |
>> + ((pix[1] & 0x000000f0) << 12);
>> + *dbuf32 = cpu_to_le32(val32);
>> + }
>> + for (; x < pixels; x++) {
>> + pix[0] = le32_to_cpu(sbuf32[x]);
>> + val16 = ((pix[0] & 0xf0000000) >> 16) |
>> + ((pix[0] & 0x00f00000) >> 12) |
>> + ((pix[0] & 0x0000f000) >> 8) |
>> + ((pix[0] & 0x000000f0) >> 4);
>> + dbuf16[x] = cpu_to_le16(val16);
>> + }
>> +}
>> +
>> +/**
>> + * drm_fb_argb8888_to_argb4444 - Convert ARGB8888 to ARGB4444 clip
>> buffer
>> + * @dst: Array of ARGB4444 destination buffers
>> + * @dst_pitch: Array of numbers of bytes between the start of two
>> consecutive scanlines
>> + * within @dst; can be NULL if scanlines are stored next
>> to each other.
>> + * @src: Array of ARGB8888 source buffer
>> + * @fb: DRM framebuffer
>> + * @clip: Clip rectangle area to copy
>> + * @state: Transform and conversion state
>> + *
>> + * This function copies parts of a framebuffer to display memory and
>> converts
>> + * the color format during the process. The parameters @dst,
>> @dst_pitch and
>> + * @src refer to arrays. Each array must have at least as many
>> entries as
>> + * there are planes in @fb's format. Each entry stores the value for
>> the
>> + * format's respective color plane at the same index.
>> + *
>> + * This function does not apply clipping on @dst (i.e. the
>> destination is at the
>> + * top-left corner).
>> + *
>> + * Drivers can use this function for ARGB4444 devices that don't
>> support
>> + * ARGB8888 natively.
>> + */
>> +void drm_fb_argb8888_to_argb4444(struct iosys_map *dst, const
>> unsigned int *dst_pitch,
>> + const struct iosys_map *src, const struct
>> drm_framebuffer *fb,
>> + const struct drm_rect *clip, struct
>> drm_format_conv_state *state)
>> +{
>> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>> + 2,
>> + };
>> +
>> + drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
>> state,
>> + drm_fb_argb8888_to_argb4444_line);
>> +}
>> +EXPORT_SYMBOL(drm_fb_argb8888_to_argb4444);
>> +
>> /**
>> * drm_fb_blit - Copy parts of a framebuffer to display memory
>> * @dst: Array of display-memory addresses to copy to
>> diff --git a/include/drm/drm_format_helper.h
>> b/include/drm/drm_format_helper.h
>> index 428d81afe215..a1347e47e9d5 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -110,6 +110,9 @@ void drm_fb_xrgb8888_to_argb2101010(struct
>> iosys_map *dst, const unsigned int *d
>> void drm_fb_xrgb8888_to_gray8(struct iosys_map *dst, const unsigned
>> int *dst_pitch,
>> const struct iosys_map *src, const struct
>> drm_framebuffer *fb,
>> const struct drm_rect *clip, struct
>> drm_format_conv_state *state);
>> +void drm_fb_argb8888_to_argb4444(struct iosys_map *dst, const
>> unsigned int *dst_pitch,
>> + const struct iosys_map *src, const struct
>> drm_framebuffer *fb,
>> + const struct drm_rect *clip, struct
>> drm_format_conv_state *state);
>> int drm_fb_blit(struct iosys_map *dst, const unsigned int
>> *dst_pitch, uint32_t dst_format,
>> const struct iosys_map *src, const struct drm_framebuffer *fb,
>
--
--
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)
More information about the dri-devel
mailing list