[PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Florian Fainelli]

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>
---
 drivers/video/fbdev/core/Kconfig     | 2 +-
 drivers/video/fbdev/core/fb_notify.c | 3 +++
 include/linux/fb.h                   | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/Kconfig b/drivers/video/fbdev/core/Kconfig
index db09fe87fcd4..036af8b5914a 100644
--- a/drivers/video/fbdev/core/Kconfig
+++ b/drivers/video/fbdev/core/Kconfig
@@ -8,7 +8,7 @@ config FB_CORE
 	tristate
 
 config FB_NOTIFY
-	bool
+	tristate
 
 config FIRMWARE_EDID
 	bool "Enable firmware EDID"
diff --git a/drivers/video/fbdev/core/fb_notify.c b/drivers/video/fbdev/core/fb_notify.c
index 10e3b9a74adc..ef707e092344 100644
--- a/drivers/video/fbdev/core/fb_notify.c
+++ b/drivers/video/fbdev/core/fb_notify.c
@@ -52,3 +52,6 @@ int fb_notifier_call_chain(unsigned long val, void *v)
 	return blocking_notifier_call_chain(&fb_notifier_list, val, v);
 }
 EXPORT_SYMBOL_GPL(fb_notifier_call_chain);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Frame buffer notifier support");
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 0dd27364d56f..8c7ae5997278 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -156,7 +156,7 @@ struct fb_blit_caps {
 	u32 flags;
 };
 
-#ifdef CONFIG_FB_NOTIFY
+#if IS_ENABLED(CONFIG_FB_NOTIFY)
 extern int fb_register_client(struct notifier_block *nb);
 extern int fb_unregister_client(struct notifier_block *nb);
 extern int fb_notifier_call_chain(unsigned long val, void *v);
-- 
2.34.1

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

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

 2. This breaks if any of the four callers of fb_register_client()
    are built-in while CONFIG_FB_NOTIFY is set to =m:

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

    Arnd

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

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

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.

IMHO it makes sense for all subsystem supporting code to be completely 
modular or completely built-in, or at least allowed to be.

> 
>   2. This breaks if any of the four callers of fb_register_client()
>      are built-in while CONFIG_FB_NOTIFY is set to =m:

Ah good point, I missed that part, thanks, adding "select FB_NOTIFY" 
ought to be enough for those.

> 
> $ 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 :)
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Daniel Vetter]

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@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.
> 
> 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 :-)

> IMHO it makes sense for all subsystem supporting code to be completely
> modular or completely built-in, or at least allowed to be.
> 
> > 
> >   2. This breaks if any of the four callers of fb_register_client()
> >      are built-in while CONFIG_FB_NOTIFY is set to =m:
> 
> Ah good point, I missed that part, thanks, adding "select FB_NOTIFY" ought
> to be enough for those.
> 
> > 
> > $ 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.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

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

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

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

On Mon, May 6, 2024, at 16:53, Arnd Bergmann wrote:
> On Mon, May 6, 2024, at 15:14, Daniel Vetter wrote:
>>
>> 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.
...
> 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?

Replying to myself here, I think I found the answer in
https://android.googlesource.com/platform/bootable/recovery/+/refs/heads/main/minui/

which has separate backends for fbdev and drm, and should
just work as long as the drm driver is loaded in the
recovery image.

Florian, do you know why this is not being used on the
machines you are looking at?

      Arnd

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Daniel Vetter]

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

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

On Tue, May 7, 2024, at 13:10, Daniel Vetter wrote:
> 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:
>> 
>> 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.

I'm pretty sure what Florian is looking for has no dependency
on VT, fbcon or logind, but I'm only guessing based on the
information I see in the public Android source trees.

My understanding is that the Android recovery application is a
graphical tool that accesses the framebuffer directly and
is controlled using the volume and power buttons on a phone.

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

Right, let's wait for Florian to reply. From what he said earlier
though, the only reason that the notifiers are getting in the
way is the link error you get from trying to load a separately
built fb.ko module on a kernel that was built with FB=n / FB_CORE=n,
so I don't think he even cares about notifiers, only about
allowing the recovery application to mmap() the framebuffer.

     Arnd

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 4441 bytes --]

On 5/7/24 04:44, Arnd Bergmann wrote:
> On Tue, May 7, 2024, at 13:10, Daniel Vetter wrote:
>> 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:
>>>
>>> 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.
> 
> I'm pretty sure what Florian is looking for has no dependency
> on VT, fbcon or logind, but I'm only guessing based on the
> information I see in the public Android source trees.
> 
> My understanding is that the Android recovery application is a
> graphical tool that accesses the framebuffer directly and
> is controlled using the volume and power buttons on a phone.
> 
>>> 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 ...
> 
> Right, let's wait for Florian to reply. From what he said earlier
> though, the only reason that the notifiers are getting in the
> way is the link error you get from trying to load a separately
> built fb.ko module on a kernel that was built with FB=n / FB_CORE=n,
> so I don't think he even cares about notifiers, only about
> allowing the recovery application to mmap() the framebuffer.

