Re: [PATCH 2/2] video: fix build with CONFIG_VIDEO && !CONFIG_VIDEO_LOGO

[Tom Rini <trini@konsulko.com>]

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

On Wed, Sep 02, 2026 at 10:48:38AM +0200, Julien Stephan wrote:
> Le mar. 1 sept. 2026 à 18:42, Tom Rini <trini@konsulko.com> a écrit :
> >
> > On Tue, Sep 01, 2026 at 10:52:43AM +0200, Julien Stephan wrote:
> > > Building with CONFIG_VIDEO enabled but CONFIG_VIDEO_LOGO disabled fails
> > > at link time:
> > >
> > >   video-uclass.o: in function `video_get_u_boot_logo':
> > >   video-uclass.c:593: undefined reference to `__splash_u_boot_logo_begin'
> > >
> > > The __splash_u_boot_logo_begin/_end symbols are provided by
> > > u_boot_logo.bmp.o, which is only built when CONFIG_VIDEO_LOGO is set:
> > >
> > >   obj-$(CONFIG_VIDEO_LOGO) += u_boot_logo.bmp.o
> > >
> > > video_get_u_boot_logo() and show_splash() reference those symbols
> > > unconditionally, so with the logo disabled the reference is left
> > > dangling. show_splash() alone would be dead-code eliminated (it is
> > > static and only reached under a CONFIG_IS_ENABLED(VIDEO_LOGO) guard),
> > > but video_get_u_boot_logo() is an exported function and is always
> > > emitted.
> > >
> > > Guard the splash helpers and their symbol references with
> > > CONFIG_IS_ENABLED(VIDEO_LOGO), and provide a static inline
> > > video_get_u_boot_logo() stub returning NULL for the disabled case in
> > > video.h. Callers already handle a NULL logo pointer (e.g.
> > > bootflow_menu.c), so no caller changes are needed.
> > >
> > > Reproduce with any board that enables VIDEO without VIDEO_LOGO or
> > > enabling SPLASH_SCREEN (it disables automatically VIDEO_LOGO).
> > >
> > > Fixes: 0d3890188d6b ("video: Add function to obtain the U-Boot logo")
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > > ---
> > >  drivers/video/video-uclass.c | 7 +++++--
> > >  include/video.h              | 9 ++++++++-
> > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> > > index de161054d52..4c959a57619 100644
> > > --- a/drivers/video/video-uclass.c
> > > +++ b/drivers/video/video-uclass.c
> > > @@ -579,6 +579,7 @@ int video_get_ysize(struct udevice *dev)
> > >       return priv->ysize;
> > >  }
> > >
> > > +#if CONFIG_IS_ENABLED(VIDEO_LOGO)
> > >  #define SPLASH_DECL(_name) \
> > >       extern u8 __splash_ ## _name ## _begin[]; \
> > >       extern u8 __splash_ ## _name ## _end[]
> > > @@ -598,6 +599,7 @@ static int show_splash(struct udevice *dev)
> > >
> > >       return video_bmp_display(dev, map_to_sysmem(data), -4, 4, true);
> > >  }
> > > +#endif
> > >
> > >  int video_default_font_height(struct udevice *dev)
> > >  {
> > > @@ -716,14 +718,15 @@ static int video_post_probe(struct udevice *dev)
> > >               return ret;
> > >       }
> > >
> > > -     if (CONFIG_IS_ENABLED(VIDEO_LOGO) &&
> > > -         !CONFIG_IS_ENABLED(SPLASH_SCREEN) && !plat->hide_logo) {
> > > +#if CONFIG_IS_ENABLED(VIDEO_LOGO)
> > > +     if (!CONFIG_IS_ENABLED(SPLASH_SCREEN) && !plat->hide_logo) {
> > >               ret = show_splash(dev);
> > >               if (ret) {
> > >                       log_debug("Cannot show splash screen\n");
> > >                       return ret;
> > >               }
> > >       }
> > > +#endif
> >
> > Is this hunk really needed? I can see getting here as part of debugging
> > the problem, but before the change it should evaluate to 'if (0 && ...)'
> > and be link-time eliminated.
> >
> 
> Hi Tom,
> 
> Yes it is needed since I moved show_splash() inside the #if
> CONFIG_IS_ENABLED(VIDEO_LOGO) guard above.
> I can go back to the runtime check, and keep show_splash() outside of
> the guard, but I'll have to use video_get_u_boot_logo() instead of
> relying on SPLASH_START(u_boot_logo);
> 
> What do you prefer?

Ah, I see now, that wasn't clear to me from the context. Looking at 1/2
and then 2/2 now, can we just remove show_splash() and call
video_bmp_display directly? That means not guarding SPLASH_START but
again it should optimize away. I do complain about how if
(CONFIG_IS_ENABLED(...)) isn't always great, but I think mixing that and
#if together just reads awkwardly too. So if we can't restructure the
code to keep if (CONFIG_IS_ENABLED(...)) then we should #if the whole
block there. Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]