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

[Peter Maydell <peter.maydell@linaro.org>]

On 30 June 2018 at 09:33, Thomas Huth <huth@tuxfamily.org> 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 <huth@tuxfamily.org>

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

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

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

Worth having at least a comment to say this device has
no mutable state and so needs no reset function or
vmstate.

> +}
> +
> +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);
> +}

This is so simple it's not really worth having, I think.
Just have the board model code create the device...

> diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
> new file mode 100644
> index 0000000000..cf07243bda
> --- /dev/null
> +++ b/include/hw/m68k/next-cube.h
> @@ -0,0 +1,8 @@
> +
> +#ifndef NEXT_CUBE_H
> +#define NEXT_CUBE_H
> +
> +/* next-fb.c */
> +void nextfb_init(void);
> +
> +#endif /* NEXT_CUBE_H */

New header files should have copyright and license comment headers
too, though in this case if you drop the nextfb_init() function
then the header isn't needed any more.

thanks
-- PMM