Right, we do not really care about notifiers AFAICT. Based upon this 
discussion there has been an action on our side to stop making use of 
the FB subsystem for recovery and use the full blow DRM driver instead.

While we get there, though I still see some value into this patch (or a 
v2, that is). I have a v2 ready if you think there is some value in 
pursuing that route, if not, we can stop there.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

On Wed, May 8, 2024, at 20:37, Florian Fainelli wrote:
> On 5/7/24 04:44, Arnd Bergmann wrote:
>> On Tue, May 7, 2024, at 13:10, Daniel Vetter wrote:
>>> On Mon, May 06, 2024 at 04:53:47PM +0200, Arnd Bergmann wrote:

>> Right, let's wait for Florian to reply. From what he said earlier
>> though, the only reason that the notifiers are getting in the
>> way is the link error you get from trying to load a separately
>> built fb.ko module on a kernel that was built with FB=n / FB_CORE=n,
>> so I don't think he even cares about notifiers, only about
>> allowing the recovery application to mmap() the framebuffer.
>
> Right, we do not really care about notifiers AFAICT. Based upon this 
> discussion there has been an action on our side to stop making use of 
> the FB subsystem for recovery and use the full blow DRM driver instead.

Ok, sounds good.

> While we get there, though I still see some value into this patch (or a 
> v2, that is). I have a v2 ready if you think there is some value in 
> pursuing that route, if not, we can stop there.

I think if you want to do a new version, that is likely to run
into new problems, given that this part of fbdev is particularly
fragile and partly wrong. On the other hand, it would be nice to
have a patch to limit the use of the notifiers to the smallest
set of kernel configs that actually need it, and leave it turned
off for everything else.

These are the ones I could find:

- CONFIG_GUMSTIX_AM200EPD (FB_EVENT_FB_REGISTERED)
- CONFIG_LCD_CORGI, CONFIG_LCD_TDO24M (FB_EVENT_MODE_CHANGE)
- CONFIG_LEDS_TRIGGER_BACKLIGHT (FB_EVENT_BLANK)
- CONFIG_FB_OLPC_DCON (FB_EVENT_BLANK/BL_CORE_FBBLANK)
- CONFIG_FB_SH_MOBILE_LCDC, CONFIG_BACKLIGHT_PCF50633,
  CONFIG_BACKLIGHT_PANDORA, CONFIG_BACKLIGHT_LP855X (BL_CORE_FBBLANK)
- CONFIG_FB_CLPS711X, CONFIG_FB_IMX, CONFIG_MACH_AMS_DELTA
  (lcd BL_CORE_FBBLANK)
- CONFIG_LCD_AMS369FG06, CONFIG_LCD_CORGI, CONFIG_LCD_HX8357,
  CONFIG_LCD_ILI922X, CONFIG_LCD_ILI9320, CONFIG_LCD_HP700,
  CONFIG_LCD_L4F00242T03, CONFIG_LCD_LMS283GF05, CONFIG_LCD_LMS501KF03
  CONFIG_LCD_LTV350QV, CONFIG_LCD_OTM3225A, CONFIG_LCD_PLATFORM,
  CONFIG_LCD_TDO24M (lcd BL_CORE_FBBLANK)

Almost all of these are exclusive to ancient ARMv5 boards or
similar, so if we make the notifiers depend on the whole list,
this would leave it disabled even for most configurations
that enable CONFIG_FB=y.

This could be done with a 'select', but I'd prefer the
'default y; depends on LCD_FOO || LCD_BAR || ...'
variant because that makes it easier to spot if someone
tries to add another one.

      Arnd

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Sam Ravnborg]

Hi Arnd,

> 
> I think if you want to do a new version, that is likely to run
> into new problems, given that this part of fbdev is particularly
> fragile and partly wrong. On the other hand, it would be nice to
> have a patch to limit the use of the notifiers to the smallest
> set of kernel configs that actually need it, and leave it turned
> off for everything else.
> 
> These are the ones I could find:
> 
> - CONFIG_GUMSTIX_AM200EPD (FB_EVENT_FB_REGISTERED)

I was surprised to see this driver is still around as many other old
drivers was nuked as part of the pxa cleanup.
It is the only user of FB_EVENT_FB_REGISTERED - so a potential cleanup
if the driver is no longer relevant.

Just a drive-by comment, this should not stop a v2 of the patchset.

	Sam

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[Arnd Bergmann]

