diff mbox series

[2/5] video console: refactoring and optimization

Message ID 20221226194929.166369-3-dsankouski@gmail.com
State Changes Requested
Delegated to: Anatolij Gustschin
Headers show
Series vidconsole: refactoring and support for wider fonts | expand

Commit Message

Dzmitry Sankouski Dec. 26, 2022, 7:49 p.m. UTC 
- get rid of code duplications in switch across bpp values
- extract common pixel fill logic in two functions one per
horizontal and vertical filling
- rearrange statements in put_xy* methods in unified way
- replace types - uint*_t to u*

Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
 drivers/video/console_simple.c | 508 ++++++++++++---------------------
 1 file changed, 184 insertions(+), 324 deletions(-)

Comments

Simon Glass Dec. 29, 2022, 10:39 p.m. UTC | #1 
Hi Dzmitry,

On Mon, 26 Dec 2022 at 13:50, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
>
> - get rid of code duplications in switch across bpp values
> - extract common pixel fill logic in two functions one per
> horizontal and vertical filling
> - rearrange statements in put_xy* methods in unified way
> - replace types - uint*_t to u*
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
>  drivers/video/console_simple.c | 508 ++++++++++++---------------------
>  1 file changed, 184 insertions(+), 324 deletions(-)

This looks like a nice tidy up.

Is there a code-size or performance penalty with this? E.g. with
CONFIG_CONSOLE_ROTATION disabled

Please can you comment the functions property so we know what they do?

Regards,
Simon
Dzmitry Sankouski Jan. 4, 2023, 11:17 a.m. UTC | #2 
Regarding code-size there's a gain with CONFIG_CONSOLE_ROTATION
enabled, and penalty with disabled:

New:
CONFIG_VIDEO_CONSOLE=y
CONFIG_CONSOLE_ROTATION=y
dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
108208  drivers/video/console_simple.o

CONFIG_VIDEO_CONSOLE=y
# CONFIG_CONSOLE_ROTATION is not set
dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
53848   drivers/video/console_simple.o

Old:
dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_normal.o
44728   drivers/video/console_normal.o
dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_rotate.o
85424   drivers/video/console_rotate.o

In theory, there should be a small performance penalty for the `if
(direction)` statement -
for every row, and for each pixel. For an 8x16 font, it'll be 144 if statements.

I'll comment on functions in the next patch versions.

пт, 30 дек. 2022 г. в 01:41, Simon Glass <sjg@chromium.org>:
>
> Hi Dzmitry,
>
> On Mon, 26 Dec 2022 at 13:50, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
> >
> > - get rid of code duplications in switch across bpp values
> > - extract common pixel fill logic in two functions one per
> > horizontal and vertical filling
> > - rearrange statements in put_xy* methods in unified way
> > - replace types - uint*_t to u*
> >
> > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> > ---
> >  drivers/video/console_simple.c | 508 ++++++++++++---------------------
> >  1 file changed, 184 insertions(+), 324 deletions(-)
>
> This looks like a nice tidy up.
>
> Is there a code-size or performance penalty with this? E.g. with
> CONFIG_CONSOLE_ROTATION disabled
>
> Please can you comment the functions property so we know what they do?
>
> Regards,
> Simon
Simon Glass Jan. 4, 2023, 8:01 p.m. UTC | #3 
Hi Dzmitry,

On Wed, 4 Jan 2023 at 04:17, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
>
> Regarding code-size there's a gain with CONFIG_CONSOLE_ROTATION
> enabled, and penalty with disabled:
>
> New:
> CONFIG_VIDEO_CONSOLE=y
> CONFIG_CONSOLE_ROTATION=y
> dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
> 108208  drivers/video/console_simple.o
>
> CONFIG_VIDEO_CONSOLE=y
> # CONFIG_CONSOLE_ROTATION is not set
> dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
> 53848   drivers/video/console_simple.o
>
> Old:
> dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_normal.o
> 44728   drivers/video/console_normal.o
> dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_rotate.o
> 85424   drivers/video/console_rotate.o
>
> In theory, there should be a small performance penalty for the `if
> (direction)` statement -
> for every row, and for each pixel. For an 8x16 font, it'll be 144 if statements.
>
> I'll comment on functions in the next patch versions.

To check this, use:

buildman -b <branch> <board>

to build each commit, then

buildman -b <branch> <board> -sS

You can add -B for function bloat and --step 0 to diff just the first
and last commits.


- Simon

>
> пт, 30 дек. 2022 г. в 01:41, Simon Glass <sjg@chromium.org>:
> >
> > Hi Dzmitry,
> >
> > On Mon, 26 Dec 2022 at 13:50, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
> > >
> > > - get rid of code duplications in switch across bpp values
> > > - extract common pixel fill logic in two functions one per
> > > horizontal and vertical filling
> > > - rearrange statements in put_xy* methods in unified way
> > > - replace types - uint*_t to u*
> > >
> > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> > > ---
> > >  drivers/video/console_simple.c | 508 ++++++++++++---------------------
> > >  1 file changed, 184 insertions(+), 324 deletions(-)
> >
> > This looks like a nice tidy up.
> >
> > Is there a code-size or performance penalty with this? E.g. with
> > CONFIG_CONSOLE_ROTATION disabled
> >
> > Please can you comment the functions property so we know what they do?
> >
> > Regards,
> > Simon
Dzmitry Sankouski Feb. 13, 2023, 5:02 p.m. UTC | #4 
For version 3 patches:

Summary of 8 commits for 1232 boards (4 threads, 1 job per thread)
01: video console: unite normal and rotated files
02: video console: refactoring and optimization
   sandbox: (for 1/7 boards) all -64.0 text -64.0
03: video console: add support for fonts wider than 1 byte
04: video console: add select font logic to vidconsole uclass driver
   sandbox: (for 1/7 boards) all +288.0 data +32.0 text +256.0
05: video console: allow font size configuration at runtime
   sandbox: (for 1/7 boards) all +1208.0 data +64.0 rodata +96.0 text +1048.0
06: video console: add 12x22 Sun font from linux
07: video console: add 16x32 Terminus font from linux
08: video console: add 12x22 console simple font test
   sandbox: (for 1/7 boards) all +13152.0 data +11336.0 rodata +256.0
text +1560.0
(no errors to report)

ср, 4 янв. 2023 г. в 23:02, Simon Glass <sjg@chromium.org>:

>
> Hi Dzmitry,
>
> On Wed, 4 Jan 2023 at 04:17, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
> >
> > Regarding code-size there's a gain with CONFIG_CONSOLE_ROTATION
> > enabled, and penalty with disabled:
> >
> > New:
> > CONFIG_VIDEO_CONSOLE=y
> > CONFIG_CONSOLE_ROTATION=y
> > dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
> > 108208  drivers/video/console_simple.o
> >
> > CONFIG_VIDEO_CONSOLE=y
> > # CONFIG_CONSOLE_ROTATION is not set
> > dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_simple.o
> > 53848   drivers/video/console_simple.o
> >
> > Old:
> > dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_normal.o
> > 44728   drivers/video/console_normal.o
> > dzmitry@debian:~/side/u-boot$ du --bytes drivers/video/console_rotate.o
> > 85424   drivers/video/console_rotate.o
> >
> > In theory, there should be a small performance penalty for the `if
> > (direction)` statement -
> > for every row, and for each pixel. For an 8x16 font, it'll be 144 if statements.
> >
> > I'll comment on functions in the next patch versions.
>
> To check this, use:
>
> buildman -b <branch> <board>
>
> to build each commit, then
>
> buildman -b <branch> <board> -sS
>
> You can add -B for function bloat and --step 0 to diff just the first
> and last commits.
>
>
> - Simon
>
> >
> > пт, 30 дек. 2022 г. в 01:41, Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Dzmitry,
> > >
> > > On Mon, 26 Dec 2022 at 13:50, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
> > > >
> > > > - get rid of code duplications in switch across bpp values
> > > > - extract common pixel fill logic in two functions one per
> > > > horizontal and vertical filling
> > > > - rearrange statements in put_xy* methods in unified way
> > > > - replace types - uint*_t to u*
> > > >
> > > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> > > > ---
> > > >  drivers/video/console_simple.c | 508 ++++++++++++---------------------
> > > >  1 file changed, 184 insertions(+), 324 deletions(-)
> > >
> > > This looks like a nice tidy up.
> > >
> > > Is there a code-size or performance penalty with this? E.g. with
> > > CONFIG_CONSOLE_ROTATION disabled
> > >
> > > Please can you comment the functions property so we know what they do?
> > >
> > > Regards,
> > > Simon
diff mbox series

Patch

