| Message ID | 20250207041818.4031-1-soci@c64.rulez.org |
|---|---|
| Headers | show |
| Series | fbdev: core: Deduplicate cfb/sys drawing fbops | expand |
Hi Am 08.02.25 um 01:51 schrieb Kajtár Zsolt: > Hello Thomas! > >> No it's not. Major code abstractions behind preprocessor tokens are >> terrible to maintain. > Hmm, don't get me wrong but I'm not sure if the changes were really > checked in detail. At first sight it might look like I'm adding tons of > new macro ridden code in those header files replacing cleaner code. > > While actually that's just how the I/O version currently is, copied and > white space cleaned (as it was requested) plus comment style matched > with sys. > > The only new thing which hides the mentioned abstraction a little more > is FB_MEM, which replaced __iomem. But that's a tradeoff to be able to > use the same source for system as well. > > Or the concern is that now system memory specific code might get mixed > in there by mistake? > > It was not planned as the final version, the current maintainer asked > for addressing some pre-existing quality issues with further patches but > otherwise accepted the taken approach. > >> It's also technically not possible to switch between system and I/O >> memory at will. These are very different things. > Yes, there are architectures where these two don't mix at all, I'm aware > of that. I need that on x86 only (for old hw), and there it seems > doable. Depending on the resolution either the aperture or the defio > memory is mapped. If the framebuffer is not remapped after a mode change > that's an application bug. Otherwise it's harmless as both are always > there and don't change. > > I'd better like to find out problems sooner than later, so if you or > anyone else could share any concerns that'd be really helpful! First of all, commit 779121e9f175 ("fbdev: Support for byte-reversed framebuffer formats") isn't super complicated AFAICT. I can be implemented in the sys_ helpers as well. It seems like you initially did that. About the series at hand: generating code by macro expansion is good for simple cases. I've done that in several places within fbdev myself, such as [1]. But if the generated code requires Turing-completeness, it becomes much harder to see through the macros and understand what is going on. This makes code undiscoverable; and discoverability is a requirement for maintenance. [1] https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/fb.h#L700 Then there's type-safety and type-casting. The current series defeats it by casting various pointers to whatever the macros define. For example, looking at the copyarea patches, they use screen_base [2] from struct fb_info. The thing is, using screen_base is wrong for sys_copyarea(). The function should use 'screen_buffer' instead. It works because both fields share the same bits of a union. Using screen_base is a bug in the current implementation that should be fixed, while this patch series would set it in stone. [2] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/video/fbdev/core/syscopyarea.c#L340 Next, if you look through the commit history, you'll find that there are several commits with performance improvements. Memory access in the sys variants is not guaranteed to be 32-bit aligned by default. The compiler has to assume unaligned access, which results in slower code. Hence, some manual intervention has to be done. It's too easy to accidentally mess this up by using nontransparent macros for access. If you want to do meaningful work here, please do actual refactoring instead of throwing unrelated code together. First of all, never use macros, but functions. You can supply callback functions to access the framebuffer. Each callback should know whether it operates on screen_base or screen_buffer. But using callbacks for individual reads and writes can have runtime overhead. It's better to operate on complete scanlines. The current helpers are already organized that way. Again, from the copyarea helper: sys_copyarea() { // first prepare // then go through the scanlines while (height) { do_something_for_the_current_scanline(). } } The inner helper do_something_...() has to be written for various cfb and sys cases and can be given as function pointer to a generic helper. Best regards Thomas > >> If you want that pixel-reversing feature in sys_ helpers, please >> implement it there. > Actually that's what I did first. Then did it once more by adapting the > I/O version as that gave me more confidence that it'll work exactly the > same and there's less room for error.