Message-ID: <ZjoMI5bJSlqhtOy1@phenom.ffwll.local>
Date: Tue, 7 May 2024 13:10:27 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Arnd Bergmann <arnd@...db.de>
Cc: Daniel Vetter <daniel@...ll.ch>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	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 06, 2024 at 04:53:47PM +0200, Arnd Bergmann wrote:
> 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.

So using /dev/fb chardev without fbcon is very much a real idea. This way
you should be able to run old userspace that uses fbdev directly for
drawing, but your console needs are served by a userspace console running
on top of drm.

vt switching gets a bit more entertaining, but I thought logind has all
the glue already to make that happen. Worst case you need a tiny launcher
tool to get your userspace console out of the way while you launch a fbdev
using application, but I think correctly implement the vt ioctls to switch
to graphics mode /should/ work automatically.

I do agree that this is only really a good idea with drm drivers, since
those do not rely on any of the fbdev infrastructure like the notifier
under discussion.

> >> > $ 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?

Uh ... I think I'm confused about the requirements. Does android's
recovery image need fbdev (meaning /dev/fb chardevs), or does it need
fbcon?

Note that fbcon runs (or well, should run) totally fine on top of drm
drivers without the fb notifier. This wasn't the case a few years ago
(because fbcon also used that notifier), but nowadays fb notifiers are
only needed for legacy fbdev drivers. So could it be that this "need fb
notifier" requirement is a leftover from rather old kernel versions and
not actually needed anymore?

I think we should nail the actual requirements here first before jumping
to solutions ...
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists