Mailing List Archive

[PATCH v4 3/7] xen: introduce a generic framebuffer driver
Abstract away from vesa.c the funcions to handle a linear framebuffer
and print characters to it.
Make use of the new functions in vesa.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Changes in v4:
- squash the vesa.c changes into this patch;
- rename fb* to lfb*;
- pass a pointer to fb_init;
- use %u for screen dimensions;
- specify loglevel in printk;
- call fb_free on error in fb_alloc;
- no __init on declarations;
- do not break messages to fit 80 columns.
---
xen/drivers/video/Makefile | 1 +
xen/drivers/video/lfb.c | 206 ++++++++++++++++++++++++++++++++++++++++++++
xen/drivers/video/lfb.h | 49 +++++++++++
xen/drivers/video/vesa.c | 179 ++++++--------------------------------
4 files changed, 282 insertions(+), 153 deletions(-)
create mode 100644 xen/drivers/video/lfb.c
create mode 100644 xen/drivers/video/lfb.h

diff --git a/xen/drivers/video/Makefile b/xen/drivers/video/Makefile
index 2993c39..77f9d5d 100644
--- a/xen/drivers/video/Makefile
+++ b/xen/drivers/video/Makefile
@@ -2,4 +2,5 @@ obj-$(HAS_VGA) := vga.o
obj-$(HAS_VIDEO) += font_8x14.o
obj-$(HAS_VIDEO) += font_8x16.o
obj-$(HAS_VIDEO) += font_8x8.o
+obj-$(HAS_VIDEO) += lfb.o
obj-$(HAS_VGA) += vesa.o
diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
new file mode 100644
index 0000000..84d92d9
--- /dev/null
+++ b/xen/drivers/video/lfb.c
@@ -0,0 +1,206 @@
+/******************************************************************************
+ * lfb.c
+ *
+ * linear frame buffer handling.
+ */
+
+#include <xen/config.h>
+#include <xen/kernel.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include "lfb.h"
+#include "font.h"
+
+#define MAX_XRES 1900
+#define MAX_YRES 1200
+#define MAX_BPP 4
+#define MAX_FONT_W 8
+#define MAX_FONT_H 16
+static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
+static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
+static __initdata unsigned char text_buf[(MAX_XRES / MAX_FONT_W) * \
+ (MAX_YRES / MAX_FONT_H)];
+
+struct lfb_status {
+ struct lfb_prop lfbp;
+
+ unsigned char *lbuf, *text_buf;
+ unsigned int *line_len;
+ unsigned int xpos, ypos;
+};
+static struct lfb_status lfb;
+
+static void lfb_show_line(
+ const unsigned char *text_line,
+ unsigned char *video_line,
+ unsigned int nr_chars,
+ unsigned int nr_cells)
+{
+ unsigned int i, j, b, bpp, pixel;
+
+ bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
+
+ for ( i = 0; i < lfb.lfbp.font->height; i++ )
+ {
+ unsigned char *ptr = lfb.lbuf;
+
+ for ( j = 0; j < nr_chars; j++ )
+ {
+ const unsigned char *bits = lfb.lfbp.font->data;
+ bits += ((text_line[j] * lfb.lfbp.font->height + i) *
+ ((lfb.lfbp.font->width + 7) >> 3));
+ for ( b = lfb.lfbp.font->width; b--; )
+ {
+ pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
+ memcpy(ptr, &pixel, bpp);
+ ptr += bpp;
+ }
+ }
+
+ memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
+ memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
+ video_line += lfb.lfbp.bytes_per_line;
+ }
+}
+
+/* Fast mode which redraws all modified parts of a 2D text buffer. */
+void lfb_redraw_puts(const char *s)
+{
+ unsigned int i, min_redraw_y = lfb.ypos;
+ char c;
+
+ /* Paste characters into text buffer. */
+ while ( (c = *s++) != '\0' )
+ {
+ if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
+ {
+ if ( ++lfb.ypos >= lfb.lfbp.text_rows )
+ {
+ min_redraw_y = 0;
+ lfb.ypos = lfb.lfbp.text_rows - 1;
+ memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
+ lfb.ypos * lfb.lfbp.text_columns);
+ memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
+ }
+ lfb.xpos = 0;
+ }
+
+ if ( c != '\n' )
+ lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
+ }
+
+ /* Render modified section of text buffer to VESA linear framebuffer. */
+ for ( i = min_redraw_y; i <= lfb.ypos; i++ )
+ {
+ const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
+ unsigned int width;
+
+ for ( width = lfb.lfbp.text_columns; width; --width )
+ if ( line[width - 1] )
+ break;
+ lfb_show_line(line,
+ lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
+ width, max(lfb.line_len[i], width));
+ lfb.line_len[i] = width;
+ }
+
+ lfb.lfbp.flush();
+}
+
+/* Slower line-based scroll mode which interacts better with dom0. */
+void lfb_scroll_puts(const char *s)
+{
+ unsigned int i;
+ char c;
+
+ while ( (c = *s++) != '\0' )
+ {
+ if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
+ {
+ unsigned int bytes = (lfb.lfbp.width *
+ ((lfb.lfbp.bits_per_pixel + 7) >> 3));
+ unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
+ unsigned char *dst = lfb.lfbp.lfb;
+
+ /* New line: scroll all previous rows up one line. */
+ for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
+ {
+ memcpy(dst, src, bytes);
+ src += lfb.lfbp.bytes_per_line;
+ dst += lfb.lfbp.bytes_per_line;
+ }
+
+ /* Render new line. */
+ lfb_show_line(
+ lfb.text_buf,
+ lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
+ lfb.xpos, lfb.lfbp.text_columns);
+
+ lfb.xpos = 0;
+ }
+
+ if ( c != '\n' )
+ lfb.text_buf[lfb.xpos++] = c;
+ }
+
+ lfb.lfbp.flush();
+}
+
+void lfb_carriage_return(void)
+{
+ lfb.xpos = 0;
+}
+
+int __init lfb_init(struct lfb_prop *lfbp)
+{
+ if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
+ {
+ printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer early.\n",
+ lfbp->width, lfbp->height);
+ return -EINVAL;
+ }
+
+ lfb.lfbp = *lfbp;
+ lfb.lbuf = lbuf;
+ lfb.text_buf = text_buf;
+ lfb.line_len = line_len;
+ return 0;
+}
+
+int __init lfb_alloc(void)
+{
+ lfb.lbuf = NULL;
+ lfb.text_buf = NULL;
+ lfb.line_len = NULL;
+
+ lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
+ if ( !lfb.lbuf )
+ goto fail;
+
+ lfb.text_buf = xzalloc_bytes(lfb.lfbp.text_columns * lfb.lfbp.text_rows);
+ if ( !lfb.text_buf )
+ goto fail;
+
+ lfb.line_len = xzalloc_array(unsigned int, lfb.lfbp.text_columns);
+ if ( !lfb.line_len )
+ goto fail;
+
+ memcpy(lfb.lbuf, lbuf, lfb.lfbp.bytes_per_line);
+ memcpy(lfb.text_buf, text_buf, lfb.lfbp.text_columns * lfb.lfbp.text_rows);
+ memcpy(lfb.line_len, line_len, lfb.lfbp.text_columns);
+
+ return 0;
+
+fail:
+ printk(XENLOG_ERR "Couldn't allocate enough memory to drive the framebuffer\n");
+ lfb_free();
+
+ return -ENOMEM;
+}
+
+void lfb_free(void)
+{
+ xfree(lfb.lbuf);
+ xfree(lfb.text_buf);
+ xfree(lfb.line_len);
+}
diff --git a/xen/drivers/video/lfb.h b/xen/drivers/video/lfb.h
new file mode 100644
index 0000000..479ed04
--- /dev/null
+++ b/xen/drivers/video/lfb.h
@@ -0,0 +1,49 @@
+/*
+ * xen/drivers/video/lfb.h
+ *
+ * Cross-platform framebuffer library
+ *
+ * Stefano Stabellini <stefano.stabellini@eu.citrix.com>
+ * Copyright (c) 2013 Citrix Systems.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _XEN_LFB_H
+#define _XEN_LFB_H
+
+#include <xen/init.h>
+
+struct lfb_prop {
+ const struct font_desc *font;
+ unsigned char *lfb;
+ unsigned int pixel_on;
+ uint16_t width, height;
+ uint16_t bytes_per_line;
+ uint16_t bits_per_pixel;
+ void (*flush)(void);
+
+ unsigned int text_columns;
+ unsigned int text_rows;
+};
+
+void lfb_redraw_puts(const char *s);
+void lfb_scroll_puts(const char *s);
+void lfb_carriage_return(void);
+void lfb_free(void);
+
+/* initialize the framebuffer, can be called early (before xmalloc is
+ * available) */
+int lfb_init(struct lfb_prop *lfbp);
+/* lfb_alloc allocates internal structures using xmalloc */
+int lfb_alloc(void);
+
+#endif
diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
index aaf8b23..c216371 100644
--- a/xen/drivers/video/vesa.c
+++ b/xen/drivers/video/vesa.c
@@ -13,20 +13,15 @@
#include <asm/io.h>
#include <asm/page.h>
#include "font.h"
+#include "lfb.h"

