From: Daniel Vetter <daniel@ffwll.ch>
To: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
linux-kernel@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch>,
Helge Deller <deller@gmx.de>,
Thomas Zimmermann <tzimmermann@suse.de>,
Javier Martinez Canillas <javierm@redhat.com>,
Sam Ravnborg <sam@ravnborg.org>,
"open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
"open list:FRAMEBUFFER LAYER" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate
Date: Mon, 6 May 2024 15:14:28 +0200 [thread overview]
Message-ID: <ZjjXtEwWWZX43c6l@phenom.ffwll.local> (raw)
In-Reply-To: <05a5e893-12f7-49fd-9a9a-abd387571f9b@broadcom.com>
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
next prev parent reply other threads:[~2024-05-06 13:14 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-03 19:28 [PATCH] fbdev: Have CONFIG_FB_NOTIFY be tristate Florian Fainelli
2024-05-03 19:45 ` Arnd Bergmann
2024-05-03 20:22 ` Florian Fainelli
2024-05-06 13:14 ` Daniel Vetter [this message]
2024-05-06 14:53 ` Arnd Bergmann
2024-05-06 15:14 ` Arnd Bergmann
2024-05-07 11:10 ` Daniel Vetter
2024-05-07 11:44 ` Arnd Bergmann
2024-05-08 18:37 ` Florian Fainelli
2024-05-08 19:35 ` Arnd Bergmann
2024-05-08 20:36 ` Sam Ravnborg
2024-05-08 21:05 ` Arnd Bergmann
2024-05-04 11:46 ` kernel test robot
2024-05-04 14:42 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjjXtEwWWZX43c6l@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=arnd@arndb.de \
--cc=deller@gmx.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=florian.fainelli@broadcom.com \
--cc=javierm@redhat.com \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).