pixman_blt on aarch64

pixman_blt on aarch64

[BALATON Zoltan]

Hello,

I'm trying to involve the pixman list in this thread on qemu-devel list 
started with subject "Display update issue on M1 Macs". See here:

https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg01033.html

We have found that on aarch64 Macs running macOS the pixman_blt and 
pixman_fill functions are disabled without fallback due to not being able 
to compile the needed assembly code. See detailed discussion below.

Is there a way to fix this in pixman in the near future or provide a 
fallback for this in pixman? Or do I need to add a fallback in QEMU or try 
using something else instead of pixman for these functions?

Thank you,
BALATON Zoltan

On Sat, 4 Feb 2023, Akihiko Odaki wrote:
> On 2023/02/03 22:45, BALATON Zoltan wrote:
>> On Fri, 3 Feb 2023, Akihiko Odaki wrote:
>>> I finally reproduced the issue with MorphOS and ati-vga and figured out 
>>> its cause.
>>> 
>>> The problem is that pixman_blt() is disabled because its backend is 
>>> written in GNU assembly, and GNU assembler is not available on macOS. 
>>> There is no fallback written in C, unfortunately. The issue is tracked by 
>>> the upstream at:
>>> https://gitlab.freedesktop.org/pixman/pixman/-/issues/59
>> 
>> Hm, OK but that ticket is just about compile error and suggests to disable 
>> it and does not say it won't work then. Are they aware this is a problem? 
>> Maybe we should write to their mailing list after we're sure what's 
>> happening.
>
> That's a good idea. They may prioritize the issue if they realize that 
> disables pixman_blt().
>
>>> I hit the same problem on Asahi Linux, which is based on Arch Linux ARM. 
>>> It is because Arch Linux copied PKGBUILD from x86 Arch Linux, which 
>>> disables Arm backends. It is easy to enable the backend for the platform 
>>> so I proposed a change at:
>>> https://github.com/archlinuxarm/PKGBUILDs/pull/1985
>> 
>> On macOS one source of pixman most people use is brew.sh where this seems 
>> to be disabled:
>> 
>> https://github.com/Homebrew/homebrew-core/blob/master/Formula/pixman.rb
>> 
>> another source is macports which has an older version and no such options:
>> 
>> https://github.com/macports/macports-ports/blob/master/graphics/libpixman-devel/Portfile
>> 
>> I wonder if it compiles from macports on aarch64 then.
>
> It's more likely that it is just outdated. It does not carry a patch to fix 
> the issue.
>
>> I wait if I can get some more test results and try to check pixman but its 
>> source is not too clear to me and there are no docs either so maybe the 
>> best way is to ask on their list. If this is a pixman issue I hope it can 
>> be fixed there and we don't need to implement a fallback in QEMU.
>
> This is certainly a pixman issue.
>
> If you read the source, you can see pixman_blt() calls 
> _pixman_implementation_blt(). _pixman_implementation_blt() calls blt member 
> of pixman_implementation_t in turn. Grepping for "blt =" tells it is only 
> assigned in:
> pixman/pixman-arm-neon.c
> pixman/pixman-arm-simd.c
> pixman/pixman-mips-dspr2.c
> pixman/pixman-mmx.c
> pixman/pixman-sse2.c
>
> For AArch64, only pixman/pixman-arm-neon.c is relevant, and it needs to be 
> disabled to build the library on macOS.
>
> Regards,
> Akihiko Odaki
>
>> 
>> Regards,
>> BALATON Zoltan

Re: pixman_blt on aarch64

[BALATON Zoltan]

This has just bounced, I hoped to still be able to post after moderation 
but now I'm resending it after subscribing to the pixman list. Meanwhile 
I've found this ticket as well: 
https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
See the rest of the message below. Looks like this is being worked on but 
I'm not sure how far is it from getting resolved. Any info on that?

