On 6/29/19 7:45 PM, Thomas Huth wrote: > On 29/06/2019 13.53, Philippe Mathieu-Daudé wrote: >> Hi Thomas, >> >> On 6/28/19 8:15 PM, Thomas Huth wrote: >>> The NeXTcube uses a linear framebuffer with 4 greyscale colors and >>> a fixed resolution of 1120 * 832. >>> This code has been taken from Bryce Lanham's GSoC 2011 NeXT branch at >>> >>> https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-fb.c >> >> Please use SHA1 for reference (unlikely case of Bryce pushing a new >> version to his repo): >> >> https://github.com/blanham/qemu-NeXT/blob/34f4323/hw/next-fb.c > > But if Bryce ever pushes a new version to his branch, the old SHA IDs > won't be part of a branch anymore, so they will be garbage collected > after a while and the links will become invalid. I think it's better to > refer to the "next-cube" branch.
OK. >>> and altered to fit the latest interface of the current QEMU (e.g. >>> the device has been "qdev"-ified etc.). >>> >>> Signed-off-by: Thomas Huth <h...@tuxfamily.org> >>> --- > [...] >>> diff --git a/hw/display/next-fb.c b/hw/display/next-fb.c >>> new file mode 100644 >>> index 0000000000..740102d7e9 >>> --- /dev/null >>> +++ b/hw/display/next-fb.c >>> @@ -0,0 +1,157 @@ >>> +/* >>> + * NeXT Cube/Station Framebuffer Emulation >>> + * >>> + * Copyright (c) 2011 Bryce Lanham >>> + * >>> + * Permission is hereby granted, free of charge, to any person obtaining a >>> copy >>> + * of this software and associated documentation files (the "Software"), >>> to deal >>> + * in the Software without restriction, including without limitation the >>> rights >>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or >>> sell >>> + * copies of the Software, and to permit persons to whom the Software is >>> + * furnished to do so, subject to the following conditions: >>> + * >>> + * The above copyright notice and this permission notice shall be included >>> in >>> + * all copies or substantial portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >>> OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >>> OTHER >>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >>> FROM, >>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >>> IN >>> + * THE SOFTWARE. >>> + */ >>> +#include "qemu/osdep.h" >>> +#include "qapi/error.h" >>> +#include "ui/console.h" >>> +#include "hw/hw.h" >>> +#include "hw/boards.h" >>> +#include "hw/loader.h" >>> +#include "hw/display/framebuffer.h" >>> +#define BITS 8 >> >> 'BITS' is not used, remove? > > Seems unused, indeed. I'll remove it. > >>> +static void nextfb_draw_line(void *opaque, uint8_t *d, const uint8_t *s, >>> + int width, int pitch) >>> +{ >>> + NeXTFbState *nfbstate = NEXTFB(opaque); >>> + static const uint32_t pal[4] = { >>> + 0xFFFFFFFF, 0xFFAAAAAA, 0xFF555555, 0xFF000000 >>> + }; >>> + uint32_t *buf = (uint32_t *)d; >>> + int i = 0; >>> + >>> + for (i = 0; i < nfbstate->cols / 4; i++) { >>> + int j = i * 4; >>> + uint8_t src = s[i]; >>> + buf[j + 3] = pal[src & 0x3]; >> >> 0x3 -> 3? > > I prefer the "0x" for bit-wise logical operations. OK >> or 0b11 :) > > Hmm, does that work with all compiler versions that we currently > support? I remember it was not working with older versions of GCC... $ git grep 0b accel/tcg/user-exec.c:608: switch (((insn >> 0) & 0b11)) { accel/tcg/user-exec.c:610: switch (((insn >> 2) & 0b11111)) { ... It now certainly does :) > Anyway, Bryce used 0x3 in his original code, so I'd like to keep it > close to his original code for the first commit. We can rework stuff > like this in later patches if we like, but for the initial commit, it > would be adequate that you can still recognize the original code, I think. Fair enough. >>> + src >>= 2; >>> + buf[j + 2] = pal[src & 0x3]; >>> + src >>= 2; >>> + buf[j + 1] = pal[src & 0x3]; >>> + src >>= 2; >>> + buf[j + 0] = pal[src & 0x3]; >>> + } >>> +} >>> + >>> +static void nextfb_update(void *opaque) >>> +{ >>> + NeXTFbState *s = NEXTFB(opaque); >>> + int dest_width = 4; >>> + int src_width; >>> + int first = 0; >>> + int last = 0; >>> + DisplaySurface *surface = qemu_console_surface(s->con); >>> + >>> + src_width = s->cols / 4 + 8; >>> + dest_width = s->cols * 4; >> >> Since those are currently const, should we move them to NeXTFbState >> and initialize them in nextfb_realize()? > > Should not matter much ... I think I'll also keep the original code here > for now. > >>> + >>> + if (s->invalidate) { >>> + framebuffer_update_memory_section(&s->fbsection, &s->fb_mr, 0, >>> + s->cols, src_width); >>> + s->invalidate = 0; >>> + } >>> + >>> + framebuffer_update_display(surface, &s->fbsection, 1120, 832, >> >> 1120 -> s->cols? >> 832 -> s->rows? >> >>> + src_width, dest_width, 0, 1, >>> nextfb_draw_line, >>> + s, &first, &last); >>> + >>> + dpy_gfx_update(s->con, 0, 0, 1120, 832); >> >> Ditto. > > Ok. > >>> +} >>> + >>> +static void nextfb_invalidate(void *opaque) >>> +{ >>> + NeXTFbState *s = NEXTFB(opaque); >>> + s->invalidate = 1; >>> +} >>> + >>> +static const GraphicHwOps nextfb_ops = { >>> + .invalidate = nextfb_invalidate, >>> + .gfx_update = nextfb_update, >>> +}; >>> + >>> +static void nextfb_realize(DeviceState *dev, Error **errp) >>> +{ >>> + NeXTFbState *s = NEXTFB(dev); >>> + >>> + memory_region_init_ram(&s->fb_mr, OBJECT(dev), "next-video", 0x1CB100, >>> + &error_fatal); >> >> 2 bits * cols * rows = 2 * 832 * 1120 = 0x1c7000 >> >> 0x1cb100 - 0x1c7000 = 0x4100 >> >> Any idea what are these 16K + 256 extra bytes for? > > No clue. But as you can see in nextfb_update() ("src_width = s->cols / 4 > + 8"), a line is a little bit wider than the visible 1120 pixels. Weird :) >> Anyway we have 2MB of VRAM on the hardware here, right? >> If so you should replace 0x1CB100 -> 2 * MiB. > > I don't know the Cube hardware that well ... so let's keep the original > values for now, we can still tune it later if necessary. > >>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->fb_mr); >>> + >>> + s->invalidate = 1; >>> + s->cols = 1120; >>> + s->rows = 832; >>> + >>> + s->con = graphic_console_init(dev, 0, &nextfb_ops, s); >>> + qemu_console_resize(s->con, s->cols, s->rows); >>> +} >>> + >>> +static void nextfb_class_init(ObjectClass *oc, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(oc); >>> + >>> + set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories); >>> + dc->realize = nextfb_realize; >>> +} >>> + >>> +static const TypeInfo nextfb_info = { >>> + .name = TYPE_NEXTFB, >>> + .parent = TYPE_SYS_BUS_DEVICE, >>> + .instance_size = sizeof(NeXTFbState), >>> + .class_init = nextfb_class_init, >>> +}; >>> + >>> +static void nextfb_register_types(void) >>> +{ >>> + type_register_static(&nextfb_info); >>> +} >>> + >>> +type_init(nextfb_register_types) >>> + >>> +void nextfb_init(void) >>> +{ >>> + DeviceState *dev; >>> + >>> + dev = qdev_create(NULL, TYPE_NEXTFB); >>> + qdev_init_nofail(dev); >>> + >>> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xB000000); >> >> I'd appreciate this written as 0x0B000000 (32-bit address range). >> >> Should you map the aliased VRAM regions here too? >> >> for (int i = 0; i <= 4; i++) { >> sysbus_mmio_map(SYS_BUS_DEVICE(dev), i, >> 0x0b000000 + 0x01000000 * i); >> } > > Where do you've got the information from that the VRAM region is aliased > a couple of times? I looked at Previous, the Next emulator: https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/cpu/memory.c#l41 Than looked around the code. >> Anyway nextfb_init() content is this is board-specific code, not >> related to the device. Can you move it there? > > Ok, will do. > > Thanks for the review! > > Thomas >