#define vlfb_info vga_console_info.u.vesa_lfb
-#define text_columns (vlfb_info.width / font->width)
-#define text_rows (vlfb_info.height / font->height)

-static void vesa_redraw_puts(const char *s);
-static void vesa_scroll_puts(const char *s);
+static void lfb_flush(void);

-static unsigned char *lfb, *lbuf, *text_buf;
-static unsigned int *__initdata line_len;
+static unsigned char *lfb;
static const struct font_desc *font;
static bool_t vga_compat;
-static unsigned int pixel_on;
-static unsigned int xpos, ypos;

static unsigned int vram_total;
integer_param("vesa-ram", vram_total);
@@ -87,29 +82,26 @@ void __init vesa_early_init(void)

void __init vesa_init(void)
{
- if ( !font )
- goto fail;
-
- lbuf = xmalloc_bytes(vlfb_info.bytes_per_line);
- if ( !lbuf )
- goto fail;
+ struct lfb_prop lfbp;

- text_buf = xzalloc_bytes(text_columns * text_rows);
- if ( !text_buf )
- goto fail;
+ if ( !font )
+ return;

- line_len = xzalloc_array(unsigned int, text_columns);
- if ( !line_len )
- goto fail;
+ lfbp.font = font;
+ lfbp.bits_per_pixel = vlfb_info.bits_per_pixel;
+ lfbp.bytes_per_line = vlfb_info.bytes_per_line;
+ lfbp.width = vlfb_info.width;
+ lfbp.height = vlfb_info.height;
+ lfbp.flush = lfb_flush;
+ lfbp.text_columns = vlfb_info.width / font->width;
+ lfbp.text_rows = vlfb_info.height / font->height;

- lfb = ioremap(vlfb_info.lfb_base, vram_remap);
+ lfbp.lfb = lfb = ioremap(vlfb_info.lfb_base, vram_remap);
if ( !lfb )
- goto fail;
+ return;

memset(lfb, 0, vram_remap);

- video_puts = vesa_redraw_puts;
-
printk(XENLOG_INFO "vesafb: framebuffer at %#x, mapped to 0x%p, "
"using %uk, total %uk\n",
vlfb_info.lfb_base, lfb,
@@ -131,7 +123,7 @@ void __init vesa_init(void)
{
/* Light grey in truecolor. */
unsigned int grey = 0xaaaaaaaa;
- pixel_on =
+ fbp.pixel_on =
((grey >> (32 - vlfb_info. red_size)) << vlfb_info. red_pos) |
((grey >> (32 - vlfb_info.green_size)) << vlfb_info.green_pos) |
((grey >> (32 - vlfb_info. blue_size)) << vlfb_info. blue_pos);
@@ -139,15 +131,14 @@ void __init vesa_init(void)
else
{
/* White(ish) in default pseudocolor palette. */
- pixel_on = 7;
+ fbp.pixel_on = 7;
}

- return;
-
- fail:
- xfree(lbuf);
- xfree(text_buf);
- xfree(line_len);
+ if ( lfb_init(&lfbp) < 0 )
+ return;
+ if ( lfb_alloc() < 0 )
+ return;
+ video_puts = lfb_redraw_puts;
}

#include <asm/mtrr.h>
@@ -192,8 +183,8 @@ void __init vesa_endboot(bool_t keep)
{
if ( keep )
{
- xpos = 0;
- video_puts = vesa_scroll_puts;
+ video_puts = lfb_scroll_puts;
+ lfb_carriage_return();
}
else
{
@@ -202,124 +193,6 @@ void __init vesa_endboot(bool_t keep)
memset(lfb + i * vlfb_info.bytes_per_line, 0,
vlfb_info.width * bpp);
lfb_flush();
+ lfb_free();
}
-
- xfree(line_len);
-}
-
-/* Render one line of text to given linear framebuffer line. */
-static void vesa_show_line(
- const unsigned char *text_line,
- unsigned char *video_line,
- unsigned int nr_chars,
- unsigned int nr_cells)
-{
- unsigned int i, j, b, bpp, pixel;
-
- bpp = (vlfb_info.bits_per_pixel + 7) >> 3;
-
- for ( i = 0; i < font->height; i++ )
- {
- unsigned char *ptr = lbuf;
-
- for ( j = 0; j < nr_chars; j++ )
- {
- const unsigned char *bits = font->data;
- bits += ((text_line[j] * font->height + i) *
- ((font->width + 7) >> 3));
- for ( b = font->width; b--; )
- {
- pixel = (*bits & (1u<<b)) ? pixel_on : 0;
- memcpy(ptr, &pixel, bpp);
- ptr += bpp;
- }
- }
-
- memset(ptr, 0, (vlfb_info.width - nr_chars * font->width) * bpp);
- memcpy(video_line, lbuf, nr_cells * font->width * bpp);
- video_line += vlfb_info.bytes_per_line;
- }
-}
-
-/* Fast mode which redraws all modified parts of a 2D text buffer. */
-static void __init vesa_redraw_puts(const char *s)
-{
- unsigned int i, min_redraw_y = ypos;
- char c;
-
- /* Paste characters into text buffer. */
- while ( (c = *s++) != '\0' )
- {
- if ( (c == '\n') || (xpos >= text_columns) )
- {
- if ( ++ypos >= text_rows )
- {
- min_redraw_y = 0;
- ypos = text_rows - 1;
- memmove(text_buf, text_buf + text_columns,
- ypos * text_columns);
- memset(text_buf + ypos * text_columns, 0, xpos);
- }
- xpos = 0;
- }
-
- if ( c != '\n' )
- text_buf[xpos++ + ypos * text_columns] = c;
- }
-
- /* Render modified section of text buffer to VESA linear framebuffer. */
- for ( i = min_redraw_y; i <= ypos; i++ )
- {
- const unsigned char *line = text_buf + i * text_columns;
- unsigned int width;
-
- for ( width = text_columns; width; --width )
- if ( line[width - 1] )
- break;
- vesa_show_line(line,
- lfb + i * font->height * vlfb_info.bytes_per_line,
- width, max(line_len[i], width));
- line_len[i] = width;
- }
-
- lfb_flush();
-}
-
-/* Slower line-based scroll mode which interacts better with dom0. */
-static void vesa_scroll_puts(const char *s)
-{
- unsigned int i;
- char c;
-
- while ( (c = *s++) != '\0' )
- {
- if ( (c == '\n') || (xpos >= text_columns) )
- {
- unsigned int bytes = (vlfb_info.width *
- ((vlfb_info.bits_per_pixel + 7) >> 3));
- unsigned char *src = lfb + font->height * vlfb_info.bytes_per_line;
- unsigned char *dst = lfb;
-
- /* New line: scroll all previous rows up one line. */
- for ( i = font->height; i < vlfb_info.height; i++ )
- {
- memcpy(dst, src, bytes);
- src += vlfb_info.bytes_per_line;
- dst += vlfb_info.bytes_per_line;
- }
-
- /* Render new line. */
- vesa_show_line(
- text_buf,
- lfb + (text_rows-1) * font->height * vlfb_info.bytes_per_line,
- xpos, text_columns);
-
- xpos = 0;
- }
-
- if ( c != '\n' )
- text_buf[xpos++] = c;
- }
-
- lfb_flush();
}
--
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH v4 3/7] xen: introduce a generic framebuffer driver [ In reply to ]
>>> On 08.01.13 at 21:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> --- /dev/null
> +++ b/xen/drivers/video/lfb.c
> @@ -0,0 +1,206 @@
> +/******************************************************************************
> + * lfb.c
> + *
> + * linear frame buffer handling.
> + */
> +
> +#include <xen/config.h>
> +#include <xen/kernel.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include "lfb.h"
> +#include "font.h"
> +
> +#define MAX_XRES 1900
> +#define MAX_YRES 1200
> +#define MAX_BPP 4
> +#define MAX_FONT_W 8
> +#define MAX_FONT_H 16
> +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
> +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
> +static __initdata unsigned char text_buf[(MAX_XRES / MAX_FONT_W) * \
> + (MAX_YRES / MAX_FONT_H)];

Imo it would be better to move all these __initdata definitions
do to where the using __init functions actually live, to make
sure unintended use of them in non-__init ones is noticed.

> +
> +struct lfb_status {
> + struct lfb_prop lfbp;
> +
> + unsigned char *lbuf, *text_buf;
> + unsigned int *line_len;
> + unsigned int xpos, ypos;
> +};
> +static struct lfb_status lfb;
> +
> +static void lfb_show_line(
> + const unsigned char *text_line,
> + unsigned char *video_line,
> + unsigned int nr_chars,
> + unsigned int nr_cells)
> +{
> + unsigned int i, j, b, bpp, pixel;
> +
> + bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
> +
> + for ( i = 0; i < lfb.lfbp.font->height; i++ )
> + {
> + unsigned char *ptr = lfb.lbuf;
> +
> + for ( j = 0; j < nr_chars; j++ )
> + {
> + const unsigned char *bits = lfb.lfbp.font->data;
> + bits += ((text_line[j] * lfb.lfbp.font->height + i) *
> + ((lfb.lfbp.font->width + 7) >> 3));
> + for ( b = lfb.lfbp.font->width; b--; )
> + {
> + pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
> + memcpy(ptr, &pixel, bpp);
> + ptr += bpp;
> + }
> + }
> +
> + memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
> + memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
> + video_line += lfb.lfbp.bytes_per_line;
> + }
> +}
> +
> +/* Fast mode which redraws all modified parts of a 2D text buffer. */
> +void lfb_redraw_puts(const char *s)
> +{
> + unsigned int i, min_redraw_y = lfb.ypos;
> + char c;
> +
> + /* Paste characters into text buffer. */
> + while ( (c = *s++) != '\0' )
> + {
> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> + {
> + if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> + {
> + min_redraw_y = 0;
> + lfb.ypos = lfb.lfbp.text_rows - 1;
> + memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
> + lfb.ypos * lfb.lfbp.text_columns);
> + memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
> + }
> + lfb.xpos = 0;
> + }
> +
> + if ( c != '\n' )
> + lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
> + }
> +
> + /* Render modified section of text buffer to VESA linear framebuffer. */
> + for ( i = min_redraw_y; i <= lfb.ypos; i++ )
> + {
> + const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
> + unsigned int width;
> +
> + for ( width = lfb.lfbp.text_columns; width; --width )
> + if ( line[width - 1] )
> + break;
> + lfb_show_line(line,
> + lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
> + width, max(lfb.line_len[i], width));
> + lfb.line_len[i] = width;
> + }
> +
> + lfb.lfbp.flush();
> +}
> +
> +/* Slower line-based scroll mode which interacts better with dom0. */
> +void lfb_scroll_puts(const char *s)
> +{
> + unsigned int i;
> + char c;
> +
> + while ( (c = *s++) != '\0' )
> + {
> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> + {
> + unsigned int bytes = (lfb.lfbp.width *
> + ((lfb.lfbp.bits_per_pixel + 7) >> 3));
> + unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
> + unsigned char *dst = lfb.lfbp.lfb;
> +
> + /* New line: scroll all previous rows up one line. */
> + for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
> + {
> + memcpy(dst, src, bytes);
> + src += lfb.lfbp.bytes_per_line;
> + dst += lfb.lfbp.bytes_per_line;
> + }
> +
> + /* Render new line. */
> + lfb_show_line(
> + lfb.text_buf,
> + lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,

Long line?

> + lfb.xpos, lfb.lfbp.text_columns);
> +
> + lfb.xpos = 0;
> + }
> +
> + if ( c != '\n' )
> + lfb.text_buf[lfb.xpos++] = c;
> + }
> +
> + lfb.lfbp.flush();
> +}
> +
> +void lfb_carriage_return(void)
> +{
> + lfb.xpos = 0;
> +}
> +
> +int __init lfb_init(struct lfb_prop *lfbp)
> +{
> + if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
> + {
> + printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer early.\n",
> + lfbp->width, lfbp->height);

Indentation?

> + return -EINVAL;
> + }
> +
> + lfb.lfbp = *lfbp;
> + lfb.lbuf = lbuf;
> + lfb.text_buf = text_buf;
> + lfb.line_len = line_len;
> + return 0;
> +}
> +
> +int __init lfb_alloc(void)
> +{
> + lfb.lbuf = NULL;
> + lfb.text_buf = NULL;
> + lfb.line_len = NULL;
> +
> + lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
> + if ( !lfb.lbuf )
> + goto fail;
> +
> + lfb.text_buf = xzalloc_bytes(lfb.lfbp.text_columns * lfb.lfbp.text_rows);
> + if ( !lfb.text_buf )
> + goto fail;
> +
> + lfb.line_len = xzalloc_array(unsigned int, lfb.lfbp.text_columns);
> + if ( !lfb.line_len )
> + goto fail;
> +
> + memcpy(lfb.lbuf, lbuf, lfb.lfbp.bytes_per_line);
> + memcpy(lfb.text_buf, text_buf, lfb.lfbp.text_columns * lfb.lfbp.text_rows);
> + memcpy(lfb.line_len, line_len, lfb.lfbp.text_columns);
> +
> + return 0;
> +
> +fail:
> + printk(XENLOG_ERR "Couldn't allocate enough memory to drive the framebuffer\n");
> + lfb_free();
> +
> + return -ENOMEM;
> +}
> +
> +void lfb_free(void)
> +{
> + xfree(lfb.lbuf);
> + xfree(lfb.text_buf);
> + xfree(lfb.line_len);
> +}


