| Message ID | 20240503192858.103640-1-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbdev: Have CONFIG_FB_NOTIFY be tristate | expand |
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
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 :)
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'
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
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
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
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
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
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
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.
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
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
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
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);
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(-)