Messages in this thread | | | From | Javier Martinez Canillas <> | Subject | Re: [PATCH 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols | Date | Fri, 30 Jun 2023 12:51:07 +0200 |
| |
"Arnd Bergmann" <arnd@arndb.de> 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@redhat.com> >> --- > > 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@redhat.com/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
| |