On Wed, May 8, 2024, at 22:36, Sam Ravnborg wrote:
>> 
>> I think if you want to do a new version, that is likely to run
>> into new problems, given that this part of fbdev is particularly
>> fragile and partly wrong. On the other hand, it would be nice to
>> have a patch to limit the use of the notifiers to the smallest
>> set of kernel configs that actually need it, and leave it turned
>> off for everything else.
>> 
>> These are the ones I could find:
>> 
>> - CONFIG_GUMSTIX_AM200EPD (FB_EVENT_FB_REGISTERED)
>
> I was surprised to see this driver is still around as many other old
> drivers was nuked as part of the pxa cleanup.
> It is the only user of FB_EVENT_FB_REGISTERED - so a potential cleanup
> if the driver is no longer relevant.

We kept gumstix at the time to give a chance to do a DT conversion
using the qemu model, but so far nobody has worked on this. My
feeling is that we'll end up removing it at some point in the
future along with the other legacy PXA board files. 

Even if someone wants to keep working on gumstix DT support for
qemu, I agree that we can probably remove AM200EPD, AM300EPD
and metronomefb, since those are not even modeled by qemu.

     Arnd

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[kernel test robot]

Hi Florian,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.9-rc6 next-20240503]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Fainelli/fbdev-Have-CONFIG_FB_NOTIFY-be-tristate/20240504-033139
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240503192858.103640-1-florian.fainelli%40broadcom.com
patch subject: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate
config: powerpc-randconfig-001-20240504 (https://download.01.org/0day-ci/archive/20240504/202405041939.MNsieCv5-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240504/202405041939.MNsieCv5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405041939.MNsieCv5-lkp@intel.com/

All errors (new ones prefixed by >>):

   powerpc-linux-ld: drivers/leds/trigger/ledtrig-backlight.o: in function `bl_trig_deactivate':
>> ledtrig-backlight.c:(.text+0x360): undefined reference to `fb_unregister_client'
   powerpc-linux-ld: drivers/leds/trigger/ledtrig-backlight.o: in function `bl_trig_activate':
>> ledtrig-backlight.c:(.text+0x3e8): undefined reference to `fb_register_client'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate

[kernel test robot]

Hi Florian,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.9-rc6 next-20240503]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Fainelli/fbdev-Have-CONFIG_FB_NOTIFY-be-tristate/20240504-033139
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240503192858.103640-1-florian.fainelli%40broadcom.com
patch subject: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate
config: i386-randconfig-015-20240504 (https://download.01.org/0day-ci/archive/20240504/202405042242.iXLDu4Xj-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240504/202405042242.iXLDu4Xj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405042242.iXLDu4Xj-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/leds/trigger/ledtrig-backlight.o: in function `bl_trig_deactivate':
>> drivers/leds/trigger/ledtrig-backlight.c:128:(.text+0x136): undefined reference to `fb_unregister_client'
   ld: drivers/leds/trigger/ledtrig-backlight.o: in function `bl_trig_activate':
>> drivers/leds/trigger/ledtrig-backlight.c:117:(.text+0x1aa): undefined reference to `fb_register_client'


vim +128 drivers/leds/trigger/ledtrig-backlight.c

e4786ba0db7b11 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  100  
2282e125a406e0 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  101  static int bl_trig_activate(struct led_classdev *led)
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  102  {
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  103  	int ret;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  104  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  105  	struct bl_trig_notifier *n;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  106  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  107  	n = kzalloc(sizeof(struct bl_trig_notifier), GFP_KERNEL);
e4786ba0db7b11 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  108  	if (!n)
e4786ba0db7b11 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  109  		return -ENOMEM;
e4786ba0db7b11 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  110  	led_set_trigger_data(led, n);
9f9455ae710786 drivers/leds/ledtrig-backlight.c         Janusz Krzysztofik 2011-01-12  111  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  112  	n->led = led;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  113  	n->brightness = led->brightness;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  114  	n->old_status = UNBLANK;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  115  	n->notifier.notifier_call = fb_notifier_callback;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  116  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13 @117  	ret = fb_register_client(&n->notifier);
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  118  	if (ret)
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  119  		dev_err(led->dev, "unable to register backlight trigger\n");
2282e125a406e0 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  120  
2282e125a406e0 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  121  	return 0;
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  122  }
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  123  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  124  static void bl_trig_deactivate(struct led_classdev *led)
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  125  {
e4786ba0db7b11 drivers/leds/trigger/ledtrig-backlight.c Uwe Kleine-König   2018-07-02  126  	struct bl_trig_notifier *n = led_get_trigger_data(led);
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  127  
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13 @128  	fb_unregister_client(&n->notifier);
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  129  	kfree(n);
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  130  }
132e9306beedd0 drivers/leds/ledtrig-backlight.c         Rodolfo Giometti   2008-10-13  131  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki