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 <[email protected]> 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.zim...@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 <[email protected]>
---
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)
