Re: [PATCH v3 5/6] hppa: Add emulation of Artist graphics

[Philippe Mathieu-Daudé]

Hi Sven,

On 10/22/19 10:59 PM, Sven Schnelle wrote:
> This adds emulation of Artist graphics good enough
> to get a Text console on both Linux and HP-UX. The
> X11 server from HP-UX also works.
>
> Signed-off-by: Sven Schnelle <address@hidden>
> ---
>   hw/display/Kconfig       |    3 +
>   hw/display/Makefile.objs |    1 +
>   hw/display/artist.c      | 1336 ++++++++++++++++++++++++++++++++++++++
>   hw/display/trace-events  |    9 +
>   hw/hppa/Kconfig          |    1 +
>   hw/hppa/hppa_hardware.h  |    1 +
>   hw/hppa/machine.c        |   10 +
>   7 files changed, 1361 insertions(+)
>   create mode 100644 hw/display/artist.c
[...]
> +static void fill_window(ARTISTState *s, int startx, int starty,
> +                        int width, int height)
> +{
> +    uint32_t offset;
> +    uint8_t color = artist_get_color(s);
> +    uint8_t *buf;
> +    int x, y;
> +
> +    trace_artist_fill_window(startx, starty, width, height,
> +        s->image_bitmap_op, s->control_plane);
> +
> +    if (s->control_plane != 0) {
> +        qemu_log_mask(LOG_UNIMP, "%s: CONTROL_PLANE: %08x\n", __func__,
> +            s->control_plane);
> +        return;
> +    }
> +
> +    if (s->reg_100080 == 0x7d) {

What is checked here? Can you add a comment about it?

> +        height = artist_get_y(s->blockmove_size);
> +        s->vram_start += height;
> +    }
> +
> +    buf = s->vram_buffer[ARTIST_BUFFER_AP].data;
> +
> +    for (y = starty; y < starty + height; y++) {
> +        offset = y * s->width;
> +
> +        for (x = startx; x < startx + width; x++) {
> +            artist_rop8(s, buf + offset + x, color);
> +        }
> +    }
> +}
> +
[...]
> +static void artist_initfn(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    ARTISTState *s = ARTIST(obj);
> +
> +    memory_region_init_io(&s->reg, obj, &artist_reg_ops, s, "artist.reg",
> +            0x400000);

Easier to read as: 4 * MiB

> +    memory_region_init_io(&s->vram_mem, obj, &artist_vram_ops, s, 
> "artist.vram",
> +            0x800000);

And 8 * MiB.

> +    sysbus_init_mmio(sbd, &s->reg);
> +    sysbus_init_mmio(sbd, &s->vram_mem);
> +}
> +
> +static void artist_set_buffer(ARTISTState *s, uint8_t **vram, unsigned int idx,
> +                              int width, int height)
> +{
> +    struct vram_buffer *buf = s->vram_buffer + idx;
> +
> +    buf->data = *vram;
> +    buf->size = height * width;
> +    buf->width = width;
> +    buf->height = height;
> +    *vram = *vram + buf->size;
> +}
> +
> +static void artist_realizefn(DeviceState *dev, Error **errp)
> +{
> +    uint8_t *vram;
> +
> +    ARTISTState *s = ARTIST(dev);
> +
> +    vram = g_malloc0(4 * 1048576);

Here you can simply use g_malloc(4 * MiB). If you really need
to bzero the VRAM, that should be done in the reset() handler.

> +    s->vram = vram;
> +    artist_set_buffer(s, &vram, ARTIST_BUFFER_CMAP, 2048, 4);
> +    artist_set_buffer(s, &vram, ARTIST_BUFFER_AP, s->width, s->height);
> +    artist_set_buffer(s, &vram, ARTIST_BUFFER_CURSOR1, 64, 64);
> +    artist_set_buffer(s, &vram, ARTIST_BUFFER_CURSOR2, 64, 64);
> +    artist_set_buffer(s, &vram, ARTIST_BUFFER_ATTRIBUTE, 64, 64);

Shouldn't this be done by firmware code? If no firmware, this seems to
belong to reset() too, isn't it?

> +
> +    /*
> +     * no idea whether the cursor is fixed size or not, so assume 32x32 which
> +     * seems sufficient for HP-UX X11.
> +     */
> +    s->cursor_height = 32;
> +    s->cursor_width = 32;
> +
> +    s->con = graphic_console_init(DEVICE(dev), 0, &artist_ops, s);
> +    qemu_console_resize(s->con, s->width, s->height);
> +}
[...]