Re: [PATCH 3/5] video console: add support for fonts wider than 1 byte

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 26 Dec 2022 22:49:27 +0300
Dzmitry Sankouski <dsankouski@gmail.com> wrote:

Hi Dzmitry,

> Devices with high ppi may benefit from wider fonts.
> 
> Current width implementation is limited by 1 byte, i.e. 8 bits.
> New version iterates VIDEO_FONT_BYTE_WIDTH times, to process all
> width bytes, thus allowing fonts wider than 1 byte.

In general I think this is probably better than my version, not only
because it unifies rotated and normal consoles.
Some minor/stylistic comments below.

> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
>  drivers/video/console_simple.c | 79 ++++++++++++++++++----------------
>  include/video_font_4x6.h       |  2 +
>  include/video_font_data.h      |  2 +
>  3 files changed, 47 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/video/console_simple.c b/drivers/video/console_simple.c
> index 20087a30af..3ae1fbdc89 100644
> --- a/drivers/video/console_simple.c
> +++ b/drivers/video/console_simple.c
> @@ -61,30 +61,32 @@ static int fill_char_horizontally(uchar *pfont, void **line, struct video_priv *
>  {
>  	int ret;
>  	void *dst;
> -	u8 mask = 0x80;
> -
> +	u8 mask;
>  	ret = check_bpix_support(vid_priv->bpix);
>  	if (ret)
>  		return ret;
>  
> -	for (int col = 0; col < VIDEO_FONT_WIDTH; col++) {
> -		dst = *line;
> -		for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) {
> -			u32 value = (pfont[row * VIDEO_FONT_BYTE_WIDTH + col] & mask) ?
> -						vid_priv->colour_fg :
> -						vid_priv->colour_bg;
> -
> -			ret = fill_pixel_and_goto_next(&dst,
> -						       value,
> -						       VNBYTES(vid_priv->bpix),
> -						       direction
> -			);
> +	for (int col = 0; col < VIDEO_FONT_BYTE_WIDTH; col++) {
> +		mask = 0x80;
> +		for (int bit = 0; bit < 8; bit++) {

I think it's cleaner to collapse this:
	for (int col = 0; col < VIDEO_FONT_WIDTH; col++) {
		if ((col % 8) == 0)
			mask = 0x80;
...
> +			dst = *line;
> +			for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) {
> +				u32 value = (pfont[row * VIDEO_FONT_BYTE_WIDTH + col] & mask) ?

... and use (col / 8) here.

> +							vid_priv->colour_fg :
> +							vid_priv->colour_bg;
> +
> +				ret = fill_pixel_and_goto_next(&dst,
> +							       value,
> +							       VNBYTES(vid_priv->bpix),
> +							       direction
> +				);
> +			}
> +			if (direction)
> +				*line += vid_priv->line_length;
> +			else
> +				*line -= vid_priv->line_length;
> +			mask >>= 1;
>  		}
> -		if (direction)
> -			*line += vid_priv->line_length;
> -		else
> -			*line -= vid_priv->line_length;
> -		mask >>= 1;
>  	}
>  	return ret;
>  }
> @@ -101,24 +103,29 @@ static int fill_char_vertically(uchar *pfont, void **line, struct video_priv *vi
>  		return ret;
>  	for (int row = 0; row < VIDEO_FONT_HEIGHT; row++) {
>  		dst = *line;
> -		uchar bits = pfont[row];
> -
> -		for (int i = 0; i < VIDEO_FONT_WIDTH; i++) {
> -			u32 value = (bits & 0x80) ?
> -				vid_priv->colour_fg :
> -				vid_priv->colour_bg;
> -
> -			ret = fill_pixel_and_goto_next(&dst,
> -						       value,
> -						       VNBYTES(vid_priv->bpix),
> -						       direction
> -			);
> -			bits <<= 1;
> +		uchar bits;
> +
> +		for (int i = 0; i < VIDEO_FONT_BYTE_WIDTH; i++) {
> +			bits = pfont[i];
> +			for (int bit = 0; bit < 8; bit++) {

Same as above, those two loops should become one.

> +				u32 value = (bits & 0x80) ?
> +					vid_priv->colour_fg :
> +					vid_priv->colour_bg;
> +
> +				ret = fill_pixel_and_goto_next(&dst,
> +							       value,
> +							       VNBYTES(vid_priv->bpix),
> +							       direction
> +				);
> +				bits <<= 1;
> +			}
>  		}
>  		if (direction)
>  			*line -= vid_priv->line_length;
>  		else
>  			*line += vid_priv->line_length;
> +
> +		pfont += VIDEO_FONT_BYTE_WIDTH;
>  	}
>  	return ret;
>  }
> @@ -177,7 +184,7 @@ static int console_putc_xy(struct udevice *dev, uint x_frac, uint y, char ch)
>  	int pbytes = VNBYTES(vid_priv->bpix);
>  	int x, linenum, ret;
>  	void *start, *line;
> -	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
> +	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE;
>  
>  	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
>  		return -EAGAIN;
> @@ -286,7 +293,7 @@ static int console_putc_xy_1(struct udevice *dev, uint x_frac, uint y, char ch)
>  	int pbytes = VNBYTES(vid_priv->bpix);
>  	int x, linenum, ret;
>  	void *start, *line;
> -	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
> +	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE;
>  
>  	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
>  		return -EAGAIN;
> @@ -357,7 +364,7 @@ static int console_putc_xy_2(struct udevice *dev, uint x_frac, uint y, char ch)
>  	int pbytes = VNBYTES(vid_priv->bpix);
>  	int linenum, x, ret;
>  	void *start, *line;
> -	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
> +	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE;
>  
>  	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
>  		return -EAGAIN;
> @@ -432,7 +439,7 @@ static int console_putc_xy_3(struct udevice *dev, uint x_frac, uint y, char ch)
>  	int pbytes = VNBYTES(vid_priv->bpix);
>  	int linenum, x, ret;
>  	void *start, *line;
> -	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
> +	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_CHAR_PIXEL_DATA_SIZE;
>  
>  	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
>  		return -EAGAIN;
> diff --git a/include/video_font_4x6.h b/include/video_font_4x6.h
> index c7e6351b64..f1eb2af861 100644
> --- a/include/video_font_4x6.h
> +++ b/include/video_font_4x6.h
> @@ -43,7 +43,9 @@ __END__;
>  
>  #define VIDEO_FONT_CHARS	256
>  #define VIDEO_FONT_WIDTH	4
> +#define VIDEO_FONT_BYTE_WIDTH	VIDEO_FONT_WIDTH / 8 + (VIDEO_FONT_WIDTH % 8 > 0)

I think the canonical way for rounding up is:
	((VIDEO_FONT_WIDTH + 7) / 8)

And as Simon said: you need parentheses, otherwise it failed for me
with a width of 12 (sun12x22 font).

>  #define VIDEO_FONT_HEIGHT	6
> +#define VIDEO_FONT_CHAR_PIXEL_DATA_SIZE	VIDEO_FONT_HEIGHT * VIDEO_FONT_BYTE_WIDTH

Those two new lines do not contain any specific data, so can be moved to
console_simple.c.

>  #define VIDEO_FONT_SIZE		(VIDEO_FONT_CHARS * VIDEO_FONT_HEIGHT)
>  
>  static unsigned char video_fontdata[VIDEO_FONT_SIZE] = {
> diff --git a/include/video_font_data.h b/include/video_font_data.h
> index 6e64198d1a..6a575e6d15 100644
> --- a/include/video_font_data.h
> +++ b/include/video_font_data.h
> @@ -11,7 +11,9 @@
>  
>  #define VIDEO_FONT_CHARS	256
>  #define VIDEO_FONT_WIDTH	8
> +#define VIDEO_FONT_BYTE_WIDTH	VIDEO_FONT_WIDTH / 8 + (VIDEO_FONT_WIDTH % 8 > 0)
>  #define VIDEO_FONT_HEIGHT	16
> +#define VIDEO_FONT_CHAR_PIXEL_DATA_SIZE	VIDEO_FONT_HEIGHT * VIDEO_FONT_BYTE_WIDTH

same here, remove from here and use one copy in console_simple.c.

Cheers,
Andre


>  #define VIDEO_FONT_SIZE		(VIDEO_FONT_CHARS * VIDEO_FONT_HEIGHT)
>  
>  static unsigned char __maybe_unused video_fontdata[VIDEO_FONT_SIZE] = {