Re: [Qemu-devel] [1/6] Add g364 framebuffer device

[Aurelien Jarno]

See my comments inline.

On Mon, Mar 03, 2008 at 11:43:28AM +0100, Hervé Poussineau wrote:
> Attached patch adds emulation for the G364 framebuffer

> Index: hw/g364fb.c
> ===================================================================
> --- hw/g364fb.c       Thu Jan  1 00:00:00 1970
> +++ hw/g364fb.c       Mon Mar  3 07:54:47 2008
> @@ -0,0 +1,408 @@
> +/*
> + * QEMU G364 framebuffer Emulator.
> + *
> + * Copyright (c) 2007-2008 Herv? Poussineau
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */

Most of the hardware emulation parts are distributed in a BSD like
license. I am not forcing you to choose this other license, I am just
wanting to make sure you've been aware of that.

> +#include "hw.h"
> +#include "console.h"
> +#include "pixel_ops.h"
> +
> +//#define DEBUG_G364
> +
> +typedef struct G364State {
> +    uint8_t *vram_ptr;
> +    target_phys_addr_t vram_base;
> +    int vram_size;
unsigned int please.

> +    uint8_t *vram_buffer;
> +    target_phys_addr_t ctrl_base;
> +    uint32_t ctla;
> +    uint8_t palette[256][3];
> +    /* display refresh support */
> +    DisplayState *ds;
> +    int graphic_mode;
> +    uint32_t scr_width, scr_height; /* in pixels */
> +    uint32_t last_scr_width, last_scr_height; /* in pixels */
> +} G364State;
> +
> +/*
> + * graphic modes
> + */
> +#define BPP 8
> +#define PIXEL_WIDTH 8
> +#include "g364fb_template.h"
> +#undef BPP
> +#undef PIXEL_WIDTH
> +
> +#define BPP 15
> +#define PIXEL_WIDTH 16
> +#include "g364fb_template.h"
> +#undef BPP
> +#undef PIXEL_WIDTH
> +
> +#define BPP 16
> +#define PIXEL_WIDTH 16
> +#include "g364fb_template.h"
> +#undef BPP
> +#undef PIXEL_WIDTH
> +
> +#define BPP 32
> +#define PIXEL_WIDTH 32
> +#include "g364fb_template.h"
> +#undef BPP
> +#undef PIXEL_WIDTH
> +
> +static void g364fb_draw_graphic(G364State *s, int full_update)
> +{
> +    if (s->scr_width <= 0 || s->scr_height <= 0)
> +        return;

scr_width and scr_height are unsigned number, so this test actually
check if those number are null or not.

> +    if (s->scr_width != s->last_scr_width
> +     || s->scr_height != s->last_scr_height) {
> +        s->last_scr_width = s->scr_width;
> +        s->last_scr_height = s->scr_height;
> +        dpy_resize(s->ds, s->last_scr_width, s->last_scr_height);
> +        full_update = 1;
> +    }
> +
> +    switch (s->ds->depth) {
> +        case 8:
> +            g364fb_draw_graphic8(s, full_update);
> +            break;
> +        case 15:
> +            g364fb_draw_graphic15(s, full_update);
> +            break;
> +        case 16:
> +            g364fb_draw_graphic16(s, full_update);
> +            break;
> +        case 32:
> +            g364fb_draw_graphic32(s, full_update);
> +            break;
> +        default:
> +            printf("g364fb: unknown depth %d\n", s->ds->depth);
> +            return;
> +    }
> +
> +    dpy_update(s->ds, 0, 0, s->last_scr_width, s->last_scr_height);
> +}
> +
> +static void g364fb_draw_blank(G364State *s, int full_update)
> +{
> +    int i, w;
> +    uint8_t *d;
> +
> +    if (!full_update)
> +        return;
> +    if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
> +        return;
> +
> +    w = s->last_scr_width * ((s->ds->depth + 7) >> 3);
> +    d = s->ds->data;
> +    for(i = 0; i < s->last_scr_height; i++) {
> +        memset(d, 0, w);
> +        d += s->ds->linesize;
> +    }
> +    dpy_update(s->ds, 0, 0,
> +               s->last_scr_width, s->last_scr_height);
> +}
> +
> +#define GMODE_GRAPH 0
> +#define GMODE_BLANK 1
> +
> +static void g364fb_update_display(void *opaque)
> +{
> +    G364State *s = opaque;
> +    int full_update, graphic_mode;
> +
> +    if (s->ctla & 0x400)

Please use a #define instead of hardcoding the value.

> +        graphic_mode = GMODE_BLANK;
> +    else
> +        graphic_mode = GMODE_GRAPH;
> +    full_update = 0;
> +    if (graphic_mode != s->graphic_mode) {
> +        s->graphic_mode = graphic_mode;
> +        full_update = 1;
> +    }
> +    switch(graphic_mode) {
> +        case GMODE_GRAPH:
> +            g364fb_draw_graphic(s, full_update);
> +            break;
> +        case GMODE_BLANK:
> +        default:
> +            g364fb_draw_blank(s, full_update);
> +            break;
> +    }
> +}
> +
> +/* force a full display refresh */
> +static void g364fb_invalidate_display(void *opaque)
> +{
> +    G364State *s = opaque;
> +    s->graphic_mode = -1; /* force full update */
> +}
> +
> +static void g364fb_reset(void *opaque)
> +{
> +    G364State *s = opaque;
> +
> +    memset(s->palette, 0, sizeof(s->palette));
> +    s->scr_width = s->scr_height = 0;
> +    s->last_scr_width = s->last_scr_height = 0;
> +    memset(s->vram_buffer, 0, s->vram_size);
> +    s->graphic_mode = -1; /* force full update */
> +}
> +
> +static void g364fb_screen_dump(void *opaque, const char *filename)
> +{
> +    G364State *s = opaque;
> +    int y, x;
> +    uint8_t index;
> +    uint8_t *data_buffer;
> +    FILE *f;
> +
> +    f = fopen(filename, "wb");
> +    if (!f)
> +        return;
> +
> +    data_buffer = s->vram_buffer;
> +    fprintf(f, "P6\n%d %d\n%d\n",
> +        s->scr_width, s->scr_height, 255);
> +    for(y = 0; y < s->scr_height; y++)
> +        for(x = 0; x < s->scr_width; x++, data_buffer++) {
> +            index = *data_buffer;
> +
> +            if (index >= 16) {
> +                /* XXX: what to do? */
> +                fputc(0, f);
> +                fputc(0, f);
> +                fputc(0, f);
> +                continue;
> +            }
> +
> +            fputc(s->palette[index][0], f);
> +            fputc(s->palette[index][1], f);
> +            fputc(s->palette[index][2], f);
> +        }
> +    fclose(f);
> +}
> +
> +static void g364fb_text_update(void *opaque, console_ch_t *chardata)
> +{
> +    /* XXX: g364fb has no text mode, so what to do? */

You should pass a NULL pointer to graphic_console_init() instead.

> +}
> +
> +/* called for accesses to io ports */
> +static uint32_t g364fb_ctrl_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    G364State *s = opaque;
> +    uint32_t relative_addr = addr - s->ctrl_base;