diff --git a/drivers/video/console_simple.c b/drivers/video/console_simple.c
index a4b3cfe3d8..20087a30af 100644
--- a/drivers/video/console_simple.c
+++ b/drivers/video/console_simple.c
@@ -3,6 +3,7 @@ 
  * Copyright (c) 2015 Google, Inc
  * (C) Copyright 2015
  * Bernecker & Rainer Industrieelektronik GmbH - http://www.br-automation.com
+ * (C) Copyright 2023 Dzmitry Sankouski <dsankouski@gmail.com>
  */
 
 #include <common.h>
@@ -11,46 +12,137 @@ 
 #include <video_console.h>
 #include <video_font.h>		/* Get font data, width and height */
 
-static int console_normal_set_row(struct udevice *dev, uint row, int clr)
+#define FLIPPED_DIRECTION 1
+#define NORMAL_DIRECTION 0
+
+static int check_bpix_support(int bpix)
+{
+	switch (bpix) {
+	case VIDEO_BPP8:
+		if (IS_ENABLED(CONFIG_VIDEO_BPP8))
+			return 0;
+	case VIDEO_BPP16:
+		if (IS_ENABLED(CONFIG_VIDEO_BPP16))
+			return 0;
+	case VIDEO_BPP32:
+		if (IS_ENABLED(CONFIG_VIDEO_BPP32))
+			return 0;
+	default:
+		return -ENOSYS;
+	}
+}
+
+static inline int fill_pixel_and_goto_next(void **dstp, u32 value, int pbytes, bool go_down)
+{
+	u8 *dst_byte = *dstp;
+
+	if (pbytes == 4) {
+		u32 *dst = *dstp;
+		*dst = value;
+	}
+	if (pbytes == 2) {
+		u16 *dst = *dstp;
+		*dst = value;
+	}
+	if (pbytes == 1) {
+		u8 *dst = *dstp;
+		*dst = value;
+	}
+	if (go_down)
+		*dstp = dst_byte - pbytes;
+	else
+		*dstp = dst_byte + pbytes;
+	return 0;
+}
+
+#if (CONFIG_IS_ENABLED(CONSOLE_ROTATION))
+static int fill_char_horizontally(uchar *pfont, void **line, struct video_priv *vid_priv,
+				  bool direction)
 {
-	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
-	void *line, *end;
-	int pixels = VIDEO_FONT_HEIGHT * vid_priv->xsize;
 	int ret;
-	int i;
+	void *dst;
+	u8 mask = 0x80;
 
-	line = vid_priv->fb + row * VIDEO_FONT_HEIGHT * vid_priv->line_length;
-	switch (vid_priv->bpix) {
-	case VIDEO_BPP8:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-			uint8_t *dst = line;
+	ret = check_bpix_support(vid_priv->bpix);
+	if (ret)
+		return ret;
 
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
-		}
-	case VIDEO_BPP16:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-			uint16_t *dst = line;
+	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;
 
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
+			ret = fill_pixel_and_goto_next(&dst,
+						       value,
+						       VNBYTES(vid_priv->bpix),
+						       direction
+			);
 		}
-	case VIDEO_BPP32:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-			uint32_t *dst = line;
+		if (direction)
+			*line += vid_priv->line_length;
+		else
+			*line -= vid_priv->line_length;
+		mask >>= 1;
+	}
+	return ret;
+}
+#endif
 
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
+static int fill_char_vertically(uchar *pfont, void **line, struct video_priv *vid_priv,
+				bool direction)
+{
+	int ret;
+	void *dst;
+
+	ret = check_bpix_support(vid_priv->bpix);
+	if (ret)
+		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;
 		}
-	default:
-		return -ENOSYS;
+		if (direction)
+			*line -= vid_priv->line_length;
+		else
+			*line += vid_priv->line_length;
 	}
+	return ret;
+}
+
+static int console_set_row(struct udevice *dev, uint row, int clr)
+{
+	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
+	void *line, *dst, *end;
+	int pixels = VIDEO_FONT_HEIGHT * vid_priv->xsize;
+	int ret;
+	int i;
+	int pbytes;
+
+	ret = check_bpix_support(vid_priv->bpix);
+	if (ret)
+		return ret;
+
+	line = vid_priv->fb + row * VIDEO_FONT_HEIGHT * vid_priv->line_length;
+	dst = line;
+	pbytes = VNBYTES(vid_priv->bpix);
+	for (i = 0; i < pixels; i++)
+		fill_pixel_and_goto_next(&dst, clr, pbytes, NORMAL_DIRECTION);
+	end = dst;
+
 	ret = vidconsole_sync_copy(dev, line, end);
 	if (ret)
 		return ret;
@@ -58,8 +150,8 @@  static int console_normal_set_row(struct udevice *dev, uint row, int clr)
 	return 0;
 }
 
