(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? ;-) > 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. >> 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. > 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