On Sat, 4 Feb 2023, BALATON Zoltan wrote:
> Hello,
>
> I'm trying to involve the pixman list in this thread on qemu-devel list 
> started with subject "Display update issue on M1 Macs". See here:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg01033.html
>
> We have found that on aarch64 Macs running macOS the pixman_blt and 
> pixman_fill functions are disabled without fallback due to not being able to 
> compile the needed assembly code. See detailed discussion below.
>
> Is there a way to fix this in pixman in the near future or provide a fallback 
> for this in pixman? Or do I need to add a fallback in QEMU or try using 
> something else instead of pixman for these functions?
>
> Thank you,
> BALATON Zoltan
>
> On Sat, 4 Feb 2023, Akihiko Odaki wrote:
>> On 2023/02/03 22:45, BALATON Zoltan wrote:
>>> On Fri, 3 Feb 2023, Akihiko Odaki wrote:
>>>> I finally reproduced the issue with MorphOS and ati-vga and figured out 
>>>> its cause.
>>>> 
>>>> The problem is that pixman_blt() is disabled because its backend is 
>>>> written in GNU assembly, and GNU assembler is not available on macOS. 
>>>> There is no fallback written in C, unfortunately. The issue is tracked by 
>>>> the upstream at:
>>>> https://gitlab.freedesktop.org/pixman/pixman/-/issues/59
>>> 
>>> Hm, OK but that ticket is just about compile error and suggests to disable 
>>> it and does not say it won't work then. Are they aware this is a problem? 
>>> Maybe we should write to their mailing list after we're sure what's 
>>> happening.
>> 
>> That's a good idea. They may prioritize the issue if they realize that 
>> disables pixman_blt().
>> 
>>>> I hit the same problem on Asahi Linux, which is based on Arch Linux ARM. 
>>>> It is because Arch Linux copied PKGBUILD from x86 Arch Linux, which 
>>>> disables Arm backends. It is easy to enable the backend for the platform 
>>>> so I proposed a change at:
>>>> https://github.com/archlinuxarm/PKGBUILDs/pull/1985
>>> 
>>> On macOS one source of pixman most people use is brew.sh where this seems 
>>> to be disabled:
>>> 
>>> https://github.com/Homebrew/homebrew-core/blob/master/Formula/pixman.rb
>>> 
>>> another source is macports which has an older version and no such options:
>>> 
>>> https://github.com/macports/macports-ports/blob/master/graphics/libpixman-devel/Portfile
>>> 
>>> I wonder if it compiles from macports on aarch64 then.
>> 
>> It's more likely that it is just outdated. It does not carry a patch to fix 
>> the issue.
>> 
>>> I wait if I can get some more test results and try to check pixman but its 
>>> source is not too clear to me and there are no docs either so maybe the 
>>> best way is to ask on their list. If this is a pixman issue I hope it can 
>>> be fixed there and we don't need to implement a fallback in QEMU.
>> 
>> This is certainly a pixman issue.
>> 
>> If you read the source, you can see pixman_blt() calls 
>> _pixman_implementation_blt(). _pixman_implementation_blt() calls blt member 
>> of pixman_implementation_t in turn. Grepping for "blt =" tells it is only 
>> assigned in:
>> pixman/pixman-arm-neon.c
>> pixman/pixman-arm-simd.c
>> pixman/pixman-mips-dspr2.c
>> pixman/pixman-mmx.c
>> pixman/pixman-sse2.c
>> 
>> For AArch64, only pixman/pixman-arm-neon.c is relevant, and it needs to be 
>> disabled to build the library on macOS.
>> 
>> Regards,
>> Akihiko Odaki
>> 
>>> 
>>> Regards,
>>> BALATON Zoltan
>

Re: pixman_blt on aarch64

[Richard Henderson]

On 2/4/23 06:57, BALATON Zoltan wrote:
> This has just bounced, I hoped to still be able to post after moderation but now I'm 
> resending it after subscribing to the pixman list. Meanwhile I've found this ticket as 
> well: https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
> See the rest of the message below. Looks like this is being worked on but I'm not sure how 
> far is it from getting resolved. Any info on that?

Please try this:

https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general

It provides a pure C version for ultimate fallback.
Unfortunately, there are no test cases for this, nor documentation.


r~

Re: pixman_blt on aarch64

[BALATON Zoltan]

On Sun, 5 Feb 2023, Richard Henderson wrote:
> On 2/4/23 06:57, BALATON Zoltan wrote:
>> This has just bounced, I hoped to still be able to post after moderation 
>> but now I'm resending it after subscribing to the pixman list. Meanwhile 
>> I've found this ticket as well: 
>> https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
>> See the rest of the message below. Looks like this is being worked on but 
>> I'm not sure how far is it from getting resolved. Any info on that?
>
> Please try this:
>
> https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general
>
> It provides a pure C version for ultimate fallback.
> Unfortunately, there are no test cases for this, nor documentation.

