Add Protectli logo within Make buildsys, don't patch post-build #771
Conversation
Makefile
Outdated
| define protectli_branding_hook | ||
|
|
||
| # Gate on vendor; use filter to avoid whitespace pitfalls | ||
| ifneq ($(filter y,$(CONFIG_VENDOR_PROTECTLI)),) |
There was a problem hiding this comment.
can we make it generic to all platforms by introducing a Kconfig option instead of checking the vendor?
|
Doesn't this effectively undo 831ace5 by ignoring what's in the defconfig/.config? |
filipleple
force-pushed
the
protectli-logo-buildsys
branch
from
909453c to
26230ab
Compare
filipleple
force-pushed
the
protectli-logo-buildsys
branch
2 times, most recently
from
319ecff to
2fe40b5
Compare
|
@SergiiDmytruk Not quite, I think what's key is that there's a black "empty" default image built into the payload, and then there's the customizable BOOTSPLASH region that's set to the Protectli logo. The customer wanted to be able to remove the logo to display the fallback black screen instead. Since we always want the logo to be in there in the binaries we deliver to the customer, I think it's more failsafe when it's inserted within the build than when we have to remember to set an extra flag in |
Upstream-Status: Inappropriate [Dasharo downstream] Signed-off-by: Filip Lewiński <[email protected]>
Add the BOOTSPLASH region logo.bmp file from within the buildsystem, instead of having to patch it afterwards in build.sh or elsewhere. Upstream-Status: Inappropriate [Dasharo downstream] Signed-off-by: Filip Lewiński <[email protected]>
The Protectli logo is now added within the buildsystem, it doesn't need to be patched externally in build.sh. Upstream-Status: Inappropriate [Dasharo downstream] Signed-off-by: Filip Lewiński <[email protected]>
filipleple
force-pushed
the
protectli-logo-buildsys
branch
2 times, most recently
from
29ea400 to
abaea78
Compare
There was a problem hiding this comment.
Not quite, I think what's key is that there's a black "empty" default image built into the payload, and then there's the customizable BOOTSPLASH region that's set to the Protectli logo. The customer wanted to be able to remove the logo to display the fallback black screen instead.
Got it, thanks.
| -n logo.bmp \ | ||
| -t raw \ | ||
| -c lzma | ||
| @: |
There was a problem hiding this comment.
Looks unnecessary.
| @: |
| # Only run if a file is provided | ||
| ifneq ($(CONFIG_BOOTSPLASH_REGION_LOGO_FILE),"") | ||
|
|
||
| real-target: branding_bootsplash_region |
There was a problem hiding this comment.
| real-target: branding_bootsplash_region | |
| files_added:: branding_bootsplash_region |
Otherwise BRANDING ... can be printed after the final Built ... line. This placement is also not ideal:
CBFSPRINT coreboot.rom
BRANDING Adding bootsplash (BOOTSPLASH/logo.bmp) from 3rdparty/dasharo-blobs/protectli/bootsplash.bmp
FMAP REGION: COREBOOT
Name Offset Type Size Comp
but it's better and I think files_added is how such modifications must be done.
| endif | ||
| endef | ||
|
|
||
| postinclude-hooks += $(bootsplash_region_hook) |
There was a problem hiding this comment.
| postinclude-hooks += $(bootsplash_region_hook) | |
| postinclude-hooks += $(bootsplash_region_hook) | |
| endif | ||
|
|
||
| # Add bootsplash to BOOTSPLASH/logo.bmp | ||
| define bootsplash_region_hook |
There was a problem hiding this comment.
Why make it a hook? .config is sourced in line 184 of this file.
Dasharo/dasharo-issues#1512
ref: prot-1894