Re: [Qemu-block] [Qemu-devel] [PATCH v4 04/11] hw/m68k: add macfb video card

[Thomas Huth]

On 2018-10-18 19:28, Mark Cave-Ayland wrote:
> From: Laurent Vivier <address@hidden>
> 
> Co-developed-by: Mark Cave-Ayland <address@hidden>
> Signed-off-by: Mark Cave-Ayland <address@hidden>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  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 0000000000..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 0x00001000
> +#define DAFB_BASE  0x00800000
> +
> +#define MACFB_PAGE_SIZE 4096
> +#define MACFB_VRAM_SIZE (4 * 1024 * 1024)
> +
> +#define DAFB_RESET      0x200
> +#define DAFB_LUT        0x213
> +
> +#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(&s->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