Hello Thomas!
Wanted to answer earlier but things took time, and a lot more than expected.
> 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.
Meanwhile I found out that this implementation had corner cases. I also
expected original implementations be a bit more complete.
> 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.
In the new version I resorted to only generate tables with them, in
close proximity. The mentioned part made me think when I first run into
it, btw.
> 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.
I've noticed the screen base vs. buffer issue back then and was already
corrected. But it's handled more cleanly now.
> 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.
In the new version I made it very hard to get the alignment wrong.
> 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.
I've used such callbacks but not for the read/writes as that would have
made the parameter list huge, in terms of lines. Not to mention passing
them down to lowest level.
> 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:
If done slightly differently the compiler inlines these and there's no
overhead.
> 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.
The vertical loops are small, but I kept them separate from the scanline
rendering part.
Thanks for the tips, that was really helpful and used them when applicable.
While the updated version is not quite so as described I hope it isn't
too bad either.
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
