Closed NiLuJe closed 3 years ago
lgtm-com[bot]
commented
3 years ago This pull request introduces 2 alerts when merging 24acf62e8eb495775051dd56ab041b5a8400718d into 9e6b568982dc628ff97c88631987ea977f4cef9e - view on LGTM.com
new alerts:
NiLuJe
commented
3 years ago A few significant quirks:
The framebuffer itself (as in, the character device) is now almost entirely meaningless. We basically only need it to tell us the screen's dimensions. This makes the whole FBFD_AUTO workflow kind of tricky, because we need a whole other set of things besides a fb fd to make it work... All of that extra stuff is kept internal, but if you're wondering why there's suddenly a few new and unfamiliar fds open, that would be the reason (they're all flagged CLOEXEC, though).
Another direct consequence of that fact is that there's no longer a public way to poke at the screen's work buffer, as each process gets one (or more, if you're really into crazy stuff) private buffer. If you just wanted screen grabs, see utils/sunxigrab.sh for a way to do that.
This also means that dealing with rotation has become even more hellish than usual, since you don't actually have any way of telling what's the layout of what is currently on the screen. Instead, I had to resort to some dirty hackery to poke directly at the accelerometer to tell us how the device is physically laid out. But that doesn't necessarily match the screen's layout... And, as a cherry on the cake, if you do attempt to display stuff in a different layout, all hell breaks loose, and none of the available workarounds are all that great. (Follow the breadcrumbs around fbink_toggle_sunxi_ntx_pen_mode at the bottom of the public header for more details).
That wasn't a surprise, because knowing other devices on sunxi SoCs, I was kind of expecting it, but, there is no hardware inversion support, there is no hardware dithering support (and software dithering selection is currently broken, more details below), and AUTO waveform mode selection is also not hardware accelerated, so, prefer specifying a mode explicitly.
The fact that each process works on a private buffer also makes anything that relied on "previous" buffer state meaninless in the CLI tool, since each invocation starts with a brand new black-filled buffer (a possibly non-exhaustive list, I'll update the CLI doc as I go. Note that this is mainly an issue for the CLI tool. API users get their own stable buffer, so, as long you're working on your own content, stuff behaves as usual.
--refresh, since it gets its own buffer, will only yield a solid black region.--norefresh useless, since each draw gets a different buffer, and the final refresh too ;p.--fgless / --bgless / --overlay drawing modes are also unusable, since they all rely on the existing state of the buffer to some extent, and that doesn't really work anymore.For API users, this affects what you can do with the dump & restore API calls. Documentation has been updated to mention those limitations.
I'm probably forgetting a few things, because I've lost count of how many times doing stuff wrong caused a kernel OOPS/BUG/hang/softlock/reboot. -_-"
NiLuJe
commented
3 years ago And now, ping @gtalusan, because I come bearing bug reports ;p.
In no particular order:
Kernel issues:
The ION_HEAP_QUERY ioctl is subtly broken, because of a mainline issue. c.f., https://github.com/NiLuJe/FBInk/blob/24acf62e8eb495775051dd56ab041b5a8400718d/utils/ion_heaps.c#L111-L116
(Granted, we don't need it, and, as the code above shows, we can get it to work, but it came in handy to triple-check a few ION things, and the fix is trivial ;)).
There's a rather nasty issue around the EINK_DITHERING_ constants: they all share a bit with EINK_DITHERING_Y1, which happens to be the bit that the if ladder checks for first in eink_image_process_thread @ drivers/video/fbdev/sunxi/disp2/disp/de/eink_buffer_manager.c, which effectively means that every dithering flag actually ends up being detected as this one.
c.f., https://github.com/NiLuJe/FBInk/blob/24acf62e8eb495775051dd56ab041b5a8400718d/fbink.c#L2619-L2624
Gnarlier, there's a similar bitmask issue with the waveform constants, this time: all the constants strictly greater than EINK_GLD16_MODE share at least one bit with other modes. In particular, the GLK/GCK ones share bits with A2, and, at a quick glance, there is code that checks and takes A2-specific code-paths after night-specific codepaths...
The issue is slightly gnarlier than the dithering one in that there is enough space in the first 16 bits (even without moving RECT & AUTO to higher bits) to fit all of these, but a lot of the code masks the wrong amount of bytes manually (& 0xFF) instead of using the GET_UPDATE_MODE macro...
In the same vein, and possibly related, GC4L & CLEAR currently crash the driver (first causes a timeout and then silent failures, and the device becomes extremely reluctant to soft reboot).
Something slightly more trivial that I just noticed when cruising by: in ring_buffer_manager_init (@ drivers/video/fbdev/sunxi/disp2/disp/de/eink_buffer_manager.c), in the error codepath, a buffer is freed with a wrong size (* 4) @ L3493 while it's allocated without a multiplier @ L3340...
nickel/pickel quirks:
pickel appears to be setting the heap_id_mask field in the ION_IOC_ALLOC ioctl to ION_HEAP_MASK_CARVEOUT|ION_HEAP_MASK_DMA. Is that on purpose (there is enough space left in the carveout heap, after all)? Right now DMA gets picked simply because it has higher priority thanks to its higher id (this is where ION_HEAP_QUERY came in handy ;p).
I haven't straced nickel yet, but it does also end up with an alloc in the DMA heap.
pickel passes a pointer instead of a value for the flags arg in the ION_IOC_ALLOC ioctl. Again, haven't checked nickel.
Both nickel & pickel pass a pointer instead of a value for the cfa_use arg in the DISP_EINK_UPDATE2 ioctl.
The ion stuff was sniffed via a patched strace, as was the disp stuff, although it's also evident in debug logging if you raise disp's debug level: https://github.com/NiLuJe/FBInk/blob/24acf62e8eb495775051dd56ab041b5a8400718d/fbink.c#L2571-L2575
gtalusan
commented
3 years ago Nickel uses its own accelerated dithering instead of the kernel's. You should too.
Don't confuse pointers with uninitialized/doesn't-crash-then-don't-care values.
gtalusan
commented
3 years ago Also CARVEOUT | DMA is probably definitely a bug on our side. CARVEOUT is the way to go. 💯 on that.
NiLuJe
commented
3 years ago Nickel uses its own accelerated dithering instead of the kernel's. You should too.
I usually am (well, without the "accelerated" part :D), but the EPDCv2 dithering support available on Mk. 7 was surprisingly decent ;).
Don't confuse pointers with uninitialized/doesn't-crash-then-don't-care values.
I was on the fence, but the fact that they appeared to be close to actual useful pointers in the same ioctl args and deref to "correct" (usually 0 ;p) values was strange ^^.
(Definitely on the "mostly harmless" side of things, though. Although I wouldn't necessarily trust all the cfa_use checks in the kernel to do the right thing...).
NiLuJe
commented
3 years ago Also CARVEOUT | DMA is probably definitely a bug on our side. CARVEOUT is the way to go. 100 on that.
Switched to the carveout in 934616f, at a very quick glance nothing new horribly blew up, thanks ;).
┌─(ROOT@europa:pts/1)──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────(~)─┐
└─(2.04:19%:21:55:100%:#)── cat /sys/kernel/debug/ion/heaps/carveout ──(Sun, Jul 11)─┘
client pid size
----------------------------------------------------
disp_ion 1 97714176
finger_trace 2908 2629632
----------------------------------------------------
orphaned allocations (info is from last known client):
----------------------------------------------------
total orphaned 0
total 100343808
deferred free 0
----------------------------------------------------This pull request introduces 2 alerts when merging 934616fee845b73ada9438bcb30197d73cbc8a7b into 9e6b568982dc628ff97c88631987ea977f4cef9e - view on LGTM.com
new alerts:
Well, that was not fun.
This change is