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

[Peter Maydell]

On 30 June 2018 at 20:47, Thomas Huth <address@hidden> wrote:
> Am Sat, 30 Jun 2018 17:12:01 +0100
> schrieb Peter Maydell <address@hidden>:
>
>> On 30 June 2018 at 09:33, Thomas Huth <address@hidden> 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
>> >
>> > 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>
>>
>> > +static void nextfb_realize(DeviceState *dev, Error **errp)
>> > +{
>> > +    NeXTFbState *s = NEXTFB(dev);
>> > +
>> > +    memory_region_allocate_system_memory(&s->fb_mr, NULL,
>> > "next.video",
>> > +                                         0x1CB100);
>>
>> memory_region_allocate_system_memory() is for allocating
>> a machine model's main memory, not for things like video
>> framebuffer RAM. (See the doc comment in memory.h.)
>
> That comment recommends memory_region_allocate_aux_memory(), but
> that function does not exist (anymore)?

Oops, good catch. That's a leftover comment that should have
been updated -- we changed the function name during discussion
of the patchset, I think, but I forgot to fix the comment.
I'll send a patch to fix that next week.

You want memory_region_init_ram() (which does both the MR
init and the mark-this-ram-for-migration).

>> > +    memory_region_add_subregion(get_system_memory(), 0xB000000,
>> > &s->fb_mr);
>>
>> Devices shouldn't directly add memory regions into
>> system memory. Instead they should expose sysbus
>> memory regions which the board model then maps into
>> the right place.
>
> Ok, ... so just to make sure that I've got you right: The device
> should call memory_region_init_ram() in it's realize function and the
> board code should then call sysbus_mmio_map() with that region, right?

The device should call memory_region_init_ram() and then
sysbus_init_mmio(); then the board code can call
sysbus_mmio_map().

I have a feeling that for framebuffer devices you should
also do something involving DIRTY_MEMORY_VGA, but I'm
not really familiar enough with the display code to say
for sure.

thanks
-- PMM