Thanks, I don't have hardware to test this but maybe Akihiko or somebody 
else here cam try. Do you think pixman_fill won't have the same problem? 
It seems to have at least a fast_path implementation but I'm not sure how 
pixman selects these.

Regards,
BALATON Zoltan

Re: pixman_blt on aarch64

[Richard Henderson]

On 2/5/23 08:44, BALATON Zoltan wrote:
> On Sun, 5 Feb 2023, Richard Henderson wrote:
>> On 2/4/23 06:57, BALATON Zoltan wrote:
>>> This has just bounced, I hoped to still be able to post after moderation but now I'm 
>>> resending it after subscribing to the pixman list. Meanwhile I've found this ticket as 
>>> well: https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
>>> See the rest of the message below. Looks like this is being worked on but I'm not sure 
>>> how far is it from getting resolved. Any info on that?
>>
>> Please try this:
>>
>> https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general
>>
>> It provides a pure C version for ultimate fallback.
>> Unfortunately, there are no test cases for this, nor documentation.
> 
> Thanks, I don't have hardware to test this but maybe Akihiko or somebody else here cam 
> try. Do you think pixman_fill won't have the same problem? It seems to have at least a 
> fast_path implementation but I'm not sure how pixman selects these.

For fill, I think the fast_path implementation should work, so long as it isn't disabled 
via environment variable.  I'm not sure why that is, and why _fast_path isn't part of 
_general.

Indeed, the fast_path implementation of fill should be easily vectorized by the compiler. 
I would expect it to be competitive with an assembly implementation.  I would expect the 
implementation chain design to only be useful when multiple vector implementations are 
supported and selected at runtime -- e.g. the x86 SSE2 vs SSSE3 stuff.


r~

Re: pixman_blt on aarch64

[Akihiko Odaki]

On 2023/02/06 4:16, Richard Henderson wrote:
> On 2/5/23 08:44, BALATON Zoltan wrote:
>> On Sun, 5 Feb 2023, Richard Henderson wrote:
>>> On 2/4/23 06:57, BALATON Zoltan wrote:
>>>> This has just bounced, I hoped to still be able to post after 
>>>> moderation but now I'm resending it after subscribing to the pixman 
>>>> list. Meanwhile I've found this ticket as well: 
>>>> https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
>>>> See the rest of the message below. Looks like this is being worked 
>>>> on but I'm not sure how far is it from getting resolved. Any info on 
>>>> that?
>>>
>>> Please try this:
>>>
>>> https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general
>>>
>>> It provides a pure C version for ultimate fallback.
>>> Unfortunately, there are no test cases for this, nor documentation.

It can share the implementation with fast_composite_src_memcpy(). 
fast_composite_src_memcpy() should be well-tested with the tests for 
pixman_image_composite(). arm-neon does similar so we can trust 
fast_composite_src_memcpy() functions as blt.

>>
>> Thanks, I don't have hardware to test this but maybe Akihiko or 
>> somebody else here cam try. Do you think pixman_fill won't have the 
>> same problem? It seems to have at least a fast_path implementation but 
>> I'm not sure how pixman selects these.
> 
> For fill, I think the fast_path implementation should work, so long as 
> it isn't disabled via environment variable.  I'm not sure why that is, 
> and why _fast_path isn't part of _general.

The implementation of fill should be moved to pixman-general.c but the 
other part of pixman-fast-path.c shouldn't be.

By isolating the non-essential fast-path code to pixman-fast-path.c, you 
can disable it with the environment variable when you are not confident 
with the implementation, and that may help debugging. However, if 
pixman-fast-path.c has some essential code like the implementation of 
fill, the utility of the environment variable will be impaired as 
setting the environment variable may break things.

> 
> Indeed, the fast_path implementation of fill should be easily vectorized 
> by the compiler. I would expect it to be competitive with an assembly 
> implementation.  I would expect the implementation chain design to only 
> be useful when multiple vector implementations are supported and 
> selected at runtime -- e.g. the x86 SSE2 vs SSSE3 stuff.
> 
> 
> r~

