Re: [PATCH v5 10/13] drm/client: splash: take the background colour from the image source

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 13.10.26 um 11:32 schrieb Màxim Pedraza Padilla:
> A logo designed for a light background looks wrong framed in black, and
> products that share a board support package differ in their colours as
> much as in their logos. The background colour belongs with the image,
> so take it from the same place:
>
> - a device tree image uses the optional "background-color" of its node;
> - the BGRT carries no colour, and firmware splashes sit on black, so it
>    uses the kernel's default;

> - the BMP loaded as firmware is named on the command line, and so is its
>    colour, through drm_client_lib.splash_color, which also covers the
>    screen when there is no image at all.

Please see my earlier comment in module parameters.

>
> CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR remains the default wherever
> the source says nothing. The command line no longer overrides a device
> tree image: a bootloader that wants another colour for a given boot,
> say to flag a failed test on a production line, changes the node.
>
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Màxim Pedraza Padilla <maximpedraza@gmail.com>
> ---
>   drivers/gpu/drm/clients/Kconfig      |  7 ++++--
>   drivers/gpu/drm/clients/drm_splash.c | 36 +++++++++++++++++++++-------
>   2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/clients/Kconfig
> index aef3781cc110..f471019bab87 100644
> --- a/drivers/gpu/drm/clients/Kconfig
> +++ b/drivers/gpu/drm/clients/Kconfig
> @@ -105,8 +105,11 @@ config DRM_CLIENT_SPLASH_BACKGROUND_COLOR
>   	help
>   	  The default splash background color, in RGB888 format.
>   
> -	  The color can be overridden through the drm_client_lib.splash_color
> -	  kernel command line parameter.
> +	  The background color comes from the same place as the image. A
> +	  device tree image can carry its own, and the drm_client_lib.splash_color
> +	  kernel command line parameter sets it for a BMP loaded as firmware,
> +	  or for the whole screen when there is no image. This color is used
> +	  otherwise, and always with the BGRT image, which carries none.
>   
>   config DRM_CLIENT_SPLASH_SRC_DT
>   	bool "Device tree as splash source"
> diff --git a/drivers/gpu/drm/clients/drm_splash.c b/drivers/gpu/drm/clients/drm_splash.c
> index fdf398987007..463aff27c747 100644
> --- a/drivers/gpu/drm/clients/drm_splash.c
> +++ b/drivers/gpu/drm/clients/drm_splash.c
> @@ -42,7 +42,8 @@
>   
>   static unsigned int splash_color = CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR;
>   module_param(splash_color, uint, 0400);
> -MODULE_PARM_DESC(splash_color, "Splash background color (RGB888)");
> +MODULE_PARM_DESC(splash_color,
> +		 "Background color (RGB888) around the BMP loaded as firmware, or of the whole screen with no image");
>   
>   #if IS_ENABLED(CONFIG_DRM_CLIENT_SPLASH_SRC_BMP)
>   #define DEFAULT_SPLASH_BMP "drm_splash.bmp"
> @@ -133,6 +134,8 @@ struct drm_splash {
>   	s32 off_x, off_y;
>   	/* One of DRM_MODE_ROTATE_*: turns the image, not the screen */
>   	unsigned int rotation;
> +	/* RGB888, from the same place as the image */
> +	u32 bg_color;
>   };
>   
>   static struct drm_splash *client_to_drm_splash(struct drm_client_dev *client)
> @@ -271,12 +274,18 @@ static struct device_node *drm_splash_dt_node(struct drm_splash *splash)
>   	return NULL;
>   }
>   
> -/* The optional placement properties of the node, read once */
> -static void drm_splash_dt_placement(struct drm_splash *splash,
> -				    struct device_node *np)
> +/* The optional properties of the node, read once */
> +static void drm_splash_dt_props(struct drm_splash *splash,
> +				struct device_node *np)
>   {
>   	u32 val[2];
>   
> +	/* The colour travels with the image, or it is the kernel's default */
> +	if (!of_property_read_u32(np, "background-color", &val[0]))
> +		splash->bg_color = val[0] & 0xffffff;
> +	else
> +		splash->bg_color = CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR;
> +
>   	if (!of_property_read_u32_array(np, "logo-position", val, 2)) {
>   		splash->pos_x = (s32)val[0];
>   		splash->pos_y = (s32)val[1];
> @@ -365,8 +374,8 @@ static struct device_node *drm_splash_dt_node(struct drm_splash *splash)
>   	return NULL;
>   }
>   
> -static void drm_splash_dt_placement(struct drm_splash *splash,
> -				    struct device_node *np)
> +static void drm_splash_dt_props(struct drm_splash *splash,
> +				struct device_node *np)
>   {
>   }
>   
> @@ -780,13 +789,22 @@ static enum drm_splash_source drm_splash_pick_source(struct drm_splash *splash)
>   
>   	np = drm_splash_dt_node(splash);
>   	if (np) {
> -		drm_splash_dt_placement(splash, np);
> +		drm_splash_dt_props(splash, np);
>   		of_node_put(np);
>   		return DRM_SPLASH_SRC_DT;
>   	}
>   
> -	if (drm_splash_bgrt_available(splash))
> +	/* The BGRT carries no colour: firmware splashes sit on black */
> +	if (drm_splash_bgrt_available(splash)) {
> +		splash->bg_color = CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR;
>   		return DRM_SPLASH_SRC_BGRT;
> +	}
> +
> +	/*
> +	 * The BMP loaded as firmware is named on the command line, and so is
> +	 * its colour, which also covers the screen when there is no image.
> +	 */
> +	splash->bg_color = splash_color & 0xffffff;
>   	if (IS_ENABLED(CONFIG_DRM_CLIENT_SPLASH_SRC_BMP))
>   		return DRM_SPLASH_SRC_BMP;
>   
> @@ -844,7 +862,7 @@ static int drm_splash_render_thread(void *data)
>   
>   			if (!scanout->bg_drawn) {
>   				drm_dbg(client->dev, "draw background for scanout %d", i);
> -				u32 color = drm_draw_color_from_xrgb8888(splash_color,
> +				u32 color = drm_draw_color_from_xrgb8888(splash->bg_color,
>   									 scanout->format);
>   				drm_splash_fill_solid_color(scanout->buffer, color);
>   				scanout->bg_drawn = true;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)