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

["Andreas Färber" <afaerber@suse.de>]

Am 08.02.2014 17:38, schrieb Mark Cave-Ayland:
> In order to allow the user to choose the framebuffer for sparc-softmmu, add
> -vga tcx and -vga cg3 options to the QEMU command line. If no option is
> specified, the default TCX framebuffer is used.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> CC: Blue Swirl <blauwirbel@gmail.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Bob Breuer <breuerr@mc.net>
> CC: Artyom Tarasenko <atar4qemu@gmail.com>

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.

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

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

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

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

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

Dito.

> +                exit(1);
> +            }

These checks are new, right? Would deserve a mention in the commit message.

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

> +        }
> +    } else if (strstart(p, "cg3", &opts)) {
> +        if (cg3_vga_available()) {
> +            vga_interface_type = VGA_CG3;
> +        } else {
> +            fprintf(stderr, "Error: CG3 framebuffer not available\n");
> +            exit(0);
> +        }
>      } else if (!strstart(p, "none", &opts)) {
>      invalid_vga:
>          fprintf(stderr, "Unknown vga type: %s\n", p);

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg