Re: [Qemu-devel] [PATCHv2 2/2] sun4m: Add Sun CG3 framebuffer initialisation function

[Mark Cave-Ayland]

On 09/02/14 15:32, Andreas Färber wrote:

> IIUC SUNW,cgthree is an optional device, so it's not covered by my
> qom-test. Please follow-up with a tests/cg3-test.c so that it gets
> covered. Compare my recent PCI NIC series for how such a stub could look
> like, in particular vmxnet3-test.c since this will be sparc-only.

Okay - should this be a totally separate patch or appended to the end of 
this patchset?

> > ---
> >   hw/sparc/sun4m.c        |   60 +++++++++++++++++++++++++++++++++++++++++++++--
> >   include/sysemu/sysemu.h |    1 +
> >   vl.c                    |   24 +++++++++++++++++++
> >   3 files changed, 83 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> > index 94f7950..4c6c450 100644
> > --- a/hw/sparc/sun4m.c
> > +++ b/hw/sparc/sun4m.c
> > @@ -561,6 +561,31 @@ static void tcx_init(hwaddr addr, int vram_size, int width,
> >       }
> >   }
> >
> > +static void cg3_init(hwaddr addr, qemu_irq irq, int vram_size, int width,
> > +                        int height, int depth)
>
> Indentation?

Fixed.

> > +{
> > +    DeviceState *dev;
> > +    SysBusDevice *s;
> > +
> > +    dev = qdev_create(NULL, "SUNW,cgthree");
> > +    qdev_prop_set_uint32(dev, "vram_size", vram_size);
> > +    qdev_prop_set_uint16(dev, "width", width);
> > +    qdev_prop_set_uint16(dev, "height", height);
> > +    qdev_prop_set_uint16(dev, "depth", depth);
> > +    qdev_prop_set_uint64(dev, "prom_addr", addr);
> > +    qdev_init_nofail(dev);
> > +    s = SYS_BUS_DEVICE(dev);
> > +
> > +    /* FCode ROM */
> > +    sysbus_mmio_map(s, 0, addr);
> > +    /* DAC */
> > +    sysbus_mmio_map(s, 1, addr + 0x400000ULL);
> > +    /* 8-bit plane */
> > +    sysbus_mmio_map(s, 2, addr + 0x800000ULL);
> > +
> > +    sysbus_connect_irq(s, 0, irq);
> > +}
> > +
> >   /* NCR89C100/MACIO Internal ID register */
> >
> >   #define TYPE_MACIO_ID_REGISTER "macio_idreg"
> > @@ -918,8 +943,39 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
> >       }
> >       num_vsimms = 0;
> >       if (num_vsimms == 0) {
> > -        tcx_init(hwdef->tcx_base, 0x00100000, graphic_width, graphic_height,
> > -                 graphic_depth);
> > +        if (vga_interface_type == VGA_CG3) {
> > +            if (graphic_depth != 8) {
> > +                fprintf(stderr, "qemu: Unsupported depth: %d\n", 
> > graphic_depth);
>
> error_report() please - without "qemu: " and without trailing \n then.

Fixed.

> > +                exit(1);
> > +            }
> > +
> > +            if (!(graphic_width == 1024&&  graphic_height == 768)&&
> > +                !(graphic_width == 1152&&  graphic_height == 900)) {
> > +                fprintf(stderr, "qemu: Unsupported resolution: %d x %d\n",
> > +                        graphic_width, graphic_height);
>
> Dito.

Fixed.

> > +                exit(1);
> > +            }
> > +
> > +            /* sbus irq 5 */
> > +            cg3_init(hwdef->tcx_base, slavio_irq[11], 0x00100000,
> > +                     graphic_width, graphic_height, graphic_depth);
> > +        } else {
> > +            /* If no display specified, default to TCX */
> > +            if (graphic_depth != 8&&  graphic_depth != 24) {
> > +                fprintf(stderr, "qemu: Unsupported depth: %d\n",
> > +                        graphic_depth);
>
> Dito.

Fixed.

> > +                exit(1);
> > +            }
> > +
> > +            if (!(graphic_width == 1024&&  graphic_height == 768)) {
> > +                fprintf(stderr, "qemu: Unsupported resolution: %d x %d\n",
> > +                        graphic_width, graphic_height);
>
> Dito.

Fixed.

> > +                exit(1);
> > +            }
>
> These checks are new, right? Would deserve a mention in the commit message.

Yes that's a good point. Sun's OBP boots in the higher resolution so 
I've altered the commit message to make this clearer.

> > +
> > +            tcx_init(hwdef->tcx_base, 0x00100000, graphic_width, 
> > graphic_height,
> > +                     graphic_depth);
> > +        }
> >       }
> >
> >       for (i = num_vsimms; i<  MAX_VSIMMS; i++) {
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 495dae8..b90df9a 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -104,6 +104,7 @@ extern int autostart;
> >
> >   typedef enum {
> >       VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
> > +    VGA_TCX, VGA_CG3,
> >   } VGAInterfaceType;
> >
> >   extern int vga_interface_type;
> > diff --git a/vl.c b/vl.c
> > index 383be1b..61c8212 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2084,6 +2084,16 @@ static bool qxl_vga_available(void)
> >       return object_class_by_name("qxl-vga");
> >   }
> >
> > +static bool tcx_vga_available(void)
> > +{
> > +    return object_class_by_name("SUNW,tcx");
> > +}
> > +
> > +static bool cg3_vga_available(void)
> > +{
> > +    return object_class_by_name("SUNW,cgthree");
> > +}
> > +
> >   static void select_vgahw (const char *p)
> >   {
> >       const char *opts;
> > @@ -2119,6 +2129,20 @@ static void select_vgahw (const char *p)
> >               fprintf(stderr, "Error: QXL VGA not available\n");
> >               exit(0);
> >           }
> > +    } else if (strstart(p, "tcx",&opts)) {
> > +        if (tcx_vga_available()) {
> > +            vga_interface_type = VGA_TCX;
> > +        } else {
> > +            fprintf(stderr, "Error: TCX framebuffer not available\n");
> > +            exit(0);
>
> I note that these two and the below two are copied from QXL above, but
> shouldn't that be an exit(1) for such errors?
>
> error_report() would also be good.

I'm not sure about this because all of the existing -vga options use 
fprintf(stderr ... ) and exit(0). Changing this behaviour for just CG3 
seems to be the worst solution because then it may cause confusion for 
programs that (incorrectly) monitor stderr to detect these errors on 
startup as CG3 would have a different behaviour to everything else.

For bisectability I think that this would best be handled by a separate 
commit to change all of the -vga devices over to error_report()/exit(1) 
if/when people decide that this is the correct behaviour.


ATB,

Mark.