This function would then perhaps better go before the __init stuff.

> @@ -139,15 +131,14 @@ void __init vesa_init(void)
> else
> {
> /* White(ish) in default pseudocolor palette. */
> - pixel_on = 7;
> + fbp.pixel_on = 7;
> }
>
> - return;
> -
> - fail:
> - xfree(lbuf);
> - xfree(text_buf);
> - xfree(line_len);
> + if ( lfb_init(&lfbp) < 0 )
> + return;
> + if ( lfb_alloc() < 0 )
> + return;

What's the point of calling lfb_alloc() right after lfb_init()? The static
buffers set up by lfb_init() will get replaced by dynamically allocated
ones right away. Or in other words - what do you need the static
buffers for in the first place?

Jan

> + video_puts = lfb_redraw_puts;
> }
>
> #include <asm/mtrr.h>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH v4 3/7] xen: introduce a generic framebuffer driver [ In reply to ]
On Wed, 9 Jan 2013, Jan Beulich wrote:
> >>> On 08.01.13 at 21:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > --- /dev/null
> > +++ b/xen/drivers/video/lfb.c
> > @@ -0,0 +1,206 @@
> > +/******************************************************************************
> > + * lfb.c
> > + *
> > + * linear frame buffer handling.
> > + */
> > +
> > +#include <xen/config.h>
> > +#include <xen/kernel.h>
> > +#include <xen/lib.h>
> > +#include <xen/errno.h>
> > +#include "lfb.h"
> > +#include "font.h"
> > +
> > +#define MAX_XRES 1900
> > +#define MAX_YRES 1200
> > +#define MAX_BPP 4
> > +#define MAX_FONT_W 8
> > +#define MAX_FONT_H 16
> > +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
> > +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
> > +static __initdata unsigned char text_buf[.(MAX_XRES / MAX_FONT_W) * \
> > + (MAX_YRES / MAX_FONT_H)];
>
> Imo it would be better to move all these __initdata definitions
> do to where the using __init functions actually live, to make
> sure unintended use of them in non-__init ones is noticed.

