| Message ID | 20250207041818.4031-1-soci@c64.rulez.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | fbdev: core: Deduplicate cfb/sys drawing fbops | expand |
On 2/7/25 05:18, Zsolt Kajtar wrote: > In 68648ed1f58d98b8e8d994022e5e25331fbfe42a the drawing routines were > duplicated to have separate I/O and system memory versions. > > Later the pixel reversing in 779121e9f17525769c04a00475fd85600c8c04eb > was only added to the I/O version and not to system. > > That's unfortunate as reversing is not something only applicable for > I/O memory and I happen to need both I/O and system version now. > > One option is to bring the system version up to date, but from the > maintenance perspective it's better to not have two versions in the > first place. > > The drawing routines (based on the cfb version) were moved to header > files. These are now included in both cfb and sys modules. The memory > access and other minor differences were handled with a few macros. > > The last patch adds a separate config option for the system version. > > Zsolt Kajtar (13): > fbdev: core: Copy cfbcopyarea to fb_copyarea > fbdev: core: Make fb_copyarea generic > fbdev: core: Use generic copyarea for as cfb_copyarea > fbdev: core: Use generic copyarea for as sys_copyarea > fbdev: core: Copy cfbfillrect to fb_fillrect > fbdev: core: Make fb_fillrect generic > fbdev: core: Use generic fillrect for as cfb_fillrect > fbdev: core: Use generic fillrect for as sys_fillrect > fbdev: core: Copy cfbimgblt to fb_imageblit > fbdev: core: Make fb_imageblit generic > fbdev: core: Use generic imageblit for as cfb_imageblit > fbdev: core: Use generic imageblit for as sys_imageblit > fbdev: core: Split CFB and SYS pixel reversing configuration > > drivers/video/fbdev/core/Kconfig | 10 +- > drivers/video/fbdev/core/cfbcopyarea.c | 427 +----------------------- > drivers/video/fbdev/core/cfbfillrect.c | 363 +------------------- > drivers/video/fbdev/core/cfbimgblt.c | 358 +------------------- > drivers/video/fbdev/core/fb_copyarea.h | 421 +++++++++++++++++++++++ > drivers/video/fbdev/core/fb_draw.h | 6 +- > drivers/video/fbdev/core/fb_fillrect.h | 359 ++++++++++++++++++++ > drivers/video/fbdev/core/fb_imageblit.h | 356 ++++++++++++++++++++ > drivers/video/fbdev/core/syscopyarea.c | 358 +------------------- > drivers/video/fbdev/core/sysfillrect.c | 315 +---------------- > drivers/video/fbdev/core/sysimgblt.c | 326 +----------------- > 11 files changed, 1208 insertions(+), 2091 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_copyarea.h > create mode 100644 drivers/video/fbdev/core/fb_fillrect.h > create mode 100644 drivers/video/fbdev/core/fb_imageblit.h It's a bigger change. I've applied the series to the fbdev for-next git tree to give it some compile- and runtime testing. Helge
Hi Am 07.02.25 um 05:18 schrieb Zsolt Kajtar: > In 68648ed1f58d98b8e8d994022e5e25331fbfe42a the drawing routines were > duplicated to have separate I/O and system memory versions. > > Later the pixel reversing in 779121e9f17525769c04a00475fd85600c8c04eb > was only added to the I/O version and not to system. > > That's unfortunate as reversing is not something only applicable for > I/O memory and I happen to need both I/O and system version now. > > One option is to bring the system version up to date, but from the > maintenance perspective it's better to not have two versions in the > first place. No it's not. Major code abstractions behind preprocessor tokens are terrible to maintain. It's also technically not possible to switch between system and I/O memory at will. These are very different things. If you want that pixel-reversing feature in sys_ helpers, please implement it there. Sorry, but NAK on this series. Best regards Thomas > > The drawing routines (based on the cfb version) were moved to header > files. These are now included in both cfb and sys modules. The memory > access and other minor differences were handled with a few macros. > > The last patch adds a separate config option for the system version. > > Zsolt Kajtar (13): > fbdev: core: Copy cfbcopyarea to fb_copyarea > fbdev: core: Make fb_copyarea generic > fbdev: core: Use generic copyarea for as cfb_copyarea > fbdev: core: Use generic copyarea for as sys_copyarea > fbdev: core: Copy cfbfillrect to fb_fillrect > fbdev: core: Make fb_fillrect generic > fbdev: core: Use generic fillrect for as cfb_fillrect > fbdev: core: Use generic fillrect for as sys_fillrect > fbdev: core: Copy cfbimgblt to fb_imageblit > fbdev: core: Make fb_imageblit generic > fbdev: core: Use generic imageblit for as cfb_imageblit > fbdev: core: Use generic imageblit for as sys_imageblit > fbdev: core: Split CFB and SYS pixel reversing configuration > > drivers/video/fbdev/core/Kconfig | 10 +- > drivers/video/fbdev/core/cfbcopyarea.c | 427 +----------------------- > drivers/video/fbdev/core/cfbfillrect.c | 363 +------------------- > drivers/video/fbdev/core/cfbimgblt.c | 358 +------------------- > drivers/video/fbdev/core/fb_copyarea.h | 421 +++++++++++++++++++++++ > drivers/video/fbdev/core/fb_draw.h | 6 +- > drivers/video/fbdev/core/fb_fillrect.h | 359 ++++++++++++++++++++ > drivers/video/fbdev/core/fb_imageblit.h | 356 ++++++++++++++++++++ > drivers/video/fbdev/core/syscopyarea.c | 358 +------------------- > drivers/video/fbdev/core/sysfillrect.c | 315 +---------------- > drivers/video/fbdev/core/sysimgblt.c | 326 +----------------- > 11 files changed, 1208 insertions(+), 2091 deletions(-) > create mode 100644 drivers/video/fbdev/core/fb_copyarea.h > create mode 100644 drivers/video/fbdev/core/fb_fillrect.h > create mode 100644 drivers/video/fbdev/core/fb_imageblit.h >
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! > 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.
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.