Re: [PATCH 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols

[Javier Martinez Canillas <javierm@redhat.com>]

"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