OK


> > +
> > +struct lfb_status {
> > + struct lfb_prop lfbp;
> > +
> > + unsigned char *lbuf, *text_buf;
> > + unsigned int *line_len;
> > + unsigned int xpos, ypos;
> > +};
> > +static struct lfb_status lfb;
> > +
> > +static void lfb_show_line(
> > + const unsigned char *text_line,
> > + unsigned char *video_line,
> > + unsigned int nr_chars,
> > + unsigned int nr_cells)
> > +{
> > + unsigned int i, j, b, bpp, pixel;
> > +
> > + bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
> > +
> > + for ( i = 0; i < lfb.lfbp.font->height; i++ )
> > + {
> > + unsigned char *ptr = lfb.lbuf;
> > +
> > + for ( j = 0; j < nr_chars; j++ )
> > + {
> > + const unsigned char *bits = lfb.lfbp.font->data;
> > + bits += ((text_line[j] * lfb.lfbp.font->height + i) *
> > + ((lfb.lfbp.font->width + 7) >> 3));
> > + for ( b = lfb.lfbp.font->width; b--; )
> > + {
> > + pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
> > + memcpy(ptr, &pixel, bpp);
> > + ptr += bpp;
> > + }
> > + }
> > +
> > + memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
> > + memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
> > + video_line += lfb.lfbp.bytes_per_line;
> > + }
> > +}
> > +
> > +/* Fast mode which redraws all modified parts of a 2D text buffer. */
> > +void lfb_redraw_puts(const char *s)
> > +{
> > + unsigned int i, min_redraw_y = lfb.ypos;
> > + char c;
> > +
> > + /* Paste characters into text buffer. */
> > + while ( (c = *s++) != '\0' )
> > + {
> > + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> > + {
> > + if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> > + {
> > + min_redraw_y = 0;
> > + lfb.ypos = lfb.lfbp.text_rows - 1;
> > + memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
> > + lfb.ypos * lfb.lfbp.text_columns);
> > + memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
> > + }
> > + lfb.xpos = 0;
> > + }
> > +
> > + if ( c != '\n' )
> > + lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
> > + }
> > +
> > + /* Render modified section of text buffer to VESA linear framebuffer. */
> > + for ( i = min_redraw_y; i <= lfb.ypos; i++ )
> > + {
> > + const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
> > + unsigned int width;
> > +
> > + for ( width = lfb.lfbp.text_columns; width; --width )
> > + if ( line[width - 1] )
> > + break;
> > + lfb_show_line(line,
> > + lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
> > + width, max(lfb.line_len[i], width));
> > + lfb.line_len[i] = width;
> > + }
> > +
> > + lfb.lfbp.flush();
> > +}
> > +
> > +/* Slower line-based scroll mode which interacts better with dom0. */
> > +void lfb_scroll_puts(const char *s)
> > +{
> > + unsigned int i;
> > + char c;
> > +
> > + while ( (c = *s++) != '\0' )
> > + {
> > + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> > + {
> > + unsigned int bytes = (lfb.lfbp.width *
> > + ((lfb.lfbp.bits_per_pixel + 7) >> 3));
> > + unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
> > + unsigned char *dst = lfb.lfbp.lfb;
> > +
> > + /* New line: scroll all previous rows up one line. */
> > + for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
> > + {
> > + memcpy(dst, src, bytes);
> > + src += lfb.lfbp.bytes_per_line;
> > + dst += lfb.lfbp.bytes_per_line;
> > + }
> > +
> > + /* Render new line. */
> > + lfb_show_line(
> > + lfb.text_buf,
> > + lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
>
> Long line?

Right


> > + lfb.xpos, lfb.lfbp.text_columns);
> > +
> > + lfb.xpos = 0;
> > + }
> > +
> > + if ( c != '\n' )
> > + lfb.text_buf[lfb.xpos++] = c;
> > + }
> > +
> > + lfb.lfbp.flush();
> > +}
> > +
> > +void lfb_carriage_return(void)
> > +{
> > + lfb.xpos = 0;
> > +}
> > +
> > +int __init lfb_init(struct lfb_prop *lfbp)
> > +{
> > + if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
> > + {
> > + printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer early.\n",
> > + lfbp->width, lfbp->height);
>
> Indentation?

OK


> > + return -EINVAL;
> > + }
> > +
> > + lfb.lfbp = *lfbp;
> > + lfb.lbuf = lbuf;
> > + lfb.text_buf = text_buf;
> > + lfb.line_len = line_len;
> > + return 0;
> > +}
> > +
> > +int __init lfb_alloc(void)
> > +{
> > + lfb.lbuf = NULL;
> > + lfb.text_buf = NULL;
> > + lfb.line_len = NULL;
> > +
> > + lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
> > + if ( !lfb.lbuf )
> > + goto fail;
> > +
> > + lfb.text_buf = xzalloc_bytes(lfb.lfbp.text_columns * lfb.lfbp.text_rows);
> > + if ( !lfb.text_buf )
> > + goto fail;
> > +
> > + lfb.line_len = xzalloc_array(unsigned int, lfb.lfbp.text_columns);
> > + if ( !lfb.line_len )
> > + goto fail;
> > +
> > + memcpy(lfb.lbuf, lbuf, lfb.lfbp.bytes_per_line);
> > + memcpy(lfb.text_buf, text_buf, lfb.lfbp.text_columns * lfb.lfbp.text_rows);
> > + memcpy(lfb.line_len, line_len, lfb.lfbp.text_columns);
> > +
> > + return 0;
> > +
> > +fail:
> > + printk(XENLOG_ERR "Couldn't allocate enough memory to drive the framebuffer\n");
> > + lfb_free();
> > +
> > + return -ENOMEM;
> > +}
> > +
> > +void lfb_free(void)
> > +{
> > + xfree(lfb.lbuf);
> > + xfree(lfb.text_buf);
> > + xfree(lfb.line_len);
> > +}
>
>
> This function would then perhaps better go before the __init stuff.
>
> > @@ -139,15 +131,14 @@ void __init vesa_init(void)
> > else
> > {
> > /* White(ish) in default pseudocolor palette. */
> > - pixel_on = 7;
> > + fbp.pixel_on = 7;
> > }
> >
> > - return;
> > -
> > - fail:
> > - xfree(lbuf);
> > - xfree(text_buf);
> > - xfree(line_len);
> > + if ( lfb_init(&lfbp) < 0 )
> > + return;
> > + if ( lfb_alloc() < 0 )
> > + return;
>
> What's the point of calling lfb_alloc() right after lfb_init()? The static
> buffers set up by lfb_init() will get replaced by dynamically allocated
> ones right away. Or in other words - what do you need the static
> buffers for in the first place?

