Re: [Qemu-devel] [PATCH v2 1/4] m68k: Add NeXTcube framebuffer device emulation

[Thomas Huth]

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.

>> 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 <address@hidden>
>> ---
[...]
>> 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.

> 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...

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.

>> +        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.

> 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?

> 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