-static int console_normal_move_rows(struct udevice *dev, uint rowdst,
-				     uint rowsrc, uint count)
+static int console_move_rows(struct udevice *dev, uint rowdst,
+			     uint rowsrc, uint count)
 {
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
 	void *dst;
@@ -77,70 +169,30 @@  static int console_normal_move_rows(struct udevice *dev, uint rowdst,
 	return 0;
 }
 
-static int console_normal_putc_xy(struct udevice *dev, uint x_frac, uint y,
-				  char ch)
+static int console_putc_xy(struct udevice *dev, uint x_frac, uint y, char ch)
 {
 	struct vidconsole_priv *vc_priv = dev_get_uclass_priv(dev);
 	struct udevice *vid = dev->parent;
 	struct video_priv *vid_priv = dev_get_uclass_priv(vid);
-	int i, row;
-	void *start;
-	void *line;
-	int ret;
+	int pbytes = VNBYTES(vid_priv->bpix);
+	int x, linenum, ret;
+	void *start, *line;
+	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 
-	start = vid_priv->fb + y * vid_priv->line_length +
-		VID_TO_PIXEL(x_frac) * VNBYTES(vid_priv->bpix);
+	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
+		return -EAGAIN;
+	linenum = y;
+	x = VID_TO_PIXEL(x_frac);
+	start = vid_priv->fb + linenum * vid_priv->line_length + x * pbytes;
 	line = start;
 
 	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
 		return -EAGAIN;
 
-	for (row = 0; row < VIDEO_FONT_HEIGHT; row++) {
-		unsigned int idx = (u8)ch * VIDEO_FONT_HEIGHT + row;
-		uchar bits = video_fontdata[idx];
-
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
+	ret = fill_char_vertically(pfont, &line, vid_priv, NORMAL_DIRECTION);
+	if (ret)
+		return ret;
 
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst++ = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst++ = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst++ = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
-		line += vid_priv->line_length;
-	}
 	ret = vidconsole_sync_copy(dev, start, line);
 	if (ret)
 		return ret;
@@ -148,7 +200,7 @@  static int console_normal_putc_xy(struct udevice *dev, uint x_frac, uint y,
 	return VID_TO_POS(VIDEO_FONT_WIDTH);
 }
 
-static int console_normal_probe(struct udevice *dev)
+static int console_probe(struct udevice *dev)
 {
 	struct vidconsole_priv *vc_priv = dev_get_uclass_priv(dev);
 	struct udevice *vid_dev = dev->parent;
@@ -162,25 +214,25 @@  static int console_normal_probe(struct udevice *dev)
 	return 0;
 }
 
-struct vidconsole_ops console_normal_ops = {
-	.putc_xy	= console_normal_putc_xy,
-	.move_rows	= console_normal_move_rows,
-	.set_row	= console_normal_set_row,
+struct vidconsole_ops console_ops = {
+	.putc_xy	= console_putc_xy,
+	.move_rows	= console_move_rows,
+	.set_row	= console_set_row,
 };
 
 U_BOOT_DRIVER(vidconsole_normal) = {
 	.name	= "vidconsole0",
 	.id	= UCLASS_VIDEO_CONSOLE,
-	.ops	= &console_normal_ops,
-	.probe	= console_normal_probe,
+	.ops	= &console_ops,
+	.probe	= console_probe,
 };
 
-#if defined(CONFIG_CONSOLE_ROTATION)
+#if (CONFIG_IS_ENABLED(CONSOLE_ROTATION))
 static int console_set_row_1(struct udevice *dev, uint row, int clr)
 {
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
 	int pbytes = VNBYTES(vid_priv->bpix);
-	void *start, *line;
+	void *start, *dst, *line;
 	int i, j;
 	int ret;
 
@@ -188,34 +240,9 @@  static int console_set_row_1(struct udevice *dev, uint row, int clr)
 		(row + 1) * VIDEO_FONT_HEIGHT * pbytes;
 	line = start;
 	for (j = 0; j < vid_priv->ysize; j++) {
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
+		dst = line;
+		for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
+			fill_pixel_and_goto_next(&dst, clr, pbytes, NORMAL_DIRECTION);
 		line += vid_priv->line_length;
 	}
 	ret = vidconsole_sync_copy(dev, start, line);
@@ -226,7 +253,7 @@  static int console_set_row_1(struct udevice *dev, uint row, int clr)
 }
 
 static int console_move_rows_1(struct udevice *dev, uint rowdst, uint rowsrc,
-			       uint count)
+				   uint count)
 {
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
 	int pbytes = VNBYTES(vid_priv->bpix);
@@ -241,7 +268,7 @@  static int console_move_rows_1(struct udevice *dev, uint rowdst, uint rowsrc,
 
 	for (j = 0; j < vid_priv->ysize; j++) {
 		ret = vidconsole_memmove(dev, dst, src,
-					 VIDEO_FONT_HEIGHT * pbytes * count);
+					VIDEO_FONT_HEIGHT * pbytes * count);
 		if (ret)
 			return ret;
 		src += vid_priv->line_length;
@@ -256,60 +283,22 @@  static int console_putc_xy_1(struct udevice *dev, uint x_frac, uint y, char ch)
 	struct vidconsole_priv *vc_priv = dev_get_uclass_priv(dev);
 	struct udevice *vid = dev->parent;
 	struct video_priv *vid_priv = dev_get_uclass_priv(vid);
-	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 	int pbytes = VNBYTES(vid_priv->bpix);
-	int i, col, x, linenum, ret;
-	int mask = 0x80;
+	int x, linenum, ret;
 	void *start, *line;
+	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 
+	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
+		return -EAGAIN;
 	linenum = VID_TO_PIXEL(x_frac) + 1;
 	x = y + 1;
 	start = vid_priv->fb + linenum * vid_priv->line_length - x * pbytes;
 	line = start;
-	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
-		return -EAGAIN;
 
-	for (col = 0; col < VIDEO_FONT_HEIGHT; col++) {
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
+	ret = fill_char_horizontally(pfont, &line, vid_priv, FLIPPED_DIRECTION);
+	if (ret)
+		return ret;
 
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst-- = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst-- = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst-- = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
-		line += vid_priv->line_length;
-		mask >>= 1;
-	}
 	/* We draw backwards from 'start, so account for the first line */
 	ret = vidconsole_sync_copy(dev, start - vid_priv->line_length, line);
 	if (ret)
@@ -322,44 +311,18 @@  static int console_putc_xy_1(struct udevice *dev, uint x_frac, uint y, char ch)
 static int console_set_row_2(struct udevice *dev, uint row, int clr)
 {
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
-	void *start, *line, *end;
+	void *start, *line, *dst, *end;
 	int pixels = VIDEO_FONT_HEIGHT * vid_priv->xsize;
 	int i, ret;
+	int pbytes = VNBYTES(vid_priv->bpix);
 
 	start = vid_priv->fb + vid_priv->ysize * vid_priv->line_length -
 		(row + 1) * VIDEO_FONT_HEIGHT * vid_priv->line_length;
 	line = start;
-	switch (vid_priv->bpix) {
-	case VIDEO_BPP8:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-			uint8_t *dst = line;
-
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
-		}
-	case VIDEO_BPP16:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-			uint16_t *dst = line;
-
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
-		}
-	case VIDEO_BPP32:
-		if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-			uint32_t *dst = line;
-
-			for (i = 0; i < pixels; i++)
-				*dst++ = clr;
-			end = dst;
-			break;
-		}
-	default:
-		return -ENOSYS;
-	}
+	dst = line;
+	for (i = 0; i < pixels; i++)
+		fill_pixel_and_goto_next(&dst, clr, pbytes, NORMAL_DIRECTION);
+	end = dst;
 	ret = vidconsole_sync_copy(dev, start, end);
 	if (ret)
 		return ret;
@@ -392,8 +355,9 @@  static int console_putc_xy_2(struct udevice *dev, uint x_frac, uint y, char ch)
 	struct udevice *vid = dev->parent;
 	struct video_priv *vid_priv = dev_get_uclass_priv(vid);
 	int pbytes = VNBYTES(vid_priv->bpix);
-	int i, row, x, linenum, ret;
+	int linenum, x, ret;
 	void *start, *line;
+	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 
 	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
 		return -EAGAIN;
@@ -402,52 +366,10 @@  static int console_putc_xy_2(struct udevice *dev, uint x_frac, uint y, char ch)
 	start = vid_priv->fb + linenum * vid_priv->line_length + x * pbytes;
 	line = start;
 
-	for (row = 0; row < VIDEO_FONT_HEIGHT; row++) {
-		unsigned int idx = (u8)ch * VIDEO_FONT_HEIGHT + row;
-		uchar bits = video_fontdata[idx];
-
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
+	ret = fill_char_vertically(pfont, &line, vid_priv, FLIPPED_DIRECTION);
+	if (ret)
+		return ret;
 
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst-- = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst-- = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_WIDTH; i++) {
-					*dst-- = (bits & 0x80) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-					bits <<= 1;
-				}
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
-		line -= vid_priv->line_length;
-	}
 	/* Add 4 bytes to allow for the first pixel writen */
 	ret = vidconsole_sync_copy(dev, start + 4, line);
 	if (ret)
@@ -460,40 +382,15 @@  static int console_set_row_3(struct udevice *dev, uint row, int clr)
 {
 	struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent);
 	int pbytes = VNBYTES(vid_priv->bpix);
-	void *start, *line;
+	void *start, *dst, *line;
 	int i, j, ret;
 
 	start = vid_priv->fb + row * VIDEO_FONT_HEIGHT * pbytes;
 	line = start;
 	for (j = 0; j < vid_priv->ysize; j++) {
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
-					*dst++ = clr;
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
+		dst = line;
+		for (i = 0; i < VIDEO_FONT_HEIGHT; i++)
+			fill_pixel_and_goto_next(&dst, clr, pbytes, NORMAL_DIRECTION);
 		line += vid_priv->line_length;
 	}
 	ret = vidconsole_sync_copy(dev, start, line);
@@ -517,7 +414,7 @@  static int console_move_rows_3(struct udevice *dev, uint rowdst, uint rowsrc,
 
 	for (j = 0; j < vid_priv->ysize; j++) {
 		ret = vidconsole_memmove(dev, dst, src,
-					 VIDEO_FONT_HEIGHT * pbytes * count);
+					VIDEO_FONT_HEIGHT * pbytes * count);
 		if (ret)
 			return ret;
 		src += vid_priv->line_length;
@@ -532,58 +429,21 @@  static int console_putc_xy_3(struct udevice *dev, uint x_frac, uint y, char ch)
 	struct vidconsole_priv *vc_priv = dev_get_uclass_priv(dev);
 	struct udevice *vid = dev->parent;
 	struct video_priv *vid_priv = dev_get_uclass_priv(vid);
-	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 	int pbytes = VNBYTES(vid_priv->bpix);
-	int i, col, x, ret;
-	int mask = 0x80;
+	int linenum, x, ret;
 	void *start, *line;
+	uchar *pfont = video_fontdata + (u8)ch * VIDEO_FONT_HEIGHT;
 
 	if (x_frac + VID_TO_POS(vc_priv->x_charsize) > vc_priv->xsize_frac)
 		return -EAGAIN;
-	x = vid_priv->ysize - VID_TO_PIXEL(x_frac) - 1;
-	start = vid_priv->fb + x * vid_priv->line_length + y * pbytes;
+	x = y;
+	linenum = vid_priv->ysize - VID_TO_PIXEL(x_frac) - 1;
+	start = vid_priv->fb + linenum * vid_priv->line_length + y * pbytes;
 	line = start;
-	for (col = 0; col < VIDEO_FONT_HEIGHT; col++) {
-		switch (vid_priv->bpix) {
-		case VIDEO_BPP8:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP8)) {
-				uint8_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst++ = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		case VIDEO_BPP16:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP16)) {
-				uint16_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst++ = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		case VIDEO_BPP32:
-			if (IS_ENABLED(CONFIG_VIDEO_BPP32)) {
-				uint32_t *dst = line;
-
-				for (i = 0; i < VIDEO_FONT_HEIGHT; i++) {
-					*dst++ = (pfont[i] & mask) ?
-						vid_priv->colour_fg :
-						vid_priv->colour_bg;
-				}
-				break;
-			}
-		default:
-			return -ENOSYS;
-		}
-		line -= vid_priv->line_length;
-		mask >>= 1;
-	}
+
+	ret = fill_char_horizontally(pfont, &line, vid_priv, NORMAL_DIRECTION);
+	if (ret)
+		return ret;
 	/* Add a line to allow for the first pixels writen */
 	ret = vidconsole_sync_copy(dev, start + vid_priv->line_length, line);
 	if (ret)