Message-Id: <47c63c4c-c657-4210-b476-c91c4f192483@app.fastmail.com>
Date: Mon, 06 May 2024 16:53:47 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Daniel Vetter" <daniel@...ll.ch>,
 "Florian Fainelli" <florian.fainelli@...adcom.com>
Cc: linux-kernel@...r.kernel.org, "Helge Deller" <deller@....de>,
 "Thomas Zimmermann" <tzimmermann@...e.de>,
 "Javier Martinez Canillas" <javierm@...hat.com>,
 "Sam Ravnborg" <sam@...nborg.org>,
 "open list:FRAMEBUFFER LAYER" <linux-fbdev@...r.kernel.org>,
 "open list:FRAMEBUFFER LAYER" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

On Mon, May 6, 2024, at 15:14, Daniel Vetter wrote:
> On Fri, May 03, 2024 at 01:22:10PM -0700, Florian Fainelli wrote:
>> On 5/3/24 12:45, Arnd Bergmann wrote:
>> > On Fri, May 3, 2024, at 21:28, Florian Fainelli wrote:
>> > > Android devices in recovery mode make use of a framebuffer device to
>> > > provide an user interface. In a GKI configuration that has CONFIG_FB=m,
>> > > but CONFIG_FB_NOTIFY=y, loading the fb.ko module will fail with:
>> > > 
>> > > fb: Unknown symbol fb_notifier_call_chain (err -2)
>> > > 
>> > > Have CONFIG_FB_NOTIFY be tristate, just like CONFIG_FB such that both
>> > > can be loaded as module with fb_notify.ko first, and fb.ko second.
>> > > 
>> > > Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
>> > 
>> > I see two problems here, so I don't think this is the right
>> > approach:
>> > 
>> >   1. I don't understand your description: Since fb_notifier_call_chain()
>> >      is an exported symbol, it should work for both FB_NOTIFY=y and
>> >      FB_NOTIFY=m, unless something in Android drops the exported
>> >      symbols for some reason.
>> 
>> The symbol is still exported in the Android tree. The issue is that the GKI
>> defconfig does not enable any CONFIG_FB options at all. This is left for SoC
>> vendors to do in their own "fragment" which would add CONFIG_FB=m. That
>> implies CONFIG_FB_NOTIFY=y which was not part of the original kernel image
>> we build/run against, and so we cannot resolve the symbol.

I see.

>> This could be resolved by having the GKI kernel have CONFIG_FB_NOTIFY=y but
>> this is a bit wasteful (not by much since the code is very slim anyway) and
>> it does require making changes specifically to the Android tree which could
>> be beneficial upstream, hence my attempt at going upstream first.
>
> Making fbdev (the driver subsystem, not the uapi, we'll still happily
> merge patches for that) more useful is really not the upstream direction :-)

I'm more worried about the idea of enabling an entire subsystem
as a loadable module and expecting that to work with an existing
kernel, specifically when the drm.ko and fb.ko interact with
one another and are built with different .config files.

This is the current Android GKI config:
https://android.googlesource.com/kernel/common.git/+/refs/heads/android-mainline/arch/arm64/configs/gki_defconfig
where I can see that CONFIG_DRM is built-in, but DRM_FBDEV_EMULATION
CONFIG_VT, CONFIG_FRAMEBUFFER_CONSOLE, CONFIG_FB_DEVICE and
CONFIG_FB_CORE are all disabled.

So the console won't work at all,I think this means that there
is no way to get the console working, but building a fb.ko module
allows using /dev/fb with simplefb.ko (or any other one) happens
to almost work, but only by dumb luck rather than by design.

>> > $ git grep -w fb_register_client
>> > arch/arm/mach-pxa/am200epd.c:   fb_register_client(&am200_fb_notif);
>> > drivers/leds/trigger/ledtrig-backlight.c:       ret = fb_register_client(&n->notifier);
>> > drivers/video/backlight/backlight.c:    return fb_register_client(&bd->fb_notif);
>> > drivers/video/backlight/lcd.c:  return fb_register_client(&ld->fb_notif);
>> > 
>> > Somewhat related but not directly addressing your patch, I wonder
>> > if Android itself could migrate to using FB_CORE=m FB=n FB_NOTIFY=n
>> > instead and use simpledrm for the console in place of the legacy
>> > fbdev layer.
>> 
>> That is beyond my reach :)
>
> This one is. And it doesn't need to be simpledrm, just a drm kms driver
> with fbdev emulation. Heck even if you have an fbdev driver you should
> control the corresponding backlight explicitly, and not rely on the fb
> notifier to magical enable/disable some random backlights somewhere.
>
> So please do not encourage using this in any modern code.

I suppose making CONFIG_FB_NOTIFIER optional for FB (on by
default if any of the consumers of the notification are turned
on) would not be a bad direction to go in general and also
address Florian's link error, but that doesn't solve the
more general concern about a third-party fb.ko module on a
kernel that was explicitly built with FB disabled.

The GKI defconfig was initially done at a time where one could
not have CONFIG_FBDEV_EMULATION and CONFIG_FB_DEVICE without
pulling in the entire fbdev module, but now that is possible.
Maybe that is something that Android could now include?

Alternatively, I wonder if that recovery image could be changed
to access the screen through the /dev/dri/ interfaces? I have
no experience in using those without Xorg or Wayland, is that
a sensible thing to do?

      Arnd

Powered by blists - more mailing lists