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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 03.12.25 um 11:14 schrieb René Rebe:
> (Resend w/o HTML, sorry!)
> Hi,
>
> thank you for the review!
>
>> On 3. Dec 2025, at 10:40, Thomas Zimmermann <tzimmermann@suse.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? ;-)

Yeah, that's what I meant.

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

OK, leave it in then.

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

The Matrox driver doesn't have hardware cursor planes. It only provides 
a primary plane and the cursor image is drawn in software by the compositor.

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

Yes, primary plane and cursor plane both use a shadow buffer in system 
memory that they copy into video memory on each display update. 
Essentially a memcpy with optimizations.  For the cursor plane, the 
driver also converts to ARGB4444, which is the only useful color format 
that the hardware supports. ARGB4444 is 16-bit wide per pixel, so we 
need to swap accordingly.

If the primary plane uses XRGB8888, it requires 32-bit swapping. Hence 
we have to set the correct mode in each plane's atomic_update helper 
before it updates video memory. It should work if you do it at the 
locations [2] and [3] that I pointed to.

Best regards
Thomas

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

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