Re: [Qemu-devel] [PATCH v4 04/11] hw/m68k: add macfb video card
On 23/10/2018 08:13, Thomas Huth wrote:
> On 2018-10-18 19:28, Mark Cave-Ayland wrote:
>> From: Laurent Vivier
>>
>> Co-developed-by: Mark Cave-Ayland
>> Signed-off-by: Mark Cave-Ayland
>> Signed-off-by: Laurent Vivier
>> ---
>> arch_init.c | 4 +
>> hw/display/Makefile.objs| 1 +
>> hw/display/macfb-template.h | 158 +++
>> hw/display/macfb.c | 252
>>
>> include/hw/display/macfb.h | 42
>> qemu-options.hx | 2 +-
>> vl.c| 3 +-
>> 7 files changed, 460 insertions(+), 2 deletions(-)
>> create mode 100644 hw/display/macfb-template.h
>> create mode 100644 hw/display/macfb.c
>> create mode 100644 include/hw/display/macfb.h
> [...]
>> +typedef void (*macfb_draw_line_func_t)(MacfbState *, uint8_t *, uint8_t *,
>> int);
>> +
>> +static macfb_draw_line_func_t macfb_draw_line[24][32] = {
>> +[0] = { [7] = draw_line1_8, [15] = draw_line1_16,
>> +[23] = draw_line1_24, [31] = draw_line1_32 },
>> +[1] = { [7] = draw_line2_8, [15] = draw_line2_16,
>> +[23] = draw_line2_24, [31] = draw_line2_32 },
>> +[3] = { [7] = draw_line4_8, [15] = draw_line4_16,
>> +[23] = draw_line4_24, [31] = draw_line4_32 },
>> +[7] = { [7] = draw_line8_8, [15] = draw_line8_16,
>> +[23] = draw_line8_24, [31] = draw_line8_32 },
>> +[15] = { [7] = draw_line16_8, [15] = draw_line16_16,
>> + [23] = draw_line16_24, [31] = draw_line16_32 },
>> +[23] = { [7] = draw_line24_8, [15] = draw_line24_16,
>> + [23] = draw_line24_24, [31] = draw_line24_32 },
>> +};
>
> May I suggest to define the array as macfb_draw_line[24][4] instead of
> macfb_draw_line[24][32] here...
Indeed, this is an artefact from the original implementation that can now be
removed
given that all host surfaces are now 32-bit.
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> new file mode 100644
>> index 00..54472c1cbb
>> --- /dev/null
>> +++ b/hw/display/macfb.c
>> @@ -0,0 +1,252 @@
>> +/*
>> + * QEMU Motorola 680x0 Macintosh Video Card Emulation
>> + * Copyright (c) 2012-2018 Laurent Vivier
>> + *
>> + * some parts from QEMU G364 framebuffer Emulator.
>> + * Copyright (c) 2007-2011 Herve Poussineau
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/sysbus.h"
>> +#include "ui/console.h"
>> +#include "ui/pixel_ops.h"
>> +#include "hw/display/macfb.h"
>> +
>> +#define VIDEO_BASE 0x1000
>> +#define DAFB_BASE 0x0080
>> +
>> +#define MACFB_PAGE_SIZE 4096
>> +#define MACFB_VRAM_SIZE (4 * 1024 * 1024)
>> +
>> +#define DAFB_RESET 0x200
>> +#define DAFB_LUT0x213
>> +
>> +#include "macfb-template.h"
>> +
>> +static int macfb_check_dirty(MacfbState *s, DirtyBitmapSnapshot *snap,
>> + ram_addr_t addr, int len)
>> +{
>> +return memory_region_snapshot_get_dirty(>mem_vram, snap, addr, len);
>> +}
>> +
>> +static void macfb_draw_graphic(MacfbState *s)
>> +{
>> +DisplaySurface *surface = qemu_console_surface(s->con);
>> +DirtyBitmapSnapshot *snap = NULL;
>> +ram_addr_t page;
>> +int y, ymin;
>> +int macfb_stride = (s->depth * s->width + 7) / 8;
>> +macfb_draw_line_func_t draw_line;
>> +
>> +if (s->depth > 24) {
>> +hw_error("macfb: unknown guest depth %d", s->depth);
>
> I think this should be checked in the realize() function instead (and
> maybe only do an assert() here).
Yes indeed - fixed in the updated version.
>> +return;
>> +}
>> +if (surface_bits_per_pixel(surface) > 32) {
>> +hw_error("macfb: unknown host depth %d",
>> + surface_bits_per_pixel(surface));
>
> Simply assert(surface_bits_per_pixel(surface) <= 32) ?
I had a look at a few other display implementations and it seems the easiest
thing to
do here is move this to realize similar to above.
>> +return;
>> +}
>> +draw_line = macfb_draw_line[s->depth -
>> 1][surface_bits_per_pixel(surface)
>> + - 1];
>
> ... If you change func_t macfb_draw_line[24][32] to func_t
> macfb_draw_line[24][4], you could then use
> surface_bits_per_pixel(surface) / 8 - 1 here instead.
>
> Also, do we have still to care about host bit depths < 32 at all these
> days? If not, I think the code could be simplified quite a bit.
Yes indeed. In fact, looking at the current VGA code the use of the template
has now
been removed so I've expanded out the remaining functions within macfb.c and
removed
macfb-template.h which is no longer required.
>> +if (draw_line == NULL) {
>> +hw_error("macfb: unknown guest/host depth combination %d/%d",
>> s->depth,
>> +
Re: [Qemu-devel] [PATCH v4 04/11] hw/m68k: add macfb video card
On 2018-10-18 19:28, Mark Cave-Ayland wrote:
> From: Laurent Vivier
>
> Co-developed-by: Mark Cave-Ayland
> Signed-off-by: Mark Cave-Ayland
> Signed-off-by: Laurent Vivier
> ---
> arch_init.c | 4 +
> hw/display/Makefile.objs| 1 +
> hw/display/macfb-template.h | 158 +++
> hw/display/macfb.c | 252
>
> include/hw/display/macfb.h | 42
> qemu-options.hx | 2 +-
> vl.c| 3 +-
> 7 files changed, 460 insertions(+), 2 deletions(-)
> create mode 100644 hw/display/macfb-template.h
> create mode 100644 hw/display/macfb.c
> create mode 100644 include/hw/display/macfb.h
[...]
> +typedef void (*macfb_draw_line_func_t)(MacfbState *, uint8_t *, uint8_t *,
> int);
> +
> +static macfb_draw_line_func_t macfb_draw_line[24][32] = {
> +[0] = { [7] = draw_line1_8, [15] = draw_line1_16,
> +[23] = draw_line1_24, [31] = draw_line1_32 },
> +[1] = { [7] = draw_line2_8, [15] = draw_line2_16,
> +[23] = draw_line2_24, [31] = draw_line2_32 },
> +[3] = { [7] = draw_line4_8, [15] = draw_line4_16,
> +[23] = draw_line4_24, [31] = draw_line4_32 },
> +[7] = { [7] = draw_line8_8, [15] = draw_line8_16,
> +[23] = draw_line8_24, [31] = draw_line8_32 },
> +[15] = { [7] = draw_line16_8, [15] = draw_line16_16,
> + [23] = draw_line16_24, [31] = draw_line16_32 },
> +[23] = { [7] = draw_line24_8, [15] = draw_line24_16,
> + [23] = draw_line24_24, [31] = draw_line24_32 },
> +};
May I suggest to define the array as macfb_draw_line[24][4] instead of
macfb_draw_line[24][32] here...
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> new file mode 100644
> index 00..54472c1cbb
> --- /dev/null
> +++ b/hw/display/macfb.c
> @@ -0,0 +1,252 @@
> +/*
> + * QEMU Motorola 680x0 Macintosh Video Card Emulation
> + * Copyright (c) 2012-2018 Laurent Vivier
> + *
> + * some parts from QEMU G364 framebuffer Emulator.
> + * Copyright (c) 2007-2011 Herve Poussineau
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "ui/console.h"
> +#include "ui/pixel_ops.h"
> +#include "hw/display/macfb.h"
> +
> +#define VIDEO_BASE 0x1000
> +#define DAFB_BASE 0x0080
> +
> +#define MACFB_PAGE_SIZE 4096
> +#define MACFB_VRAM_SIZE (4 * 1024 * 1024)
> +
> +#define DAFB_RESET 0x200
> +#define DAFB_LUT0x213
> +
> +#include "macfb-template.h"
> +
> +static int macfb_check_dirty(MacfbState *s, DirtyBitmapSnapshot *snap,
> + ram_addr_t addr, int len)
> +{
> +return memory_region_snapshot_get_dirty(>mem_vram, snap, addr, len);
> +}
> +
> +static void macfb_draw_graphic(MacfbState *s)
> +{
> +DisplaySurface *surface = qemu_console_surface(s->con);
> +DirtyBitmapSnapshot *snap = NULL;
> +ram_addr_t page;
> +int y, ymin;
> +int macfb_stride = (s->depth * s->width + 7) / 8;
> +macfb_draw_line_func_t draw_line;
> +
> +if (s->depth > 24) {
> +hw_error("macfb: unknown guest depth %d", s->depth);
I think this should be checked in the realize() function instead (and
maybe only do an assert() here).
> +return;
> +}
> +if (surface_bits_per_pixel(surface) > 32) {
> +hw_error("macfb: unknown host depth %d",
> + surface_bits_per_pixel(surface));
Simply assert(surface_bits_per_pixel(surface) <= 32) ?
> +return;
> +}
> +draw_line = macfb_draw_line[s->depth - 1][surface_bits_per_pixel(surface)
> + - 1];
... If you change func_t macfb_draw_line[24][32] to func_t
macfb_draw_line[24][4], you could then use
surface_bits_per_pixel(surface) / 8 - 1 here instead.
Also, do we have still to care about host bit depths < 32 at all these
days? If not, I think the code could be simplified quite a bit.
> +if (draw_line == NULL) {
> +hw_error("macfb: unknown guest/host depth combination %d/%d",
> s->depth,
> + surface_bits_per_pixel(surface));
hw_error() is meant for CPU errors only (it prints out a CPU register
dump), please don't use it in the framebuffer code.
> +return;
> +}
Thomas
[Qemu-devel] [PATCH v4 04/11] hw/m68k: add macfb video card
From: Laurent Vivier
Co-developed-by: Mark Cave-Ayland
Signed-off-by: Mark Cave-Ayland
Signed-off-by: Laurent Vivier
---
arch_init.c | 4 +
hw/display/Makefile.objs| 1 +
hw/display/macfb-template.h | 158 +++
hw/display/macfb.c | 252
include/hw/display/macfb.h | 42
qemu-options.hx | 2 +-
vl.c| 3 +-
7 files changed, 460 insertions(+), 2 deletions(-)
create mode 100644 hw/display/macfb-template.h
create mode 100644 hw/display/macfb.c
create mode 100644 include/hw/display/macfb.h
diff --git a/arch_init.c b/arch_init.c
index f4f3f610c8..5a71b48dc5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -39,6 +39,10 @@
int graphic_width = 1024;
int graphic_height = 768;
int graphic_depth = 8;
+#elif defined(TARGET_M68K)
+int graphic_width = 800;
+int graphic_height = 600;
+int graphic_depth = 8;
#else
int graphic_width = 800;
int graphic_height = 600;
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 97acd5b6cb..1685492ea0 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -27,6 +27,7 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_fimd.o
common-obj-$(CONFIG_FRAMEBUFFER) += framebuffer.o
common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
+common-obj-$(CONFIG_MACFB) += macfb.o
common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
milkymist-tmu2.o-cflags := $(X11_CFLAGS)
diff --git a/hw/display/macfb-template.h b/hw/display/macfb-template.h
new file mode 100644
index 00..b6ae5d728f
--- /dev/null
+++ b/hw/display/macfb-template.h
@@ -0,0 +1,158 @@
+#if defined(READ_BITS)
+#define PALETTE(i, r, g, b)\
+do { \
+r = s->color_palette[i * 3]; \
+g = s->color_palette[i * 3 + 1]; \
+b = s->color_palette[i * 3 + 2]; \
+} while (0)
+
+#if READ_BITS == 1
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+int bit = x & 7; \
+int idx = (*from >> (7 - bit)) & 1;\
+r = g = b = ((1 - idx) << 7); \
+from += (bit == 7);\
+} while (0)
+#elif READ_BITS == 2
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+int bit = (x & 3); \
+int idx = (*from >> ((3 - bit) << 1)) & 3; \
+PALETTE(idx, r, g, b); \
+from += (bit == 3);\
+} while (0)
+#elif READ_BITS == 4
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+int bit = x & 1; \
+int idx = (*from >> ((1 - bit) << 2)) & 15; \
+PALETTE(idx, r, g, b); \
+from += (bit == 1);\
+} while (0)
+#elif READ_BITS == 8
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+PALETTE(*from, r, g, b); \
+from++;\
+} while (0)
+#elif READ_BITS == 16
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+uint16_t pixel;\
+pixel = (from[0] << 8) | from[1]; \
+r = ((pixel >> 10) & 0x1f) << 3; \
+g = ((pixel >> 5) & 0x1f) << 3;\
+b = (pixel & 0x1f) << 3; \
+from += 2; \
+} while (0)
+#elif READ_BITS == 24
+#define READ_PIXEL(from, x, r, g, b) \
+do { \
+r = *from++; \
+g = *from++; \
+b = *from++; \
+} while (0)
+#else
+#error unknown bit depth
+#endif
+
+#if WRITE_BITS == 8
+#define WRITE_PIXEL(to, r, g, b) \
+do { \
+*to = rgb_to_pixel8(r, g, b); \
+to += 1; \
+} while (0)
+#elif WRITE_BITS == 15
+#define WRITE_PIXEL(to, r, g, b) \
+do { \
+*(uint16_t *)to = rgb_to_pixel15(r, g, b); \
+to += 2; \
+} while (0)
+#elif WRITE_BITS == 16
+#define WRITE_PIXEL(to, r, g, b) \
+do { \
+*(uint16_t *)to = rgb_to_pixel16(r, g, b); \
+to += 2; \
+} while (0)
+#elif WRITE_BITS ==
