qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH v2 1/4] m68k: Add NeXTcube framebuffer device emulation
Date: Tue, 2 Jul 2019 18:01:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

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

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
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]