"Arnd Bergmann" <arnd@xxxxxxxx> writes: Hello Arnd, Thanks for your review! > On Fri, Jun 30, 2023, at 00:51, Javier Martinez Canillas wrote: >> Currently the CONFIG_FB option has to be enabled even if no legacy fbdev >> drivers are needed (e.g: only to have support for framebuffer consoles). >> >> The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB >> and so it can only be enabled if that dependency is enabled as well. >> >> That means fbdev drivers have to be explicitly disabled if users want to >> enable CONFIG_FB, only to use fbcon and/or the DRM fbdev emulation layer. >> >> This patch introduces a CONFIG_FB_CORE option that could be enabled just >> to have the core support needed for CONFIG_DRM_FBDEV_EMULATION, allowing >> CONFIG_FB to be disabled (and automatically disabling all fbdev drivers). >> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> >> --- > > This looks really nice! > > I tried to do something like this a few years ago, but failed as Yes, I also tried to do this before some time ago [0]: [0]: https://lore.kernel.org/lkml/20210827100027.1577561-1-javierm@xxxxxxxxxx/t/#mc8bb6cda8c2d795673618049b6c834b34bf86162 and at the time required some code refactoring but now thanks to all the cleanups that Thomas has been doing over, I could just do it with Kconfig. > I did too much at once by attempting to cut out msot of the fb core > support that is not actually used by DRM at the same time. > > Doing just the Kconfig bits as you do here is probably better > anyway, cutting out unneeded bits into separate modules or #ifdef > sections can come later. > Exactly, that was my thought too. Glad that you agree with the approach. >> @@ -59,7 +71,7 @@ config FIRMWARE_EDID >> >> config FB_DEVICE >> bool "Provide legacy /dev/fb* device" >> - depends on FB >> + depends on FB_CORE >> default y >> help >> Say Y here if you want the legacy /dev/fb* device file and > > I don't see this symbol in linux-next yet, what tree are you using > as a base? > It's now in the drm-misc/drm-misc-next branch [1]. It's not in -next yet because it just landed a few days ago [2]. [1]: https://cgit.freedesktop.org/drm/drm-misc/log/?h=drm-misc-next [2]: https://cgit.freedesktop.org/drm/drm-misc/commit/?id=701d2054fa3 In fact, that's the reason why I rebased my previous attempt [0]. >> @@ -69,13 +81,13 @@ config FB_DEVICE >> >> config FB_DDC >> tristate >> - depends on FB >> + depends on FB_CORE >> select I2C_ALGOBIT >> select I2C > > This seems to only be used by actual fbdev drivers, so maybe > don't change the dependency here. > Indeed. >> @@ -162,22 +174,22 @@ endchoice >> >> config FB_SYS_FOPS >> tristate >> - depends on FB >> + depends on FB_CORE > > Same for this one > Ok. >> config FB_HECUBA >> tristate >> - depends on FB >> + depends on FB_CORE >> depends on FB_DEFERRED_IO >> >> config FB_SVGALIB >> tristate >> - depends on FB >> + depends on FB_CORE >> help >> Common utility functions useful to fbdev drivers of VGA-based >> cards. >> config FB_MACMODES >> tristate >> - depends on FB >> + depends on FB_CORE >> > > These three seem to actually be part of fbdev drivers rather > than the core, and it may be best to move them into > drviers/video/fbdev/ as standalone modules. That would be a > separate patch of course. > Agreed. I'll then also don't change the dependency on these ones. >> config FB_BACKLIGHT >> tristate >> - depends on FB >> + depends on FB_CORE >> select BACKLIGHT_CLASS_DEVICE > > Separating this one from FB_CORE would help avoid circular dependencies, > this one keeps causing issues. > You mean separating from FB or should I keep the existing depends on FB? It seems this is only used by fbdev drivers so probably the latter? >> @@ -1,22 +1,22 @@ >> # SPDX-License-Identifier: GPL-2.0 >> obj-$(CONFIG_FB_NOTIFY) += fb_notify.o >> -obj-$(CONFIG_FB) += fb.o >> -fb-y := fb_backlight.o \ >> +obj-$(CONFIG_FB_CORE) += fb_core.o >> +fb_core-y := fb_backlight.o \ >> fb_info.o \ >> fbmem.o fbmon.o fbcmap.o \ >> modedb.o fbcvt.o fb_cmdline.o > > I would not bother renaming the module itself here, that > might cause problems if anything relies on loading the > module by name or a named module parameter. > I was actually not sure about this, but then thought that someone could had complained that the Kconfig symbol and module name wouldn't match :) I'll just keep the existing module name then and drop the rename. > Arnd > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
