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.
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