Use addr &= 0xffff; instead. This will save a temporary register, and
avoid problems if sizeof(target_phys_addr_t) != sizeof(uint32_t);

> +    uint32_t val;
> +
> +    switch (relative_addr) {
> +        default:
> +#ifdef DEBUG_G364
> +            printf("g364fb/ctrl: invalid read at [0x%x]\n", relative_addr);
> +#endif
> +            val = 0;
> +            break;
> +    }
> +
> +#ifdef DEBUG_G364
> +    printf("g364fb/ctrl: read 0x%02x at [0x%x]\n", val, relative_addr);
> +#endif
> +
> +    return val;
> +}
> +
> +static uint32_t g364fb_ctrl_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t v;
> +    v = g364fb_ctrl_readb(opaque, addr);
> +    v |= g364fb_ctrl_readb(opaque, addr + 1) << 8;
> +    return v;
> +}
> +
> +static uint32_t g364fb_ctrl_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t v;
> +    v = g364fb_ctrl_readb(opaque, addr);
> +    v |= g364fb_ctrl_readb(opaque, addr + 1) << 8;
> +    v |= g364fb_ctrl_readb(opaque, addr + 2) << 16;
> +    v |= g364fb_ctrl_readb(opaque, addr + 3) << 24;
> +    return v;
> +}
> +
> +static void g364fb_ctrl_writeb(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    G364State *s = opaque;
> +    uint32_t relative_addr = addr - s->ctrl_base;

Same here.

> +#ifdef DEBUG_G364
> +    printf("g364fb/ctrl: write 0x%02x at [0x%x]\n", val, relative_addr);
> +#endif
> +
> +    if (relative_addr < 0x0800) {
> +        /* color palette */
> +        int idx = relative_addr >> 3;
> +        int c = relative_addr & 7;
> +        if (c < 3)
> +            s->palette[idx][c] = (uint8_t)val;
> +        return;

This return does nothing.

> +    } else {
> +        switch (relative_addr) {
> +            case 0x0918:

Please use a #define instead of hardcoding the value.

> +                s->scr_width = (s->scr_width & 0xfffffc03) | (val << 2);
> +                break;
> +            case 0x0919:
> +                s->scr_width = (s->scr_width & 0xfffc03ff) | (val << 10);
> +                break;
> +            case 0x0940:
> +                s->scr_height = (s->scr_height & 0xffffff80) | (val >> 1);
> +                break;
> +            case 0x0941:
> +                s->scr_height = (s->scr_height & 0xffff801f) | (val << 7);
> +                break;
> +            default:
> +#ifdef DEBUG_G364
> +                printf("g364fb/ctrl: invalid write of 0x%02x at [0x%x]\n", 
> val, relative_addr);
> +#endif
> +                break;
> +        }
> +    }
> +}
> +
> +static void g364fb_ctrl_writew(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    g364fb_ctrl_writeb(opaque, addr, val & 0xff);
> +    g364fb_ctrl_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> +}
> +
> +static void g364fb_ctrl_writel(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    g364fb_ctrl_writeb(opaque, addr, val & 0xff);
> +    g364fb_ctrl_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> +    g364fb_ctrl_writeb(opaque, addr + 2, (val >> 16) & 0xff);
> +    g364fb_ctrl_writeb(opaque, addr + 3, (val >> 24) & 0xff);
> +}
> +
> +static CPUReadMemoryFunc *g364fb_ctrl_read[3] = {
> +    g364fb_ctrl_readb,
> +    g364fb_ctrl_readw,
> +    g364fb_ctrl_readl,
> +};
> +
> +static CPUWriteMemoryFunc *g364fb_ctrl_write[3] = {
> +    g364fb_ctrl_writeb,
> +    g364fb_ctrl_writew,
> +    g364fb_ctrl_writel,
> +};
> +
> +/* called for accesses to video ram */
> +static uint32_t g364fb_mem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    G364State *s = opaque;
> +    uint32_t relative_addr = addr - s->vram_base;

Same as before.

> +    return s->vram_buffer[relative_addr];
> +}
> +
> +static uint32_t g364fb_mem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t v;
> +    v = g364fb_mem_readb(opaque, addr);
> +    v |= g364fb_mem_readb(opaque, addr + 1) << 8;
> +    return v;
> +}
> +
> +static uint32_t g364fb_mem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t v;
> +    v = g364fb_mem_readb(opaque, addr);
> +    v |= g364fb_mem_readb(opaque, addr + 1) << 8;
> +    v |= g364fb_mem_readb(opaque, addr + 2) << 16;
> +    v |= g364fb_mem_readb(opaque, addr + 3) << 24;
> +    return v;
> +}
> +
> +static void g364fb_mem_writeb(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    G364State *s = opaque;
> +    uint32_t relative_addr = addr - s->vram_base;

Same as before.

> +
> +    s->vram_buffer[relative_addr] = val;
> +}
> +
> +static void g364fb_mem_writew(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    g364fb_mem_writeb(opaque, addr, val & 0xff);
> +    g364fb_mem_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> +}
> +
> +static void g364fb_mem_writel(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +{
> +    g364fb_mem_writeb(opaque, addr, val & 0xff);
> +    g364fb_mem_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> +    g364fb_mem_writeb(opaque, addr + 2, (val >> 16) & 0xff);
> +    g364fb_mem_writeb(opaque, addr + 3, (val >> 24) & 0xff);
> +}
> +
> +static CPUReadMemoryFunc *g364fb_mem_read[3] = {
> +    g364fb_mem_readb,
> +    g364fb_mem_readw,
> +    g364fb_mem_readl,
> +};
> +
> +static CPUWriteMemoryFunc *g364fb_mem_write[3] = {
> +    g364fb_mem_writeb,
> +    g364fb_mem_writew,
> +    g364fb_mem_writel,
> +};
> +
> +int g364fb_mm_init(DisplayState *ds, uint8_t *vga_vram_base,
> +                   int vram_size, int it_shift,
> +                   target_phys_addr_t vram_base, target_phys_addr_t 
> ctrl_base)
> +{
> +    G364State *s;
> +    int io_vram, io_ctrl;
> +
> +    s = qemu_mallocz(sizeof(G364State));
> +    if (!s)
> +        return -1;
> +
> +    s->vram_size = vram_size;
> +    s->vram_buffer = qemu_mallocz(s->vram_size);
> +
> +    qemu_register_reset(g364fb_reset, s);
> +    g364fb_reset(s);
> +
> +    s->ds = ds;
> +    s->vram_ptr = vga_vram_base;
> +    s->vram_base = vram_base;
> +    s->ctrl_base = ctrl_base;
> +
> +    graphic_console_init(ds, g364fb_update_display,
> +                         g364fb_invalidate_display, g364fb_screen_dump,
> +                         g364fb_text_update, s);
> +
> +    io_vram = cpu_register_io_memory(0, g364fb_mem_read, g364fb_mem_write, 
> s);
> +    cpu_register_physical_memory(s->vram_base, vram_size, io_vram);
> +
> +    io_ctrl = cpu_register_io_memory(0, g364fb_ctrl_read, g364fb_ctrl_write, 
> s);
> +    cpu_register_physical_memory(s->ctrl_base, 0x10000, io_ctrl);
> +
> +    return 0;
> +}
> Index: hw/g364fb_template.h
> ===================================================================
> --- hw/g364fb_template.h      Thu Jan  1 00:00:00 1970
> +++ hw/g364fb_template.h      Fri Jan 25 22:00:42 2008
> @@ -0,0 +1,55 @@
> +/*
> + * QEMU G364 framebuffer Emulator.
> + *
> + * Copyright (c) 2007 Herv? Poussineau
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */

Same as in the previous file.

> +#define CONCAT2(x, y)            x ## y
> +#define CONCAT3(x, y, z)         x ## y ## z

You may probably want to use glue instead which is defined in osdep.h

> +#define DRAW_GRAPHIC_FUNC(BPP)   CONCAT2(g364fb_draw_graphic, BPP)
> +#define RGB_TO_PIXEL_FUNC(BPP)   CONCAT2(rgb_to_pixel, BPP)
> +#define PIXEL_TYPE(PIXEL_WIDTH)  CONCAT3(uint, PIXEL_WIDTH, _t)
> +
> +static void DRAW_GRAPHIC_FUNC(BPP)(G364State *s, int full_update)
> +{
> +    int i, j;
> +    int w_display;
> +    uint8_t *data_buffer;
> +    uint8_t *data_display, *dd;
> +
> +    data_buffer = s->vram_buffer;
> +    w_display = s->last_scr_width * PIXEL_WIDTH / 8;
> +    data_display = s->ds->data;
> +    for(i = 0; i < s->last_scr_height; i++) {
> +        dd = data_display;
> +        for (j = 0; j < s->last_scr_width; j++, dd += PIXEL_WIDTH / 8, 
> data_buffer++) {
> +            uint8_t index = *data_buffer;
> +
> +            if (index >= 16) {
> +                /* XXX: what to do? */
> +                continue;
> +            }

palette is defined as:  
        uint8_t palette[256][3];

Why is the index limited to 16 now?

> +            *((PIXEL_TYPE(PIXEL_WIDTH) *)dd) = RGB_TO_PIXEL_FUNC(BPP)(
> +                s->palette[index][0],
> +                s->palette[index][1],
> +                s->palette[index][2]);
> +        }
> +        data_display += s->ds->linesize;
> +    }
> +}

Here, you should add the #undef corresponding to the #define, as this
file is included more than once.

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net