The idea was that you could have output on the screen before xmalloc
works.

Given that vesa_init is always called after xmalloc is available we call
lfb_alloc() right away. Thanks to the fact that I moved setup_mm()
earlier in the ARM version of start_xen, that should be now possible for the
arm_hdlcd driver too. I'll to remove lfb_alloc.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH v4 3/7] xen: introduce a generic framebuffer driver [ In reply to ]
On 09/01/13 14:05, Stefano Stabellini wrote:
> On Wed, 9 Jan 2013, Jan Beulich wrote:
>>>>> On 08.01.13 at 21:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>>> --- /dev/null
>>> +++ b/xen/drivers/video/lfb.c
>>> @@ -0,0 +1,206 @@
>>> +/******************************************************************************
>>> + * lfb.c
>>> + *
>>> + * linear frame buffer handling.
>>> + */
>>> +
>>> +#include <xen/config.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/errno.h>
>>> +#include "lfb.h"
>>> +#include "font.h"
>>> +
>>> +#define MAX_XRES 1900
>>> +#define MAX_YRES 1200
>>> +#define MAX_BPP 4
>>> +#define MAX_FONT_W 8
>>> +#define MAX_FONT_H 16
>>> +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
>>> +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
>>> +static __initdata unsigned char text_buf[(MAX_XRES / MAX_FONT_W) * \
>>> + (MAX_YRES / MAX_FONT_H)];
>> Imo it would be better to move all these __initdata definitions
>> do to where the using __init functions actually live, to make
>> sure unintended use of them in non-__init ones is noticed.
> OK
>
>
>>> +
>>> +struct lfb_status {
>>> + struct lfb_prop lfbp;
>>> +
>>> + unsigned char *lbuf, *text_buf;
>>> + unsigned int *line_len;
>>> + unsigned int xpos, ypos;
>>> +};
>>> +static struct lfb_status lfb;
>>> +
>>> +static void lfb_show_line(
>>> + const unsigned char *text_line,
>>> + unsigned char *video_line,
>>> + unsigned int nr_chars,
>>> + unsigned int nr_cells)
>>> +{
>>> + unsigned int i, j, b, bpp, pixel;
>>> +
>>> + bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
>>> +
>>> + for ( i = 0; i < lfb.lfbp.font->height; i++ )
>>> + {
>>> + unsigned char *ptr = lfb.lbuf;
>>> +
>>> + for ( j = 0; j < nr_chars; j++ )
>>> + {
>>> + const unsigned char *bits = lfb.lfbp.font->data;
>>> + bits += ((text_line[j] * lfb.lfbp.font->height + i) *
>>> + ((lfb.lfbp.font->width + 7) >> 3));
>>> + for ( b = lfb.lfbp.font->width; b--; )
>>> + {
>>> + pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
>>> + memcpy(ptr, &pixel, bpp);
>>> + ptr += bpp;
>>> + }
>>> + }
>>> +
>>> + memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
>>> + memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
>>> + video_line += lfb.lfbp.bytes_per_line;
>>> + }
>>> +}
>>> +
>>> +/* Fast mode which redraws all modified parts of a 2D text buffer. */
>>> +void lfb_redraw_puts(const char *s)
>>> +{
>>> + unsigned int i, min_redraw_y = lfb.ypos;
>>> + char c;
>>> +
>>> + /* Paste characters into text buffer. */
>>> + while ( (c = *s++) != '\0' )
>>> + {
>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>>> + {
>>> + if ( ++lfb.ypos >= lfb.lfbp.text_rows )
>>> + {
>>> + min_redraw_y = 0;
>>> + lfb.ypos = lfb.lfbp.text_rows - 1;
>>> + memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
>>> + lfb.ypos * lfb.lfbp.text_columns);
>>> + memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
>>> + }
>>> + lfb.xpos = 0;
>>> + }
>>> +
>>> + if ( c != '\n' )
>>> + lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
>>> + }
>>> +
>>> + /* Render modified section of text buffer to VESA linear framebuffer. */
>>> + for ( i = min_redraw_y; i <= lfb.ypos; i++ )
>>> + {
>>> + const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
>>> + unsigned int width;
>>> +
>>> + for ( width = lfb.lfbp.text_columns; width; --width )
>>> + if ( line[width - 1] )
>>> + break;
>>> + lfb_show_line(line,
>>> + lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
>>> + width, max(lfb.line_len[i], width));
>>> + lfb.line_len[i] = width;
>>> + }
>>> +
>>> + lfb.lfbp.flush();
>>> +}
>>> +
>>> +/* Slower line-based scroll mode which interacts better with dom0. */
>>> +void lfb_scroll_puts(const char *s)
>>> +{
>>> + unsigned int i;
>>> + char c;
>>> +
>>> + while ( (c = *s++) != '\0' )
>>> + {
>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>>> + {
>>> + unsigned int bytes = (lfb.lfbp.width *
>>> + ((lfb.lfbp.bits_per_pixel + 7) >> 3));
>>> + unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
>>> + unsigned char *dst = lfb.lfbp.lfb;
>>> +
>>> + /* New line: scroll all previous rows up one line. */
>>> + for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
>>> + {
>>> + memcpy(dst, src, bytes);
>>> + src += lfb.lfbp.bytes_per_line;
>>> + dst += lfb.lfbp.bytes_per_line;
>>> + }
>>> +
>>> + /* Render new line. */
>>> + lfb_show_line(
>>> + lfb.text_buf,
>>> + lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
>> Long line?
> Right
>
The calculation of

lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;

is done twice in the above section - surely that's constant over time, so could be stored in another local variable - thus making the line shorter and at the same time more obvious that "it's the same thing".

--
Mats



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH v4 3/7] xen: introduce a generic framebuffer driver [ In reply to ]
On 09/01/13 14:45, Mats Petersson wrote:
> On 09/01/13 14:05, Stefano Stabellini wrote:
>> On Wed, 9 Jan 2013, Jan Beulich wrote:
>>>>>> On 08.01.13 at 21:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>>>> --- /dev/null
>>>> +++ b/xen/drivers/video/lfb.c
>>>> @@ -0,0 +1,206 @@
>>>> +/******************************************************************************
>>>> + * lfb.c
>>>> + *
>>>> + * linear frame buffer handling.
>>>> + */
>>>> +
>>>> +#include <xen/config.h>
>>>> +#include <xen/kernel.h>
>>>> +#include <xen/lib.h>
>>>> +#include <xen/errno.h>
>>>> +#include "lfb.h"
>>>> +#include "font.h"
>>>> +
>>>> +#define MAX_XRES 1900
>>>> +#define MAX_YRES 1200
>>>> +#define MAX_BPP 4
>>>> +#define MAX_FONT_W 8
>>>> +#define MAX_FONT_H 16
>>>> +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
>>>> +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
>>>> +static __initdata unsigned char text_buf[(MAX_XRES / MAX_FONT_W) * \
>>>> + (MAX_YRES / MAX_FONT_H)];
>>> Imo it would be better to move all these __initdata definitions
>>> do to where the using __init functions actually live, to make
>>> sure unintended use of them in non-__init ones is noticed.
>> OK
>>
>>
>>>> +
>>>> +struct lfb_status {
>>>> + struct lfb_prop lfbp;
>>>> +
>>>> + unsigned char *lbuf, *text_buf;
>>>> + unsigned int *line_len;
>>>> + unsigned int xpos, ypos;
>>>> +};
>>>> +static struct lfb_status lfb;
>>>> +
>>>> +static void lfb_show_line(
>>>> + const unsigned char *text_line,
>>>> + unsigned char *video_line,
>>>> + unsigned int nr_chars,
>>>> + unsigned int nr_cells)
>>>> +{
>>>> + unsigned int i, j, b, bpp, pixel;
>>>> +
>>>> + bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
>>>> +
>>>> + for ( i = 0; i < lfb.lfbp.font->height; i++ )
>>>> + {
>>>> + unsigned char *ptr = lfb.lbuf;
>>>> +
>>>> + for ( j = 0; j < nr_chars; j++ )
>>>> + {
>>>> + const unsigned char *bits = lfb.lfbp.font->data;
>>>> + bits += ((text_line[j] * lfb.lfbp.font->height + i) *
>>>> + ((lfb.lfbp.font->width + 7) >> 3));
>>>> + for ( b = lfb.lfbp.font->width; b--; )
>>>> + {
>>>> + pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
>>>> + memcpy(ptr, &pixel, bpp);
>>>> + ptr += bpp;
>>>> + }
>>>> + }
>>>> +
>>>> + memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
>>>> + memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
>>>> + video_line += lfb.lfbp.bytes_per_line;
>>>> + }
>>>> +}
>>>> +
>>>> +/* Fast mode which redraws all modified parts of a 2D text buffer. */
>>>> +void lfb_redraw_puts(const char *s)
>>>> +{
>>>> + unsigned int i, min_redraw_y = lfb.ypos;
>>>> + char c;
>>>> +
>>>> + /* Paste characters into text buffer. */
>>>> + while ( (c = *s++) != '\0' )
>>>> + {
>>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>>>> + {
>>>> + if ( ++lfb.ypos >= lfb.lfbp.text_rows )
>>>> + {
>>>> + min_redraw_y = 0;
>>>> + lfb.ypos = lfb.lfbp.text_rows - 1;
>>>> + memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
>>>> + lfb.ypos * lfb.lfbp.text_columns);
>>>> + memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
>>>> + }
>>>> + lfb.xpos = 0;
>>>> + }
>>>> +
>>>> + if ( c != '\n' )
>>>> + lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
>>>> + }
>>>> +
>>>> + /* Render modified section of text buffer to VESA linear framebuffer. */
>>>> + for ( i = min_redraw_y; i <= lfb.ypos; i++ )
>>>> + {
>>>> + const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
>>>> + unsigned int width;
>>>> +
>>>> + for ( width = lfb.lfbp.text_columns; width; --width )
>>>> + if ( line[width - 1] )
>>>> + break;
>>>> + lfb_show_line(line,
>>>> + lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
>>>> + width, max(lfb.line_len[i], width));
>>>> + lfb.line_len[i] = width;
>>>> + }
>>>> +
>>>> + lfb.lfbp.flush();
>>>> +}
>>>> +
>>>> +/* Slower line-based scroll mode which interacts better with dom0. */
>>>> +void lfb_scroll_puts(const char *s)
>>>> +{
>>>> + unsigned int i;
>>>> + char c;
>>>> +
>>>> + while ( (c = *s++) != '\0' )
>>>> + {
>>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>>>> + {
>>>> + unsigned int bytes = (lfb.lfbp.width *
>>>> + ((lfb.lfbp.bits_per_pixel + 7) >> 3));
>>>> + unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
>>>> + unsigned char *dst = lfb.lfbp.lfb;
>>>> +
>>>> + /* New line: scroll all previous rows up one line. */
>>>> + for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
>>>> + {
>>>> + memcpy(dst, src, bytes);
>>>> + src += lfb.lfbp.bytes_per_line;
>>>> + dst += lfb.lfbp.bytes_per_line;
>>>> + }
>>>> +
>>>> + /* Render new line. */
>>>> + lfb_show_line(
>>>> + lfb.text_buf,
>>>> + lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
>>> Long line?
>> Right
>>
> The calculation of
>
> lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
>
> is done twice in the above section - surely that's constant over time, so could be stored in another local variable - thus making the line shorter and at the same time more obvious that "it's the same thing".
No, it isn't! I started out with

lfb.lfbp.font->height * lfb.lfbp.bytes_per_line

being calculated twice. Then mistakenly thought that it was MORE of the same, missing out that the second calculation is different. Sorry for that. But the font->hight * bytes_per_line calculation is definitely the same in both places.

--
Mats

>
> --
> Mats
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH v4 3/7] xen: introduce a generic framebuffer driver [ In reply to ]
On Wed, 9 Jan 2013, Mats Petersson wrote:
> On 09/01/13 14:45, Mats Petersson wrote:
> > On 09/01/13 14:05, Stefano Stabellini wrote:
> >> On Wed, 9 Jan 2013, Jan Beulich wrote:
> >>>>>> On 08.01.13 at 21:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> >>>> --- /dev/null
> >>>> +++ b/xen/drivers/video/lfb.c
> >>>> @@ -0,0 +1,206 @@
> >>>> +/******************************************************************************
> >>>> + * lfb.c
> >>>> + *
> >>>> + * linear frame buffer handling.
> >>>> + */
> >>>> +
> >>>> +#include <xen/config.h>
> >>>> +#include <xen/kernel.h>
> >>>> +#include <xen/lib.h>
> >>>> +#include <xen/errno.h>
> >>>> +#include "lfb.h"
> >>>> +#include "font.h"
> >>>> +
> >>>> +#define MAX_XRES 1900
> >>>> +#define MAX_YRES 1200
> >>>> +#define MAX_BPP 4
> >>>> +#define MAX_FONT_W 8
> >>>> +#define MAX_FONT_H 16
> >>>> +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
> >>>> +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
> >>>> +static __initdata unsigned char text_buf[.(MAX_XRES / MAX_FONT_W) * \
> >>>> + (MAX_YRES / MAX_FONT_H)];
> >>> Imo it would be better to move all these __initdata definitions
> >>> do to where the using __init functions actually live, to make
> >>> sure unintended use of them in non-__init ones is noticed.
> >> OK
> >>
> >>
> >>>> +
> >>>> +struct lfb_status {
> >>>> + struct lfb_prop lfbp;
> >>>> +
> >>>> + unsigned char *lbuf, *text_buf;
> >>>> + unsigned int *line_len;
> >>>> + unsigned int xpos, ypos;
> >>>> +};
> >>>> +static struct lfb_status lfb;
> >>>> +
> >>>> +static void lfb_show_line(
> >>>> + const unsigned char *text_line,
> >>>> + unsigned char *video_line,
> >>>> + unsigned int nr_chars,
> >>>> + unsigned int nr_cells)
> >>>> +{
> >>>> + unsigned int i, j, b, bpp, pixel;
> >>>> +
> >>>> + bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
> >>>> +
> >>>> + for ( i = 0; i < lfb.lfbp.font->height; i++ )
> >>>> + {
> >>>> + unsigned char *ptr = lfb.lbuf;
> >>>> +
> >>>> + for ( j = 0; j < nr_chars; j++ )
> >>>> + {
> >>>> + const unsigned char *bits = lfb.lfbp.font->data;
> >>>> + bits += ((text_line[j] * lfb.lfbp.font->height + i) *
> >>>> + ((lfb.lfbp.font->width + 7) >> 3));
> >>>> + for ( b = lfb.lfbp.font->width; b--; )
> >>>> + {
> >>>> + pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
> >>>> + memcpy(ptr, &pixel, bpp);
> >>>> + ptr += bpp;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + memset(ptr, 0, (lfb.lfbp.width - nr_chars * lfb.lfbp.font->width) * bpp);
> >>>> + memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * bpp);
> >>>> + video_line += lfb.lfbp.bytes_per_line;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +/* Fast mode which redraws all modified parts of a 2D text buffer. */
> >>>> +void lfb_redraw_puts(const char *s)
> >>>> +{
> >>>> + unsigned int i, min_redraw_y = lfb.ypos;
> >>>> + char c;
> >>>> +
> >>>> + /* Paste characters into text buffer. */
> >>>> + while ( (c = *s++) != '\0' )
> >>>> + {
> >>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> >>>> + {
> >>>> + if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> >>>> + {
> >>>> + min_redraw_y = 0;
> >>>> + lfb.ypos = lfb.lfbp.text_rows - 1;
> >>>> + memmove(lfb.text_buf, lfb.text_buf + lfb.lfbp.text_columns,
> >>>> + lfb.ypos * lfb.lfbp.text_columns);
> >>>> + memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 0, lfb.xpos);
> >>>> + }
> >>>> + lfb.xpos = 0;
> >>>> + }
> >>>> +
> >>>> + if ( c != '\n' )
> >>>> + lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] = c;
> >>>> + }
> >>>> +
> >>>> + /* Render modified section of text buffer to VESA linear framebuffer. */
> >>>> + for ( i = min_redraw_y; i <= lfb.ypos; i++ )
> >>>> + {
> >>>> + const unsigned char *line = lfb.text_buf + i * lfb.lfbp.text_columns;
> >>>> + unsigned int width;
> >>>> +
> >>>> + for ( width = lfb.lfbp.text_columns; width; --width )
> >>>> + if ( line[width - 1] )
> >>>> + break;
> >>>> + lfb_show_line(line,
> >>>> + lfb.lfbp.lfb + i * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
> >>>> + width, max(lfb.line_len[i], width));
> >>>> + lfb.line_len[i] = width;
> >>>> + }
> >>>> +
> >>>> + lfb.lfbp.flush();
> >>>> +}
> >>>> +
> >>>> +/* Slower line-based scroll mode which interacts better with dom0. */
> >>>> +void lfb_scroll_puts(const char *s)
> >>>> +{
> >>>> + unsigned int i;
> >>>> + char c;
> >>>> +
> >>>> + while ( (c = *s++) != '\0' )
> >>>> + {
> >>>> + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> >>>> + {
> >>>> + unsigned int bytes = (lfb.lfbp.width *
> >>>> + ((lfb.lfbp.bits_per_pixel + 7) >> 3));
> >>>> + unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
> >>>> + unsigned char *dst = lfb.lfbp.lfb;
> >>>> +
> >>>> + /* New line: scroll all previous rows up one line. */
> >>>> + for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
> >>>> + {
> >>>> + memcpy(dst, src, bytes);
> >>>> + src += lfb.lfbp.bytes_per_line;
> >>>> + dst += lfb.lfbp.bytes_per_line;
> >>>> + }
> >>>> +
> >>>> + /* Render new line. */
> >>>> + lfb_show_line(
> >>>> + lfb.text_buf,
> >>>> + lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
> >>> Long line?
> >> Right
> >>
> > The calculation of
> >
> > lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
> >
> > is done twice in the above section - surely that's constant over time, so could be stored in another local variable - thus making the line shorter and at the same time more obvious that "it's the same thing".
> No, it isn't! I started out with
>
> lfb.lfbp.font->height * lfb.lfbp.bytes_per_line
>
> being calculated twice. Then mistakenly thought that it was MORE of the same, missing out that the second calculation is different. Sorry for that. But the font->hight * bytes_per_line calculation is definitely the same in both places.

Thanks for the review!

lfb_scroll_puts and lfb_show_line are just vesa_scroll_puts and
vesa_show_line renamed and with some basic variable name changes.
I would be reticent to change them in this patch unless necessary.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel