| Message ID | 20230512084152.31233-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/fbdev: Remove DRM's helpers for fbdev I/O | expand |
Hi Thomas, On Fri, May 12, 2023 at 10:41:41AM +0200, Thomas Zimmermann wrote: > DRM provides a number of wrappers around fbdev cfb_() sys_(), fb_io_() > and fb_sys_() helpers. The DRM functions don't provide any additional > functionality for most DRM drivers. So remove them and call the fbdev > I/O helpers directly. > > The DRM fbdev I/O wrappers were originally added because <linux/fb.h> > does not protect its content with CONFIG_FB. DRM fbdev emulation did > not build if the the config option had been disabled. This has been > fixed. For fbdev-generic and i915, the wrappers added support for damage > handling. But this is better handled within the two callers, as each > is special in its damage handling. > > Patches 1 to 8 replace the DRM wrappers in a number of fbdev emulations. > Patch 9 exports two helpers for damage handling. Patches 10 and 11 > update fbdev-generic and i915 with the help of the exported functions. > The patches also remove DRM's fbdev I/O helpers, which are now unused. > > DRM's fbdev helpers had to select fbdev I/O helpers for I/O and for > system memory. Each fbdev emulation now selects the correct helpers > for itself. Depending on the selected DRM drivers, kernel builds will > now only contain the necessary fbdev I/O helpers and might be slightly > smaller in size. Nice cleanup. From one of the patches: > +config DRM_ARMADA_FBDEV_EMULATION > + bool > + depends on DRM_ARMADA > + select FB_CFB_COPYAREA > + select FB_CFB_FILLRECT > + select FB_CFB_IMAGEBLIT This seems like a hard to maintain way to select a few helper functions. Today we have LD_DEAD_CODE_DATA_ELIMINATION for the configs that care about size - and that should work here as well. I understand where this comes from and I am not against the solution, but wanted to point at a more modern approach to deal with the bloat. Maybe some of the embbedded folks can tell if LD_DEAD_CODE_DATA_ELIMINATION can be trusted yet or that is something for the future. In barebox -ffunction-section (what LD_DEAD_CODE_DATA_ELIMINATION enables) is used with success - there it really helps when generating different barebox images where size matters a lot. Sam
Hi Sam Am 12.05.23 um 12:29 schrieb Sam Ravnborg: > Hi Thomas, > > On Fri, May 12, 2023 at 10:41:41AM +0200, Thomas Zimmermann wrote: >> DRM provides a number of wrappers around fbdev cfb_() sys_(), fb_io_() >> and fb_sys_() helpers. The DRM functions don't provide any additional >> functionality for most DRM drivers. So remove them and call the fbdev >> I/O helpers directly. >> >> The DRM fbdev I/O wrappers were originally added because <linux/fb.h> >> does not protect its content with CONFIG_FB. DRM fbdev emulation did >> not build if the the config option had been disabled. This has been >> fixed. For fbdev-generic and i915, the wrappers added support for damage >> handling. But this is better handled within the two callers, as each >> is special in its damage handling. >> >> Patches 1 to 8 replace the DRM wrappers in a number of fbdev emulations. >> Patch 9 exports two helpers for damage handling. Patches 10 and 11 >> update fbdev-generic and i915 with the help of the exported functions. >> The patches also remove DRM's fbdev I/O helpers, which are now unused. >> >> DRM's fbdev helpers had to select fbdev I/O helpers for I/O and for >> system memory. Each fbdev emulation now selects the correct helpers >> for itself. Depending on the selected DRM drivers, kernel builds will >> now only contain the necessary fbdev I/O helpers and might be slightly >> smaller in size. > > Nice cleanup. > > From one of the patches: > >> +config DRM_ARMADA_FBDEV_EMULATION >> + bool >> + depends on DRM_ARMADA >> + select FB_CFB_COPYAREA >> + select FB_CFB_FILLRECT >> + select FB_CFB_IMAGEBLIT > > This seems like a hard to maintain way to select a few helper functions. > Today we have LD_DEAD_CODE_DATA_ELIMINATION for the configs that care > about size - and that should work here as well. I wasn't too happy about this solution either as it is quite verbose. But I don't want to rely on the linker either. It certainly cannot remove exported symbols. But the pattern is very common among the fbdev drivers. We could introduce common Kconfig options in fbdev and selcet those instead. Like this: const FB_IO_HELPERS bool depends on FB select FB_CFB_COPYAREA select FB_CFB_FILLRECT select FB_CFB_IMAGEBLIT const FB_SYS_HELPERS bool depends on FB select FB_SYS_COPYAREA select FB_SYS_FILLRECT select FB_SYS_FOPS select FB_SYS_IMAGEBLIT Apart from DRM, most of the fbdev drivers could use these as well. Best regards Thomas > > I understand where this comes from and I am not against the > solution, but wanted to point at a more modern approach to deal with the > bloat. > > Maybe some of the embbedded folks can tell if LD_DEAD_CODE_DATA_ELIMINATION > can be trusted yet or that is something for the future. > > In barebox -ffunction-section (what LD_DEAD_CODE_DATA_ELIMINATION > enables) is used with success - there it really helps when generating > different barebox images where size matters a lot. > > Sam
Hi Thomas, > > > > Nice cleanup. > > > > From one of the patches: > > > > > +config DRM_ARMADA_FBDEV_EMULATION > > > + bool > > > + depends on DRM_ARMADA > > > + select FB_CFB_COPYAREA > > > + select FB_CFB_FILLRECT > > > + select FB_CFB_IMAGEBLIT > > > > This seems like a hard to maintain way to select a few helper functions. > > Today we have LD_DEAD_CODE_DATA_ELIMINATION for the configs that care > > about size - and that should work here as well. > > I wasn't too happy about this solution either as it is quite verbose. But I > don't want to rely on the linker either. It certainly cannot remove exported > symbols. I forgot about exported symbols - that makes the idea futile. > > But the pattern is very common among the fbdev drivers. We could introduce > common Kconfig options in fbdev and selcet those instead. Like this: > > const FB_IO_HELPERS > bool > depends on FB > select FB_CFB_COPYAREA > select FB_CFB_FILLRECT > select FB_CFB_IMAGEBLIT > > const FB_SYS_HELPERS > bool > depends on FB > select FB_SYS_COPYAREA > select FB_SYS_FILLRECT > select FB_SYS_FOPS > select FB_SYS_IMAGEBLIT > > Apart from DRM, most of the fbdev drivers could use these as well. That's a much nicer way to express it - and with this we do not introduce the IMO confusing CFB (Color Frame Buffer) abbreviation in every driver. Sam