Re: [PATCH v3 1/7] fbdev: describe where the boot logo goes in one place

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 23.09.26 um 22:10 schrieb Max Pedraza:
> fb_prepare_logo() works out how many rows to keep clear for the logo and
> fb_show_logo_line() works out where to draw it, and both of them open code
> the same decision: centred if fb_center_logo, top left otherwise. The two
> calculations have to agree, because fbcon erases whatever falls outside the
> rows that were reserved, and nothing makes them.
>
> Give the position a small structure of its own, with -1 on an axis meaning
> centre on that axis, and have both callers ask for it. fb_center_logo
> becomes {-1, -1} rather than a second path, and the arithmetic lives in one
> function that both use, so they cannot end up disagreeing.
>
> The structure and logo_place_axis() go in linux_logo.h rather than in the
> frame buffer code, since where a logo goes is a property of the logo, not
> of the one thing that draws it today.

No, the position is not a property of the logo. The code in linux_logo.h 
is only about the image itself.

In the case of fbdev, the policy for drawin the the image should be part 
of fbcon.

Best regards
Thomas

>
> logo_place_axis() also clamps, which the open coded version did not: the
> result is now always a position at which the logo lies entirely on screen.
> That is not reachable today, since the only positions are the two the
> console offers, but it stops being something the next caller has to
> remember.
>
> No functional change intended.
>
> Signed-off-by: Max Pedraza <maximpedraza@gmail.com>
> ---
>   drivers/video/fbdev/core/fb_logo.c | 58 +++++++++++++++++++-----------
>   include/linux/linux_logo.h         | 29 +++++++++++++++
>   2 files changed, 67 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_logo.c b/drivers/video/fbdev/core/fb_logo.c
> index 0bab8352b6..5ec9f9554f 100644
> --- a/drivers/video/fbdev/core/fb_logo.c
> +++ b/drivers/video/fbdev/core/fb_logo.c
> @@ -8,6 +8,17 @@
>   bool fb_center_logo __read_mostly;
>   int fb_logo_count __read_mostly = -1;
>   
> +/*
> + * The placement in effect, as asked for on the console command line.
> + */
> +static const struct logo_placement *fb_logo_placement(void)
> +{
> +	static const struct logo_placement centred = { .x = -1, .y = -1 };
> +	static const struct logo_placement top_left = { };
> +
> +	return fb_center_logo ? &centred : &top_left;
> +}
> +
>   static inline unsigned int safe_shift(unsigned int d, int n)
>   {
>   	return n < 0 ? d >> -n : d << n;
> @@ -281,7 +292,11 @@ static int fb_show_logo_line(struct fb_info *info, int rotate,
>   {
>   	u32 *palette = NULL, *saved_pseudo_palette = NULL;
>   	unsigned char *logo_new = NULL, *logo_rotate = NULL;
> +	const struct logo_placement *p;
> +	unsigned int xres = info->var.xres;
> +	unsigned int yres = info->var.yres;
>   	struct fb_image image;
> +	unsigned int block;
>   
>   	/* Return if the frame buffer is not mapped or suspended */
>   	if (logo == NULL || info->state != FBINFO_STATE_RUNNING ||
> @@ -322,26 +337,22 @@ static int fb_show_logo_line(struct fb_info *info, int rotate,
>   		fb_set_logo(info, logo, logo_new, fb_logo.depth);
>   	}
>   
> -	if (fb_center_logo) {
> -		int xres = info->var.xres;
> -		int yres = info->var.yres;
> +	image.width = logo->width;
> +	image.height = logo->height;
>   
> -		if (rotate == FB_ROTATE_CW || rotate == FB_ROTATE_CCW) {
> -			xres = info->var.yres;
> -			yres = info->var.xres;
> -		}
> +	if (rotate == FB_ROTATE_CW || rotate == FB_ROTATE_CCW)
> +		swap(xres, yres);
>   
> -		while (n && (n * (logo->width + 8) - 8 > xres))
> -			--n;
> -		image.dx = (xres - (n * (logo->width + 8) - 8)) / 2;
> -		image.dy = y ?: (yres - logo->height) / 2;
> -	} else {
> -		image.dx = 0;
> -		image.dy = y;
> -	}
> +	while (n && (n * (logo->width + 8) - 8 > xres))
> +		--n;
>   
> -	image.width = logo->width;
> -	image.height = logo->height;
> +	/* The copies are drawn in a row, so they are centred as one block */
> +	block = n ? n * (logo->width + 8) - 8 : logo->width;
> +
> +	p = fb_logo_placement();
> +	image.dx = logo_place_axis(p->x, xres, block);
> +	/* A stacked logo goes where the caller put it */
> +	image.dy = y ? y : logo_place_axis(p->y, yres, image.height);
>   
>   	if (rotate) {
>   		logo_rotate = kmalloc_array(logo->width, logo->height,
> @@ -418,6 +429,7 @@ static int fb_show_extra_logos(struct fb_info *info, int y, int rotate)
>   int fb_prepare_logo(struct fb_info *info, int rotate)
>   {
>   	int depth = fb_get_color_depth(&info->var, &info->fix);
> +	const struct logo_placement *p;
>   	unsigned int yres;
>   	int height;
>   
> @@ -480,9 +492,15 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
>   		}
>   	}
>   
> -	height = fb_logo.logo->height;
> -	if (fb_center_logo)
> -		height += (yres - fb_logo.logo->height) / 2;
> +	/*
> +	 * fbcon only leaves the first @height rows of the screen alone, so a
> +	 * logo placed further down would be drawn and then immediately
> +	 * cleared. Ask the same placement fb_show_logo_line() will use, so
> +	 * that the two cannot disagree.
> +	 */
> +	p = fb_logo_placement();
> +	height = logo_place_axis(p->y, yres, fb_logo.logo->height) +
> +		 fb_logo.logo->height;
>   #ifdef CONFIG_FB_LOGO_EXTRA
>   	height = fb_prepare_extra_logos(info, height, yres);
>   #endif
> diff --git a/include/linux/linux_logo.h b/include/linux/linux_logo.h
> index 1e727a2cb4..b3b15d3800 100644
> --- a/include/linux/linux_logo.h
> +++ b/include/linux/linux_logo.h
> @@ -13,6 +13,8 @@
>    */
>   
>   #include <linux/init.h>
> +#include <linux/minmax.h>
> +#include <linux/types.h>
>   
>   
>   #define LINUX_LOGO_MONO		1	/* monochrome black/white */
> @@ -36,6 +38,33 @@ extern const struct linux_logo logo_linux_clut224;
>   extern const struct linux_logo logo_spe_clut224;
>   
>   extern const struct linux_logo *fb_find_logo(int depth);
> +
> +/*
> + * Where a boot logo goes. A coordinate of -1 centres the logo on that axis.
> + * Whatever draws the logo describes its placement this way and computes it
> + * with logo_place_axis(), so that no two places can end up disagreeing about
> + * where the logo is.
> + */
> +struct logo_placement {
> +	s32 x, y;
> +};
> +
> +/*
> + * Place a logo along one axis. @pos is the coordinate asked for, or -1 to
> + * centre on that axis. The result is clamped so that the logo always lies
> + * entirely within the screen: the drawing code does not clip, so asking for
> + * more than that would otherwise scribble past the end of the frame buffer.
> + */
> +static inline int logo_place_axis(s32 pos, unsigned int span, unsigned int size)
> +{
> +	int last = (int)span - (int)size;
> +
> +	if (size > span)
> +		return 0;
> +
> +	return pos == -1 ? last / 2 : clamp(pos, 0, last);
> +}
> +
>   #ifdef CONFIG_FB_LOGO_EXTRA
>   extern void fb_append_extra_logo(const struct linux_logo *logo,
>   				 unsigned int n);

-- 
--
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)