Re: pixman_blt on aarch64

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]

Maybe we should include pixman list in this. In case you're not subscribed 
I'm forwarding it to that list now.

On Tue, 7 Feb 2023, Akihiko Odaki wrote:
> On 2023/02/06 4:16, Richard Henderson wrote:
>> On 2/5/23 08:44, BALATON Zoltan wrote:
>>> On Sun, 5 Feb 2023, Richard Henderson wrote:
>>>> On 2/4/23 06:57, BALATON Zoltan wrote:
>>>>> This has just bounced, I hoped to still be able to post after moderation 
>>>>> but now I'm resending it after subscribing to the pixman list. Meanwhile 
>>>>> I've found this ticket as well: 
>>>>> https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
>>>>> See the rest of the message below. Looks like this is being worked on 
>>>>> but I'm not sure how far is it from getting resolved. Any info on that?
>>>> 
>>>> Please try this:
>>>> 
>>>> https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general
>>>> 
>>>> It provides a pure C version for ultimate fallback.
>>>> Unfortunately, there are no test cases for this, nor documentation.
>
> It can share the implementation with fast_composite_src_memcpy(). 
> fast_composite_src_memcpy() should be well-tested with the tests for 
> pixman_image_composite(). arm-neon does similar so we can trust 
> fast_composite_src_memcpy() functions as blt.
>
>>> 
>>> Thanks, I don't have hardware to test this but maybe Akihiko or somebody 
>>> else here cam try. Do you think pixman_fill won't have the same problem? 
>>> It seems to have at least a fast_path implementation but I'm not sure how 
>>> pixman selects these.
>> 
>> For fill, I think the fast_path implementation should work, so long as it 
>> isn't disabled via environment variable.  I'm not sure why that is, and why 
>> _fast_path isn't part of _general.
>
> The implementation of fill should be moved to pixman-general.c but the other 
> part of pixman-fast-path.c shouldn't be.
>
> By isolating the non-essential fast-path code to pixman-fast-path.c, you can 
> disable it with the environment variable when you are not confident with the 
> implementation, and that may help debugging. However, if pixman-fast-path.c 
> has some essential code like the implementation of fill, the utility of the 
> environment variable will be impaired as setting the environment variable may 
> break things.
>
>> 
>> Indeed, the fast_path implementation of fill should be easily vectorized by 
>> the compiler. I would expect it to be competitive with an assembly 
>> implementation.  I would expect the implementation chain design to only be 
>> useful when multiple vector implementations are supported and selected at 
>> runtime -- e.g. the x86 SSE2 vs SSSE3 stuff.
>> 
>> 
>> r~
>
>

Re: pixman_blt on aarch64

[BALATON Zoltan]

On Sun, 5 Feb 2023, BALATON Zoltan wrote:
> On Sun, 5 Feb 2023, Richard Henderson wrote:
>> On 2/4/23 06:57, BALATON Zoltan wrote:
>>> This has just bounced, I hoped to still be able to post after moderation 
>>> but now I'm resending it after subscribing to the pixman list. Meanwhile 
>>> I've found this ticket as well: 
>>> https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71
>>> See the rest of the message below. Looks like this is being worked on but 
>>> I'm not sure how far is it from getting resolved. Any info on that?
>> 
>> Please try this:
>> 
>> https://gitlab.freedesktop.org/rth7680/pixman/-/tree/general
>> 
>> It provides a pure C version for ultimate fallback.
>> Unfortunately, there are no test cases for this, nor documentation.
>
> Thanks, I don't have hardware to test this but maybe Akihiko or somebody else 
> here cam try. Do you think pixman_fill won't have the same problem? It seems 
> to have at least a fast_path implementation but I'm not sure how pixman 
> selects these.

We have tried the branch above and while it does make it better and usable 
in 16bit mode, 8bit is still missing (no wonder as it does not have that 
implemented but the AmigaOS driver only allows 16bit and 8bit with the 
sm501 device this is used in but probably that's not important when 16 bit 
works). Even in 16bit more there were some graphics problems seen but that 
may need some more checking to make sure it's not some other change as the 
report was not clear if that worked before. I've asked to check the 
changes in 
https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/71 next to 
see if that would work better.

Regards,
BALATON Zoltan