Re: [PATCH] drm/ast: Fix big-endian support

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi,

thanks for the patch.

Am 02.12.25 um 17:06 schrieb René Rebe:
> The Aspeed ast drm driver has the frame-buffer RGBX swapped on

It is XRGB.

> big-endian RISC systems. Fix by enabling byte swapping for any
> __BIG_ENDIAN config.

Is this the RISC support that Linux famously shot down? :)

Anyway, I have another BE fix for PPC64, which I didn't take. I'd rather 
merge your fix with some changes.

[1] 
https://lore.kernel.org/dri-devel/407388289.1798972.1760725035958.JavaMail.zimbra@raptorengineeringinc.com/

>
> Fixes: 12fec1405dd5 ("drm: Initial KMS driver for AST (ASpeed Technologies) 2000 series (v2)")

I'd leave out the Fixes tag.  We never claimed that the drivers supports 
BE, so it's not really a fix.

> Signed-off-by: René Rebe <rene@exactco.de>
> ---
> Tested on Oracle T4-1 running sparc64 T2/Linux.
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++++++++
>   drivers/gpu/drm/ast/ast_reg.h  |  6 ++++++
>   2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 30b011ed0a05..155ae35470d9 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -708,6 +708,20 @@ static void ast_crtc_helper_mode_set_nofb(struct drm_crtc *crtc)
>   	ast_set_dclk_reg(ast, adjusted_mode, vmode);
>   	ast_set_crtthd_reg(ast);
>   	ast_set_sync_reg(ast, adjusted_mode, vmode);
> +
> +#ifdef __BIG_ENDIAN
> +	/* Big-endian byte-swapping */
> +	switch (ast_crtc_state->format->format) {

This function sets the display mode. But the color format can change per 
frame. So it's not the right place.

Then, we also have a cursor plane that always scans out in ARGB4444 
format. How does this change interact with the cursor? AFAIU it should 
mix up the pixels if set to 32-bit BE.

Therefore, I think we need to set the BE mode in each plane's atomic 
update before it updates the video memory. At [2], for the primary 
plane, it has to be located between the color-update code and the damage 
handling. At [3], for the cursor plane, it can be within the if-damage 
branch. The cursor update needs unconditional 16-but swapping.

[2] 
https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next-2025-12-01-1/drivers/gpu/drm/ast/ast_mode.c?ref_type=tags#L559
[3] 
https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next-2025-12-01-1/drivers/gpu/drm/ast/ast_cursor.c?ref_type=tags#L209

> +	case DRM_FORMAT_RGB565:
> +		ast_set_index_reg_mask(ast, AST_IO_VGACRI, AST_IO_VGACRA2, 0x3f, 0x40);
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +		ast_set_index_reg_mask(ast, AST_IO_VGACRI, AST_IO_VGACRA2, 0x3f, 0x80);

Where did you get these bits from? According to the docs I have, 0x80 
enables BE swapping in general and 0x40 selects 16-bit vs 32-bit swaps. 
So the 16-bit case would rather be 0xc0. But I might be wrong, as the 
docs are vague.

Did you test 16-bit output?

> +		break;
> +	default:
> +		break;
> +	}
> +#endif
>   }
>   
>   static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
> index 30578e3b07e4..5c8c0fd2e229 100644
> --- a/drivers/gpu/drm/ast/ast_reg.h
> +++ b/drivers/gpu/drm/ast/ast_reg.h
> @@ -75,4 +75,10 @@
>   #define AST_IO_VGAIR1_R			(0x5A)
>   #define AST_IO_VGAIR1_VREFRESH		BIT(3)
>   
> +/*
> + * PCI Control
> + */
> +

No separate block please. Just put the define between  VGACRA1 and 
VGACRA3 above.

Please also add constants for setting the bits:

#define AST_IO_VGACRA2_BE_MODE        BIT(7)
#define AST_IO_VGACRA2_BE_MODE_16    BIT(6)

Best regards
Thomas

> +#define AST_IO_VGACRA2			(0xA2) /* PCI control & big-endian */
> +
>   #endif

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)