[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
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 1/4] m68k: Add NeXTcube framebuffer device emulation,
Philippe Mathieu-Daudé <=