Hi
Am 11.12.25 um 15:31 schrieb René Rebe:
Hi,
On Thu, 11 Dec 2025 15:03:48 +0100, Thomas Zimmermann <[email protected]>
wrote:
Hi,
Am 11.12.25 um 13:43 schrieb René Rebe:
[...]
The code for the primary plane should be fine now. But we also need
something for the cursor plane as well. There's a
ast_set_cursor_image() with a memcpy_toio() [1] and several additional
writes. IIUC they all have to be swapped as well.
Of course, any obvious style issue or endianess swapping linux-kernel
would like to see differently? You did not answer if I should just
conditionalize on the chip id. I used a bool to avoid intermangled
#ifdef conditionals to hopefully match kernel style.
Btw. checkpatch.pl warns:
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
I could add this if desired while at it.
Only compile tested, will do a final hw test once patch is approved in
general.
It's all a bit excessive. There's a patch attached that will hopefully
fix the issues.
If you could test it, I'll send it out for official review. The
easiest way of testing cursor support is to run Xorg and see if the
cursor looks correct.
The Co-developed-by tag requires your Signed-off-by.
Ok, so you are not a fan of using the hw swapping. I think I asked two
emails ago if having both pathes is acceptable. To be honest this
driver is for some reason already annoyingly slow. Buf of course we
can just keep the sw swapping for now.
I know. But it's all just a niche thing and I'm not sure if there's much
benefit for the HW swapper if Aspeed doesn't even care. This has been
going back and forth for some time. I'd really like to get it done ASAP.
Performance is mostly slow because of software rendering. Really
improving the copying in the kernel requires a larger commitment and
refactoring that is out of the scope of the patch. We can talk about
that, but changes would touch a lot of drivers.
/* write checksum + signature */
+ writel(swab32(csum), dst);
+ writel(swab32(width), dst + AST_HWC_SIGNATURE_SizeX);
+ writel(swab32(height), dst + AST_HWC_SIGNATURE_SizeY);
+ writel(swab32(0), dst + AST_HWC_SIGNATURE_HOTSPOTX);
+ writel(swab32(0), dst + AST_HWC_SIGNATURE_HOTSPOTY);
+#else
+ memcpy_toio(dst, src, AST_HWC_SIZE);
dst += AST_HWC_SIZE;
+
+ /* write checksum + signature */
writel(csum, dst);
writel(width, dst + AST_HWC_SIGNATURE_SizeX);
writel(height, dst + AST_HWC_SIGNATURE_SizeY);
writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTX);
writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY);
+#endif
I'm pretty sure this will break the cursor, as the position was
working correctly and I only had to swap the cursor image data. The
csum will also not be identical anyway, as the checksum function
computes it in native byte order. Theoretically that would have to be
changed. However, I do not see where it is really used, maybe only
some special remote desktop vendor protocol that I'm not using. Maybe
the exact checksum does not even matter and is only used as
optimization to not resend an unchanged cursor image.
Oh well! I though that the bus does implicit byte swaps? Or does
writel() already swap to little endian, which the AST chip expects? I'm
confused.
Best regards
Thomas
I'll send a final version after validating it w/ HW later.
Thanks,
René
--
--
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)
