Message-Id: <0D9D1FD3-F716-40E2-B412-BAEA990DF8C0@exactco.de>
Date: Wed, 3 Dec 2025 11:14:26 +0100
From: René Rebe <rene@...ctco.de>
To: Thomas Zimmermann <tzimmermann@...e.de>
Cc: dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org,
 Dave Airlie <airlied@...hat.com>
Subject: Re: [PATCH] drm/ast: Fix big-endian support

(Resend w/o HTML, sorry!)
Hi,

thank you for the review!

> On 3. Dec 2025, at 10:40, Thomas Zimmermann <tzimmermann@...e.de> wrote:
> 
> 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.

indeed.

>> big-endian RISC systems. Fix by enabling byte swapping for any
>> __BIG_ENDIAN config.
> 
> Is this the RISC support that Linux famously shot down? :)

Linux shut down RISC? Or Linus the non existing big-endian RISCV? ;-)

> 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.

Well, in practice this machines are broken for years, would probably be nice to finally get this to production datacenter running Power 9, … or the last SPARC users.

>> Signed-off-by: René Rebe <rene@...ctco.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.

Ah.

> 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.

The cursor was correct in X, though I tested w/ the mag driver (which I also fixed some more). I’ll try the modesetting driver, too.

> 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

I was not considering buffers in different modes. Can there be different VRAM access of different formats with this simple driver? Because 16 and 32-bit planes / buffers will not work concurrently with global byte-swapping at the same time, ...

>> + 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.

From the spec I found with Google, but I think you are indeed correct, I somehow missed that bit that day.

> 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)

Sure will update, I was just using the style already present with reg magic numbers all over the place :-/

Thanks!
René

> 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)
> 
> 

-- 
https://exactco.de • https://t2linux.com • https://patreon.com/renerebe

Powered by blists - more mailing lists