On Mon, May 06, 2024 at 04:53:47PM +0200, Arnd Bergmann wrote:

On Mon, May 6, 2024, at 15:14, Daniel Vetter wrote:

quoted

On Fri, May 03, 2024 at 01:22:10PM -0700, Florian Fainelli wrote:

quoted

On 5/3/24 12:45, Arnd Bergmann wrote:

quoted

On Fri, May 3, 2024, at 21:28, Florian Fainelli wrote:

quoted

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@broadcom.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.

quoted

quoted

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.

quoted

quoted

quoted

$ 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