[PATCH 00/12] macfb: fixes for booting MacOS

[PATCH 00/12] macfb: fixes for booting MacOS

[Mark Cave-Ayland]

This is the next set of patches to allow users to boot MacOS in QEMU's
q800 machine.

Patches 1 to 3 are fixes for existing bugs that I discovered whilst
developing the remainder of the patchset whilst patch 4 simplifies the
registration of the framebuffer RAM.

Patch 5 adds trace events to the framebuffer register accesses. The
framebuffer registers are not officially documented, so the macfb
device changes here are based upon reading of Linux/NetBSD source code,
using gdbstub during the MacOS toolbox ROM initialisation, and changing
the framebuffer size/depth within MacOS itself with these trace events
enabled.

Patches 6 and 7 implement the mode sense logic documented in Apple
Technical Note HW26 "Macintosh Quadra Built-In Video" and configure the
default display type to be VGA.

Patch 8 implements the common monitor modes used for VGA at 640x480 and
800x600 for 1, 2, 4, 8 and 24-bit depths and also the Apple 21" color
monitor at 1152x870 with 8-bit depth.

Patches 9 and 10 fix up errors in the 1-bit and 24-bit pixel encodings
discovered when testing these color depths in MacOS.

Patch 11 adds a timer to implement the 60.15Hz VBL interrupt which is
required for MacOS to process mouse movements, whilst patch 12 wires the
same interrupt to a dedicated pin on VIA2 reserved for the video
interrupt on the Quadra 800.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (12):
  macfb: handle errors that occur during realize
  macfb: fix invalid object reference in macfb_common_realize()
  macfb: fix overflow of color_palette array
  macfb: use memory_region_init_ram() in macfb_common_realize() for the
    framebuffer
  macfb: add trace events for reading and writing the control registers
  macfb: implement mode sense to allow display type to be detected
  macfb: add qdev property to specify display type
  macfb: add common monitor modes supported by the MacOS toolbox ROM
  macfb: fix up 1-bit pixel encoding
  macfb: fix 24-bit RGB pixel encoding
  macfb: add vertical blank interrupt
  q800: wire macfb IRQ to separate video interrupt on VIA2

 hw/display/macfb.c         | 348 +++++++++++++++++++++++++++++++++++--
 hw/display/trace-events    |   7 +
 hw/m68k/q800.c             |  23 ++-
 include/hw/display/macfb.h |  43 +++++
 4 files changed, 397 insertions(+), 24 deletions(-)

-- 
2.20.1

[PATCH 01/12] macfb: handle errors that occur during realize

[Mark Cave-Ayland]

Make sure any errors that occur within the macfb realize chain are detected
and handled correctly to prevent crashes and to ensure that error messages are
reported back to the user.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 76808b69cc..2b747a8de8 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
     MacfbState *ms = &s->macfb;
 
     macfb_common_realize(dev, ms, errp);
+    if (*errp) {
+        return;
+    }
+
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
 }
@@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
     MacfbState *ms = &s->macfb;
 
     ndc->parent_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
 
     macfb_common_realize(dev, ms, errp);
+    if (*errp) {
+        return;
+    }
+
     memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
     memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
 }
-- 
2.20.1

[PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[Mark Cave-Ayland]

During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
memory region used for the framebuffer but the owner object reference is
incorrect since MacFbState is a typedef and not a QOM type.

Change the memory region owner to be the corresponding DeviceState to fix the
issue and prevent random crashes during macfb_common_realize().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 2b747a8de8..815870f2fe 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -365,7 +365,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
     memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
                           "macfb-ctrl", 0x1000);
 
-    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram",
+    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
                                      MACFB_VRAM_SIZE, errp);
     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-- 
2.20.1

[PATCH 03/12] macfb: fix overflow of color_palette array

[Mark Cave-Ayland]

The palette_current index counter has a maximum size of 256 * 3 to cover a full
color palette of 256 RGB entries. Linux assumes that the palette_current index
wraps back around to zero after writing 256 RGB entries so ensure that
palette_current is reset at this point to prevent data corruption within
MacfbState.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 815870f2fe..f4e789d0d7 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -307,6 +307,9 @@ static void macfb_ctrl_write(void *opaque,
         if (s->palette_current % 3) {
             macfb_invalidate_display(s);
         }
+        if (s->palette_current >= sizeof(s->color_palette)) {
+            s->palette_current = 0;
+        }
         break;
     }
 }
-- 
2.20.1

[PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[Mark Cave-Ayland]

Currently macfb_common_realize() defines the framebuffer RAM memory region as
being non-migrateable but then immediately registers it for migration. Replace
memory_region_init_ram_nomigrate() with memory_region_init_ram() which is clearer
and does exactly the same thing.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index f4e789d0d7..e86fbbbb64 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -368,11 +368,10 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
     memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
                           "macfb-ctrl", 0x1000);
 
-    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
-                                     MACFB_VRAM_SIZE, errp);
+    memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
+                           MACFB_VRAM_SIZE, errp);
     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-    vmstate_register_ram(&s->mem_vram, dev);
     memory_region_set_coalescing(&s->mem_vram);
 }
 
-- 
2.20.1

[PATCH 05/12] macfb: add trace events for reading and writing the control registers

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c      | 8 +++++++-
 hw/display/trace-events | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index e86fbbbb64..62c2727a5b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "trace.h"
 
 #define VIDEO_BASE 0x00001000
 #define DAFB_BASE  0x00800000
@@ -289,7 +290,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
                                 hwaddr addr,
                                 unsigned int size)
 {
-    return 0;
+    uint64_t val = 0;
+
+    trace_macfb_ctrl_read(addr, val, size);
+    return val;
 }
 
 static void macfb_ctrl_write(void *opaque,
@@ -312,6 +316,8 @@ static void macfb_ctrl_write(void *opaque,
         }
         break;
     }
+
+    trace_macfb_ctrl_write(addr, val, size);
 }
 
 static const MemoryRegionOps macfb_ctrl_ops = {
diff --git a/hw/display/trace-events b/hw/display/trace-events
index f03f6655bc..be1353e8e7 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -167,3 +167,7 @@ sm501_disp_ctrl_read(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 sm501_disp_ctrl_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 sm501_2d_engine_read(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 sm501_2d_engine_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
+
+# macfb.c
+macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
+macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
-- 
2.20.1

[PATCH 06/12] macfb: implement mode sense to allow display type to be detected

[Mark Cave-Ayland]

The MacOS toolbox ROM uses the monitor sense to detect the display type and then
offer a fixed set of resolutions and colour depths accordingly. Implement the
monitor sense using information found in Apple Technical Note HW26: "Macintosh
Quadra Built-In Video" along with some local experiments.

Since the default configuration is 640 x 480 with 8-bit colour then hardcode
the sense register to return MACFB_DISPLAY_VGA for now.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c         | 117 ++++++++++++++++++++++++++++++++++++-
 hw/display/trace-events    |   2 +
 include/hw/display/macfb.h |  20 +++++++
 3 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 62c2727a5b..5c95aa4a11 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -28,8 +28,66 @@
 #define MACFB_PAGE_SIZE 4096
 #define MACFB_VRAM_SIZE (4 * MiB)
 
-#define DAFB_RESET      0x200
-#define DAFB_LUT        0x213
+#define DAFB_MODE_SENSE     0x1c
+#define DAFB_RESET          0x200
+#define DAFB_LUT            0x213
+
+
+/*
+ * Quadra sense codes taken from Apple Technical Note HW26:
+ * "Macintosh Quadra Built-In Video". The sense codes and
+ * extended sense codes have different meanings:
+ *
+ * Sense:
+ *    bit 2: SENSE2 (pin 10)
+ *    bit 1: SENSE1 (pin 7)
+ *    bit 0: SENSE0 (pin 4)
+ *
+ * 0 = pin tied to ground
+ * 1 = pin unconnected
+ *
+ * Extended Sense:
+ *    bit 2: pins 4-10
+ *    bit 1: pins 10-7
+ *    bit 0: pins 7-4
+ *
+ * 0 = pins tied together
+ * 1 = pins unconnected
+ *
+ * Reads from the sense register appear to be active low, i.e. a 1 indicates
+ * that the pin is tied to ground, a 0 indicates the pin is disconnected.
+ *
+ * Writes to the sense register appear to activate pulldowns i.e. a 1 enables
+ * a pulldown on a particular pin.
+ *
+ * The MacOS toolbox appears to use a series of reads and writes to first
+ * determine if extended sense is to be used, and then check which pins are
+ * tied together in order to determine the display type.
+ */
+
+typedef struct MacFbSense {
+    uint8_t type;
+    uint8_t sense;
+    uint8_t ext_sense;
+} MacFbSense;
+
+static MacFbSense macfb_sense_table[] = {
+    { MACFB_DISPLAY_APPLE_21_COLOR, 0x0, 0 },
+    { MACFB_DISPLAY_APPLE_PORTRAIT, 0x1, 0 },
+    { MACFB_DISPLAY_APPLE_12_RGB, 0x2, 0 },
+    { MACFB_DISPLAY_APPLE_2PAGE_MONO, 0x3, 0 },
+    { MACFB_DISPLAY_NTSC_UNDERSCAN, 0x4, 0 },
+    { MACFB_DISPLAY_NTSC_OVERSCAN, 0x4, 0 },
+    { MACFB_DISPLAY_APPLE_12_MONO, 0x6, 0 },
+    { MACFB_DISPLAY_APPLE_13_RGB, 0x6, 0 },
+    { MACFB_DISPLAY_16_COLOR, 0x7, 0x3 },
+    { MACFB_DISPLAY_PAL1_UNDERSCAN, 0x7, 0x0 },
+    { MACFB_DISPLAY_PAL1_OVERSCAN, 0x7, 0x0 },
+    { MACFB_DISPLAY_PAL2_UNDERSCAN, 0x7, 0x6 },
+    { MACFB_DISPLAY_PAL2_OVERSCAN, 0x7, 0x6 },
+    { MACFB_DISPLAY_VGA, 0x7, 0x5 },
+    { MACFB_DISPLAY_SVGA, 0x7, 0x5 },
+};
 
 
 typedef void macfb_draw_line_func(MacfbState *s, uint8_t *d, uint32_t addr,
@@ -253,6 +311,50 @@ static void macfb_invalidate_display(void *opaque)
     memory_region_set_dirty(&s->mem_vram, 0, MACFB_VRAM_SIZE);
 }
 
+static uint32_t macfb_sense_read(MacfbState *s)
+{
+    MacFbSense *macfb_sense;
+    uint8_t sense;
+
+    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
+    if (macfb_sense->sense == 0x7) {
+        /* Extended sense */
+        sense = 0;
+        if (!(macfb_sense->ext_sense & 1)) {
+            /* Pins 7-4 together */
+            if (~s->sense & 3) {
+                sense = (~s->sense & 7) | 3;
+            }
+        }
+        if (!(macfb_sense->ext_sense & 2)) {
+            /* Pins 10-7 together */
+            if (~s->sense & 6) {
+                sense = (~s->sense & 7) | 6;
+            }
+        }
+        if (!(macfb_sense->ext_sense & 4)) {
+            /* Pins 4-10 together */
+            if (~s->sense & 5) {
+                sense = (~s->sense & 7) | 5;
+            }
+        }
+    } else {
+        /* Normal sense */
+        sense = (~macfb_sense->sense & 7) | (~s->sense & 7);
+    }
+
+    trace_macfb_sense_read(sense);
+    return sense;
+}
+
+static void macfb_sense_write(MacfbState *s, uint32_t val)
+{
+    s->sense = val;
+
+    trace_macfb_sense_write(val);
+    return;
+}
+
 static void macfb_update_display(void *opaque)
 {
     MacfbState *s = opaque;
@@ -290,8 +392,15 @@ static uint64_t macfb_ctrl_read(void *opaque,
                                 hwaddr addr,
                                 unsigned int size)
 {
+    MacfbState *s = opaque;
     uint64_t val = 0;
 
+    switch (addr) {
+    case DAFB_MODE_SENSE:
+        val = macfb_sense_read(s);
+        break;
+    }
+
     trace_macfb_ctrl_read(addr, val, size);
     return val;
 }
@@ -303,6 +412,9 @@ static void macfb_ctrl_write(void *opaque,
 {
     MacfbState *s = opaque;
     switch (addr) {
+    case DAFB_MODE_SENSE:
+        macfb_sense_write(s, val);
+        break;
     case DAFB_RESET:
         s->palette_current = 0;
         break;
@@ -343,6 +455,7 @@ static const VMStateDescription vmstate_macfb = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
         VMSTATE_UINT32(palette_current, MacfbState),
+        VMSTATE_UINT32(sense, MacfbState),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/display/trace-events b/hw/display/trace-events
index be1353e8e7..6e378036ab 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -171,3 +171,5 @@ sm501_2d_engine_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
 # macfb.c
 macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
 macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
+macfb_sense_read(uint32_t value) "video sense: 0x%"PRIx32
+macfb_sense_write(uint32_t value) "video sense: 0x%"PRIx32
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index 80806b0306..febf4ce0e8 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -17,6 +17,24 @@
 #include "ui/console.h"
 #include "qom/object.h"
 
+typedef enum  {
+    MACFB_DISPLAY_APPLE_21_COLOR = 0,
+    MACFB_DISPLAY_APPLE_PORTRAIT = 1,
+    MACFB_DISPLAY_APPLE_12_RGB = 2,
+    MACFB_DISPLAY_APPLE_2PAGE_MONO = 3,
+    MACFB_DISPLAY_NTSC_UNDERSCAN = 4,
+    MACFB_DISPLAY_NTSC_OVERSCAN = 5,
+    MACFB_DISPLAY_APPLE_12_MONO = 6,
+    MACFB_DISPLAY_APPLE_13_RGB = 7,
+    MACFB_DISPLAY_16_COLOR = 8,
+    MACFB_DISPLAY_PAL1_UNDERSCAN = 9,
+    MACFB_DISPLAY_PAL1_OVERSCAN = 10,
+    MACFB_DISPLAY_PAL2_UNDERSCAN = 11,
+    MACFB_DISPLAY_PAL2_OVERSCAN = 12,
+    MACFB_DISPLAY_VGA = 13,
+    MACFB_DISPLAY_SVGA = 14,
+} MacfbDisplayType;
+
 typedef struct MacfbState {
     MemoryRegion mem_vram;
     MemoryRegion mem_ctrl;
@@ -28,6 +46,8 @@ typedef struct MacfbState {
     uint8_t color_palette[256 * 3];
     uint32_t width, height; /* in pixels */
     uint8_t depth;
+
+    uint32_t sense;
 } MacfbState;
 
 #define TYPE_MACFB "sysbus-macfb"
-- 
2.20.1

[PATCH 07/12] macfb: add qdev property to specify display type

[Mark Cave-Ayland]

Since the available resolutions and colour depths are determined by the attached
display type, add a qdev property to allow the display type to be specified.

The main resolutions of interest are high resolution 1152x870 with 8-bit colour
and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
to allow high resolution mode if specified and otherwise fall back to SVGA.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c         | 6 +++++-
 hw/m68k/q800.c             | 5 +++++
 include/hw/display/macfb.h | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 5c95aa4a11..023d1f0cd1 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
     MacFbSense *macfb_sense;
     uint8_t sense;
 
-    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
+    macfb_sense = &macfb_sense_table[s->type];
     if (macfb_sense->sense == 0x7) {
         /* Extended sense */
         sense = 0;
@@ -545,6 +545,8 @@ static Property macfb_sysbus_properties[] = {
     DEFINE_PROP_UINT32("width", MacfbSysBusState, macfb.width, 640),
     DEFINE_PROP_UINT32("height", MacfbSysBusState, macfb.height, 480),
     DEFINE_PROP_UINT8("depth", MacfbSysBusState, macfb.depth, 8),
+    DEFINE_PROP_UINT8("display", MacfbSysBusState, macfb.type,
+                      MACFB_DISPLAY_VGA),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -552,6 +554,8 @@ static Property macfb_nubus_properties[] = {
     DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
     DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
     DEFINE_PROP_UINT8("depth", MacfbNubusState, macfb.depth, 8),
+    DEFINE_PROP_UINT8("display", MacfbNubusState, macfb.type,
+                      MACFB_DISPLAY_VGA),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 09b3366024..5223b880bc 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -421,6 +421,11 @@ static void q800_init(MachineState *machine)
     qdev_prop_set_uint32(dev, "width", graphic_width);
     qdev_prop_set_uint32(dev, "height", graphic_height);
     qdev_prop_set_uint8(dev, "depth", graphic_depth);
+    if (graphic_width == 1152 && graphic_height == 870 && graphic_depth == 8) {
+        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_APPLE_21_COLOR);
+    } else {
+        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_VGA);
+    }
     qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
 
     cs = CPU(cpu);
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index febf4ce0e8..e95a97ebdc 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -46,6 +46,7 @@ typedef struct MacfbState {
     uint8_t color_palette[256 * 3];
     uint32_t width, height; /* in pixels */
     uint8_t depth;
+    uint8_t type;
 
     uint32_t sense;
 } MacfbState;
-- 
2.20.1

[PATCH 08/12] macfb: add common monitor modes supported by the MacOS toolbox ROM

[Mark Cave-Ayland]

The monitor modes table is found by experimenting with the Monitors Control
Panel in MacOS and analysing the reads/writes. From this it can be found that
the mode is controlled by writes to the DAFB_MODE_CTRL1 and DAFB_MODE_CTRL2
registers.

Implement the first block of DAFB registers as a register array including the
existing sense register, the newly discovered control registers above, and also
the DAFB_MODE_VADDR1 and DAFB_MODE_VADDR2 registers which are used by NetBSD to
determine the current video mode.

These experiments also show that the offset of the start of video RAM and the
stride can change depending upon the monitor mode, so update macfb_draw_graphic()
and both the BI_MAC_VADDR and BI_MAC_VROW bootinfo for the q800 machine
accordingly.

Finally update macfb_common_realize() so that only the resolution and depth
supported by the display type can be specified on the command line.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c         | 125 ++++++++++++++++++++++++++++++++-----
 hw/display/trace-events    |   1 +
 hw/m68k/q800.c             |  11 ++--
 include/hw/display/macfb.h |  16 ++++-
 4 files changed, 132 insertions(+), 21 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 023d1f0cd1..6a69334565 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -22,12 +22,16 @@
 #include "migration/vmstate.h"
 #include "trace.h"
 
-#define VIDEO_BASE 0x00001000
+#define VIDEO_BASE 0x0
 #define DAFB_BASE  0x00800000
 
 #define MACFB_PAGE_SIZE 4096
 #define MACFB_VRAM_SIZE (4 * MiB)
 
+#define DAFB_MODE_VADDR1    0x0
+#define DAFB_MODE_VADDR2    0x4
+#define DAFB_MODE_CTRL1     0x8
+#define DAFB_MODE_CTRL2     0xc
 #define DAFB_MODE_SENSE     0x1c
 #define DAFB_RESET          0x200
 #define DAFB_LUT            0x213
@@ -89,6 +93,22 @@ static MacFbSense macfb_sense_table[] = {
     { MACFB_DISPLAY_SVGA, 0x7, 0x5 },
 };
 
+static MacFbMode macfb_mode_table[] = {
+    { MACFB_DISPLAY_VGA, 1, 0x100, 0x71e, 640, 480, 0x400, 0x1000 },
+    { MACFB_DISPLAY_VGA, 2, 0x100, 0x70e, 640, 480, 0x400, 0x1000 },
+    { MACFB_DISPLAY_VGA, 4, 0x100, 0x706, 640, 480, 0x400, 0x1000 },
+    { MACFB_DISPLAY_VGA, 8, 0x100, 0x702, 640, 480, 0x400, 0x1000 },
+    { MACFB_DISPLAY_VGA, 24, 0x100, 0x7ff, 640, 480, 0x1000, 0x1000 },
+    { MACFB_DISPLAY_VGA, 1, 0xd0 , 0x70e, 800, 600, 0x340, 0xe00 },
+    { MACFB_DISPLAY_VGA, 2, 0xd0 , 0x706, 800, 600, 0x340, 0xe00 },
+    { MACFB_DISPLAY_VGA, 4, 0xd0 , 0x702, 800, 600, 0x340, 0xe00 },
+    { MACFB_DISPLAY_VGA, 8, 0xd0,  0x700, 800, 600, 0x340, 0xe00 },
+    { MACFB_DISPLAY_VGA, 24, 0x340, 0x100, 800, 600, 0xd00, 0xe00 },
+    { MACFB_DISPLAY_APPLE_21_COLOR, 1, 0x90, 0x506, 1152, 870, 0x240, 0x80 },
+    { MACFB_DISPLAY_APPLE_21_COLOR, 2, 0x90, 0x502, 1152, 870, 0x240, 0x80 },
+    { MACFB_DISPLAY_APPLE_21_COLOR, 4, 0x90, 0x500, 1152, 870, 0x240, 0x80 },
+    { MACFB_DISPLAY_APPLE_21_COLOR, 8, 0x120, 0x5ff, 1152, 870, 0x480, 0x80 },
+};
 
 typedef void macfb_draw_line_func(MacfbState *s, uint8_t *d, uint32_t addr,
                                   int width);
@@ -246,7 +266,7 @@ static void macfb_draw_graphic(MacfbState *s)
     ram_addr_t page;
     uint32_t v = 0;
     int y, ymin;
-    int macfb_stride = (s->depth * s->width + 7) / 8;
+    int macfb_stride = s->mode->stride;
     macfb_draw_line_func *macfb_draw_line;
 
     switch (s->depth) {
@@ -278,7 +298,7 @@ static void macfb_draw_graphic(MacfbState *s)
                                              DIRTY_MEMORY_VGA);
 
     ymin = -1;
-    page = 0;
+    page = s->mode->offset;
     for (y = 0; y < s->height; y++, page += macfb_stride) {
         if (macfb_check_dirty(s, snap, page, macfb_stride)) {
             uint8_t *data_display;
@@ -322,25 +342,26 @@ static uint32_t macfb_sense_read(MacfbState *s)
         sense = 0;
         if (!(macfb_sense->ext_sense & 1)) {
             /* Pins 7-4 together */
-            if (~s->sense & 3) {
-                sense = (~s->sense & 7) | 3;
+            if (~s->regs[DAFB_MODE_SENSE >> 2] & 3) {
+                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 3;
             }
         }
         if (!(macfb_sense->ext_sense & 2)) {
             /* Pins 10-7 together */
-            if (~s->sense & 6) {
-                sense = (~s->sense & 7) | 6;
+            if (~s->regs[DAFB_MODE_SENSE >> 2] & 6) {
+                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 6;
             }
         }
         if (!(macfb_sense->ext_sense & 4)) {
             /* Pins 4-10 together */
-            if (~s->sense & 5) {
-                sense = (~s->sense & 7) | 5;
+            if (~s->regs[DAFB_MODE_SENSE >> 2] & 5) {
+                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 5;
             }
         }
     } else {
         /* Normal sense */
-        sense = (~macfb_sense->sense & 7) | (~s->sense & 7);
+        sense = (~macfb_sense->sense & 7) |
+                (~s->regs[DAFB_MODE_SENSE >> 2] & 7);
     }
 
     trace_macfb_sense_read(sense);
@@ -349,12 +370,64 @@ static uint32_t macfb_sense_read(MacfbState *s)
 
 static void macfb_sense_write(MacfbState *s, uint32_t val)
 {
-    s->sense = val;
+    s->regs[DAFB_MODE_SENSE >> 2] = val;
 
     trace_macfb_sense_write(val);
     return;
 }
 
+static void macfb_update_mode(MacfbState *s)
+{
+    s->width = s->mode->width;
+    s->height = s->mode->height;
+    s->depth = s->mode->depth;
+
+    trace_macfb_update_mode(s->width, s->height, s->depth);
+    macfb_invalidate_display(s);
+}
+
+static void macfb_mode_write(MacfbState *s)
+{
+    MacFbMode *macfb_mode;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(macfb_mode_table); i++) {
+        macfb_mode = &macfb_mode_table[i];
+
+        if (s->type != macfb_mode->type) {
+            continue;
+        }
+
+        if ((s->regs[DAFB_MODE_CTRL1 >> 2] & 0xff) ==
+             (macfb_mode->mode_ctrl1 & 0xff) &&
+            (s->regs[DAFB_MODE_CTRL2 >> 2] & 0xff) ==
+             (macfb_mode->mode_ctrl2 & 0xff)) {
+            s->mode = macfb_mode;
+            macfb_update_mode(s);
+            break;
+        }
+    }
+}
+
+static MacFbMode *macfb_find_mode(MacfbDisplayType display_type,
+                                  uint16_t width, uint16_t height,
+                                  uint8_t depth)
+{
+    MacFbMode *macfb_mode;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(macfb_mode_table); i++) {
+        macfb_mode = &macfb_mode_table[i];
+
+        if (display_type == macfb_mode->type && width == macfb_mode->width &&
+                height == macfb_mode->height && depth == macfb_mode->depth) {
+            return macfb_mode;
+        }
+    }
+
+    return NULL;
+}
+
 static void macfb_update_display(void *opaque)
 {
     MacfbState *s = opaque;
@@ -396,6 +469,12 @@ static uint64_t macfb_ctrl_read(void *opaque,
     uint64_t val = 0;
 
     switch (addr) {
+    case DAFB_MODE_VADDR1:
+    case DAFB_MODE_VADDR2:
+    case DAFB_MODE_CTRL1:
+    case DAFB_MODE_CTRL2:
+        val = s->regs[addr >> 2];
+        break;
     case DAFB_MODE_SENSE:
         val = macfb_sense_read(s);
         break;
@@ -412,6 +491,17 @@ static void macfb_ctrl_write(void *opaque,
 {
     MacfbState *s = opaque;
     switch (addr) {
+    case DAFB_MODE_VADDR1:
+    case DAFB_MODE_VADDR2:
+        s->regs[addr >> 2] = val;
+        break;
+    case DAFB_MODE_CTRL1 ... DAFB_MODE_CTRL1 + 3:
+    case DAFB_MODE_CTRL2 ... DAFB_MODE_CTRL2 + 3:
+        s->regs[addr >> 2] = val;
+        if (val) {
+            macfb_mode_write(s);
+        }
+        break;
     case DAFB_MODE_SENSE:
         macfb_sense_write(s, val);
         break;
@@ -442,7 +532,7 @@ static const MemoryRegionOps macfb_ctrl_ops = {
 
 static int macfb_post_load(void *opaque, int version_id)
 {
-    macfb_invalidate_display(opaque);
+    macfb_mode_write(opaque);
     return 0;
 }
 
@@ -455,7 +545,7 @@ static const VMStateDescription vmstate_macfb = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
         VMSTATE_UINT32(palette_current, MacfbState),
-        VMSTATE_UINT32(sense, MacfbState),
+        VMSTATE_UINT32_ARRAY(regs, MacfbState, MACFB_NUM_REGS),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -469,9 +559,10 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
 {
     DisplaySurface *surface;
 
-    if (s->depth != 1 && s->depth != 2 && s->depth != 4 && s->depth != 8 &&
-        s->depth != 16 && s->depth != 24) {
-        error_setg(errp, "unknown guest depth %d", s->depth);
+    s->mode = macfb_find_mode(s->type, s->width, s->height, s->depth);
+    if (!s->mode) {
+        error_setg(errp, "unknown display mode: width %d, height %d, depth %d",
+                   s->width, s->height, s->depth);
         return;
     }
 
@@ -492,6 +583,8 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
     memory_region_set_coalescing(&s->mem_vram);
+
+    macfb_update_mode(s);
 }
 
 static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 6e378036ab..75574e5976 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -173,3 +173,4 @@ macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " valu
 macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
 macfb_sense_read(uint32_t value) "video sense: 0x%"PRIx32
 macfb_sense_write(uint32_t value) "video sense: 0x%"PRIx32
+macfb_update_mode(uint32_t width, uint32_t height, uint8_t depth) "setting mode to width %"PRId32 " height %"PRId32 " size %d"
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 5223b880bc..df3fd3711e 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -74,7 +74,7 @@
  * is needed by the kernel to have early display and
  * thus provided by the bootloader
  */
-#define VIDEO_BASE            0xf9001000
+#define VIDEO_BASE            0xf9000000
 
 #define MAC_CLOCK  3686418
 
@@ -221,6 +221,7 @@ static void q800_init(MachineState *machine)
     uint8_t *prom;
     const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
     int i, checksum;
+    MacFbMode *macfb_mode;
     ram_addr_t ram_size = machine->ram_size;
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
@@ -428,6 +429,8 @@ static void q800_init(MachineState *machine)
     }
     qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
 
+    macfb_mode = (NUBUS_MACFB(dev)->macfb).mode;
+
     cs = CPU(cpu);
     if (linux_boot) {
         uint64_t high;
@@ -450,12 +453,12 @@ static void q800_init(MachineState *machine)
         BOOTINFO1(cs->as, parameters_base,
                   BI_MAC_MEMSIZE, ram_size >> 20); /* in MB */
         BOOTINFO2(cs->as, parameters_base, BI_MEMCHUNK, 0, ram_size);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VADDR, VIDEO_BASE);
+        BOOTINFO1(cs->as, parameters_base, BI_MAC_VADDR,
+                  VIDEO_BASE + macfb_mode->offset);
         BOOTINFO1(cs->as, parameters_base, BI_MAC_VDEPTH, graphic_depth);
         BOOTINFO1(cs->as, parameters_base, BI_MAC_VDIM,
                   (graphic_height << 16) | graphic_width);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VROW,
-                  (graphic_width * graphic_depth + 7) / 8);
+        BOOTINFO1(cs->as, parameters_base, BI_MAC_VROW, macfb_mode->stride);
         BOOTINFO1(cs->as, parameters_base, BI_MAC_SCCBASE, SCC_BASE);
 
         rom = g_malloc(sizeof(*rom));
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index e95a97ebdc..0aff0d84d2 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -35,6 +35,19 @@ typedef enum  {
     MACFB_DISPLAY_SVGA = 14,
 } MacfbDisplayType;
 
+typedef struct MacFbMode {
+    uint8_t type;
+    uint8_t depth;
+    uint32_t mode_ctrl1;
+    uint32_t mode_ctrl2;
+    uint32_t width;
+    uint32_t height;
+    uint32_t stride;
+    uint32_t offset;
+} MacFbMode;
+
+#define MACFB_NUM_REGS      8
+
 typedef struct MacfbState {
     MemoryRegion mem_vram;
     MemoryRegion mem_ctrl;
@@ -48,7 +61,8 @@ typedef struct MacfbState {
     uint8_t depth;
     uint8_t type;
 
-    uint32_t sense;
+    uint32_t regs[MACFB_NUM_REGS];
+    MacFbMode *mode;
 } MacfbState;
 
 #define TYPE_MACFB "sysbus-macfb"
-- 
2.20.1

[PATCH 09/12] macfb: fix up 1-bit pixel encoding

[Mark Cave-Ayland]

The MacOS driver expects the RGB values for the pixel to be in entries 0 and 1
of the colour palette.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 6a69334565..0c9e181b9b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -128,7 +128,9 @@ static void macfb_draw_line1(MacfbState *s, uint8_t *d, uint32_t addr,
     for (x = 0; x < width; x++) {
         int bit = x & 7;
         int idx = (macfb_read_byte(s, addr) >> (7 - bit)) & 1;
-        r = g = b  = ((1 - idx) << 7);
+        r = s->color_palette[idx * 3];
+        g = s->color_palette[idx * 3 + 1];
+        b = s->color_palette[idx * 3 + 2];
         addr += (bit == 7);
 
         *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-- 
2.20.1

[PATCH 10/12] macfb: fix 24-bit RGB pixel encoding

[Mark Cave-Ayland]

According to Apple Technical Note HW26: "Macintosh Quadra Built-In Video" the
in-built framebuffer encodes each 24-bit pixel into 4 bytes. Adjust the 24-bit
RGB pixel encoding accordingly which agrees with the encoding expected by MacOS
when changing into 24-bit colour mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 0c9e181b9b..29f6ad8eba 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -224,10 +224,10 @@ static void macfb_draw_line24(MacfbState *s, uint8_t *d, uint32_t addr,
     int x;
 
     for (x = 0; x < width; x++) {
-        r = macfb_read_byte(s, addr);
-        g = macfb_read_byte(s, addr + 1);
-        b = macfb_read_byte(s, addr + 2);
-        addr += 3;
+        r = macfb_read_byte(s, addr + 1);
+        g = macfb_read_byte(s, addr + 2);
+        b = macfb_read_byte(s, addr + 3);
+        addr += 4;
 
         *(uint32_t *)d = rgb_to_pixel32(r, g, b);
         d += 4;
-- 
2.20.1

[PATCH 11/12] macfb: add vertical blank interrupt

[Mark Cave-Ayland]

The MacOS driver expects a 60.15Hz vertical blank interrupt to be generated by
the framebuffer which in turn schedules the mouse driver via the Vertical Retrace
Manager.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c         | 81 ++++++++++++++++++++++++++++++++++++++
 include/hw/display/macfb.h |  8 ++++
 2 files changed, 89 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 29f6ad8eba..60a203e67b 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -33,9 +33,16 @@
 #define DAFB_MODE_CTRL1     0x8
 #define DAFB_MODE_CTRL2     0xc
 #define DAFB_MODE_SENSE     0x1c
+#define DAFB_INTR_MASK      0x104
+#define DAFB_INTR_STAT      0x108
+#define DAFB_INTR_CLEAR     0x10c
 #define DAFB_RESET          0x200
 #define DAFB_LUT            0x213
 
+#define DAFB_INTR_VBL   0x4
+
+/* Vertical Blank period (60.15Hz) */
+#define DAFB_INTR_VBL_PERIOD_NS 16625800
 
 /*
  * Quadra sense codes taken from Apple Technical Note HW26:
@@ -449,6 +456,32 @@ static void macfb_update_display(void *opaque)
     macfb_draw_graphic(s);
 }
 
+static void macfb_update_irq(MacfbState *s)
+{
+    uint32_t irq_state = s->irq_state & s->irq_mask;
+
+    if (irq_state) {
+        qemu_irq_raise(s->irq);
+    } else {
+        qemu_irq_lower(s->irq);
+    }
+}
+
+static void macfb_vbl_timer(void *opaque)
+{
+    MacfbState *s = opaque;
+    int64_t next_vbl;
+
+    s->irq_state |= DAFB_INTR_VBL;
+    macfb_update_irq(s);
+
+    /* 60 Hz irq */
+    next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                DAFB_INTR_VBL_PERIOD_NS) /
+                DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
+    timer_mod(s->vbl_timer, next_vbl);
+}
+
 static void macfb_reset(MacfbState *s)
 {
     int i;
@@ -477,6 +510,9 @@ static uint64_t macfb_ctrl_read(void *opaque,
     case DAFB_MODE_CTRL2:
         val = s->regs[addr >> 2];
         break;
+    case DAFB_INTR_STAT:
+        val = s->irq_state;
+        break;
     case DAFB_MODE_SENSE:
         val = macfb_sense_read(s);
         break;
@@ -492,6 +528,8 @@ static void macfb_ctrl_write(void *opaque,
                              unsigned int size)
 {
     MacfbState *s = opaque;
+    int64_t next_vbl;
+
     switch (addr) {
     case DAFB_MODE_VADDR1:
     case DAFB_MODE_VADDR2:
@@ -507,8 +545,25 @@ static void macfb_ctrl_write(void *opaque,
     case DAFB_MODE_SENSE:
         macfb_sense_write(s, val);
         break;
+    case DAFB_INTR_MASK:
+        s->irq_mask = val;
+        if (val & DAFB_INTR_VBL) {
+            next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                        DAFB_INTR_VBL_PERIOD_NS) /
+                        DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
+            timer_mod(s->vbl_timer, next_vbl);
+        } else {
+            timer_del(s->vbl_timer);
+        }
+        break;
+    case DAFB_INTR_CLEAR:
+        s->irq_state &= ~DAFB_INTR_VBL;
+        macfb_update_irq(s);
+        break;
     case DAFB_RESET:
         s->palette_current = 0;
+        s->irq_state &= ~DAFB_INTR_VBL;
+        macfb_update_irq(s);
         break;
     case DAFB_LUT:
         s->color_palette[s->palette_current++] = val;
@@ -586,6 +641,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
     memory_region_set_coalescing(&s->mem_vram);
 
+    s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
     macfb_update_mode(s);
 }
 
@@ -601,6 +657,16 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
 
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
+
+    qdev_init_gpio_out(dev, &ms->irq, 1);
+}
+
+static void macfb_nubus_set_irq(void *opaque, int n, int level)
+{
+    MacfbNubusState *s = NUBUS_MACFB(opaque);
+    NubusDevice *nd = NUBUS_DEVICE(s);
+
+    nubus_set_irq(nd, level);
 }
 
 static void macfb_nubus_realize(DeviceState *dev, Error **errp)
@@ -622,6 +688,19 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
 
     memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
     memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
+
+    ms->irq = qemu_allocate_irq(macfb_nubus_set_irq, s, 0);
+}
+
+static void macfb_nubus_unrealize(DeviceState *dev)
+{
+    MacfbNubusState *s = NUBUS_MACFB(dev);
+    MacfbNubusDeviceClass *ndc = NUBUS_MACFB_GET_CLASS(dev);
+    MacfbState *ms = &s->macfb;
+
+    ndc->parent_unrealize(dev);
+
+    qemu_free_irq(ms->irq);
 }
 
 static void macfb_sysbus_reset(DeviceState *d)
@@ -672,6 +751,8 @@ static void macfb_nubus_class_init(ObjectClass *klass, void *data)
 
     device_class_set_parent_realize(dc, macfb_nubus_realize,
                                     &ndc->parent_realize);
+    device_class_set_parent_unrealize(dc, macfb_nubus_unrealize,
+                                      &ndc->parent_unrealize);
     dc->desc = "Nubus Macintosh framebuffer";
     dc->reset = macfb_nubus_reset;
     dc->vmsd = &vmstate_macfb;
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index 0aff0d84d2..e52775aa21 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -14,7 +14,9 @@
 #define MACFB_H
 
 #include "exec/memory.h"
+#include "hw/irq.h"
 #include "ui/console.h"
+#include "qemu/timer.h"
 #include "qom/object.h"
 
 typedef enum  {
@@ -63,6 +65,11 @@ typedef struct MacfbState {
 
     uint32_t regs[MACFB_NUM_REGS];
     MacFbMode *mode;
+
+    uint32_t irq_state;
+    uint32_t irq_mask;
+    QEMUTimer *vbl_timer;
+    qemu_irq irq;
 } MacfbState;
 
 #define TYPE_MACFB "sysbus-macfb"
@@ -81,6 +88,7 @@ struct MacfbNubusDeviceClass {
     DeviceClass parent_class;
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
 };
 
 
-- 
2.20.1

[PATCH 12/12] q800: wire macfb IRQ to separate video interrupt on VIA2

[Mark Cave-Ayland]

Whilst the in-built Quadra 800 framebuffer exists within the Nubus address
space for slot 9, it has its own dedicated interrupt on VIA2. Force the
macfb device to occupy slot 9 in the q800 machine and wire its IRQ to the
separate video interrupt since this is what is expected by the MacOS
interrupt handler.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index df3fd3711e..fd4855047e 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -407,8 +407,10 @@ static void q800_init(MachineState *machine)
                     MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
                     MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
-
-    for (i = 0; i < VIA2_NUBUS_IRQ_NB; i++) {
+    qdev_connect_gpio_out(dev, 9,
+                          qdev_get_gpio_in_named(via2_dev, "nubus-irq",
+                          VIA2_NUBUS_IRQ_INTVIDEO));
+    for (i = 1; i < VIA2_NUBUS_IRQ_NB; i++) {
         qdev_connect_gpio_out(dev, 9 + i,
                               qdev_get_gpio_in_named(via2_dev, "nubus-irq",
                                                      VIA2_NUBUS_IRQ_9 + i));
@@ -419,6 +421,7 @@ static void q800_init(MachineState *machine)
     /* framebuffer in nubus slot #9 */
 
     dev = qdev_new(TYPE_NUBUS_MACFB);
+    qdev_prop_set_uint32(dev, "slot", 9);
     qdev_prop_set_uint32(dev, "width", graphic_width);
     qdev_prop_set_uint32(dev, "height", graphic_height);
     qdev_prop_set_uint8(dev, "depth", graphic_depth);
-- 
2.20.1

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/display/macfb.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c

There's one more in macfb_common_realize() after:

memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram", MACFB_VRAM_SIZE, errp);

otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>


> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>     MacfbState *ms = &s->macfb;
>
>     macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
> }
> @@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>     MacfbState *ms = &s->macfb;
>
>     ndc->parent_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
>
>     macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>     memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>     memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
> }
>

Re: [PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
> During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
> memory region used for the framebuffer but the owner object reference is
> incorrect since MacFbState is a typedef and not a QOM type.
>
> Change the memory region owner to be the corresponding DeviceState to fix the
> issue and prevent random crashes during macfb_common_realize().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/display/macfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 2b747a8de8..815870f2fe 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -365,7 +365,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>     memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
>                           "macfb-ctrl", 0x1000);
>
> -    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram",
> +    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
>                                      MACFB_VRAM_SIZE, errp);
>     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>

Re: [PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[BALATON Zoltan]

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
> Currently macfb_common_realize() defines the framebuffer RAM memory region as
> being non-migrateable but then immediately registers it for migration. Replace
> memory_region_init_ram_nomigrate() with memory_region_init_ram() which is clearer
> and does exactly the same thing.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/display/macfb.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index f4e789d0d7..e86fbbbb64 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -368,11 +368,10 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>     memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
>                           "macfb-ctrl", 0x1000);
>
> -    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
> -                                     MACFB_VRAM_SIZE, errp);
> +    memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
> +                           MACFB_VRAM_SIZE, errp);
>     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
> -    vmstate_register_ram(&s->mem_vram, dev);
>     memory_region_set_coalescing(&s->mem_vram);
> }
>
>

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }

See a related discussion:
https://lore.kernel.org/qemu-devel/87bl47ll9l.fsf@dusky.pond.sub.org/

Re: [PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
> memory region used for the framebuffer but the owner object reference is
> incorrect since MacFbState is a typedef and not a QOM type.
> 
> Change the memory region owner to be the corresponding DeviceState to fix the
> issue and prevent random crashes during macfb_common_realize().
> 

Fixes: 8ac919a0654 ("hw/m68k: add Nubus macfb video card")
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[Philippe Mathieu-Daudé]

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> Currently macfb_common_realize() defines the framebuffer RAM memory region as
> being non-migrateable but then immediately registers it for migration. Replace
> memory_region_init_ram_nomigrate() with memory_region_init_ram() which is clearer
> and does exactly the same thing.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 05/12] macfb: add trace events for reading and writing the control registers

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c      | 8 +++++++-
>  hw/display/trace-events | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)

> @@ -289,7 +290,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
>                                  hwaddr addr,
>                                  unsigned int size)
>  {

> +# macfb.c
> +macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
> +macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
> 

'size' is unsigned, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Since the available resolutions and colour depths are determined by the attached
> display type, add a qdev property to allow the display type to be specified.
> 
> The main resolutions of interest are high resolution 1152x870 with 8-bit colour
> and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
> to allow high resolution mode if specified and otherwise fall back to SVGA.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c         | 6 +++++-
>  hw/m68k/q800.c             | 5 +++++
>  include/hw/display/macfb.h | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 5c95aa4a11..023d1f0cd1 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
>      MacFbSense *macfb_sense;
>      uint8_t sense;
>  

What about:

       assert(s->type < ARRAY_SIZE(macfb_sense_table));

> -    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
> +    macfb_sense = &macfb_sense_table[s->type];

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 12/12] q800: wire macfb IRQ to separate video interrupt on VIA2

[Philippe Mathieu-Daudé]

On 10/2/21 13:00, Mark Cave-Ayland wrote:
> Whilst the in-built Quadra 800 framebuffer exists within the Nubus address
> space for slot 9, it has its own dedicated interrupt on VIA2. Force the
> macfb device to occupy slot 9 in the q800 machine and wire its IRQ to the
> separate video interrupt since this is what is expected by the MacOS
> interrupt handler.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[Laurent Vivier]

Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
>  }
> @@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      ndc->parent_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>      memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
>  }
> 

Perhaps as suggested by Philippe you change macfb_common_realize() to return a value and an error?

Otherwise:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 02/12] macfb: fix invalid object reference in macfb_common_realize()

[Laurent Vivier]

Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
> During realize memory_region_init_ram_nomigrate() is used to initialise the RAM
> memory region used for the framebuffer but the owner object reference is
> incorrect since MacFbState is a typedef and not a QOM type.
> 
> Change the memory region owner to be the corresponding DeviceState to fix the
> issue and prevent random crashes during macfb_common_realize().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 2b747a8de8..815870f2fe 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -365,7 +365,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>      memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
>                            "macfb-ctrl", 0x1000);
>  
> -    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram",
> +    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
>                                       MACFB_VRAM_SIZE, errp);
>      s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>      s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 03/12] macfb: fix overflow of color_palette array

[Laurent Vivier]

Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
> The palette_current index counter has a maximum size of 256 * 3 to cover a full
> color palette of 256 RGB entries. Linux assumes that the palette_current index
> wraps back around to zero after writing 256 RGB entries so ensure that
> palette_current is reset at this point to prevent data corruption within
> MacfbState.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 815870f2fe..f4e789d0d7 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -307,6 +307,9 @@ static void macfb_ctrl_write(void *opaque,
>          if (s->palette_current % 3) {
>              macfb_invalidate_display(s);
>          }
> +        if (s->palette_current >= sizeof(s->color_palette)) {
> +            s->palette_current = 0;
> +        }
>          break;
>      }
>  }
> 

What about "s->palette_current = (s->palette_current + 1) % sizeof(s->color_palette)"?

Thanks,
Laurent

Re: [PATCH 04/12] macfb: use memory_region_init_ram() in macfb_common_realize() for the framebuffer

[Laurent Vivier]

Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
> Currently macfb_common_realize() defines the framebuffer RAM memory region as
> being non-migrateable but then immediately registers it for migration. Replace
> memory_region_init_ram_nomigrate() with memory_region_init_ram() which is clearer
> and does exactly the same thing.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index f4e789d0d7..e86fbbbb64 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -368,11 +368,10 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>      memory_region_init_io(&s->mem_ctrl, OBJECT(dev), &macfb_ctrl_ops, s,
>                            "macfb-ctrl", 0x1000);
>  
> -    memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(dev), "macfb-vram",
> -                                     MACFB_VRAM_SIZE, errp);
> +    memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
> +                           MACFB_VRAM_SIZE, errp);
>      s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>      s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
> -    vmstate_register_ram(&s->mem_vram, dev);
>      memory_region_set_coalescing(&s->mem_vram);
>  }
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 05/12] macfb: add trace events for reading and writing the control registers

[Laurent Vivier]

Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c      | 8 +++++++-
>  hw/display/trace-events | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index e86fbbbb64..62c2727a5b 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -20,6 +20,7 @@
>  #include "qapi/error.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> +#include "trace.h"
>  
>  #define VIDEO_BASE 0x00001000
>  #define DAFB_BASE  0x00800000
> @@ -289,7 +290,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
>                                  hwaddr addr,
>                                  unsigned int size)
>  {
> -    return 0;
> +    uint64_t val = 0;
> +
> +    trace_macfb_ctrl_read(addr, val, size);
> +    return val;
>  }
>  
>  static void macfb_ctrl_write(void *opaque,
> @@ -312,6 +316,8 @@ static void macfb_ctrl_write(void *opaque,
>          }
>          break;
>      }
> +
> +    trace_macfb_ctrl_write(addr, val, size);
>  }
>  
>  static const MemoryRegionOps macfb_ctrl_ops = {
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index f03f6655bc..be1353e8e7 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -167,3 +167,7 @@ sm501_disp_ctrl_read(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
>  sm501_disp_ctrl_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
>  sm501_2d_engine_read(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
>  sm501_2d_engine_write(uint32_t addr, uint32_t val) "addr=0x%x, val=0x%x"
> +
> +# macfb.c
> +macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
> +macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
> 

As suggested by Philippe: size is unsigned

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 06/12] macfb: implement mode sense to allow display type to be detected

[Laurent Vivier]

Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> The MacOS toolbox ROM uses the monitor sense to detect the display type and then
> offer a fixed set of resolutions and colour depths accordingly. Implement the
> monitor sense using information found in Apple Technical Note HW26: "Macintosh
> Quadra Built-In Video" along with some local experiments.
> 
> Since the default configuration is 640 x 480 with 8-bit colour then hardcode
> the sense register to return MACFB_DISPLAY_VGA for now.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c         | 117 ++++++++++++++++++++++++++++++++++++-
>  hw/display/trace-events    |   2 +
>  include/hw/display/macfb.h |  20 +++++++
>  3 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 62c2727a5b..5c95aa4a11 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -28,8 +28,66 @@
>  #define MACFB_PAGE_SIZE 4096
>  #define MACFB_VRAM_SIZE (4 * MiB)
>  
> -#define DAFB_RESET      0x200
> -#define DAFB_LUT        0x213
> +#define DAFB_MODE_SENSE     0x1c
> +#define DAFB_RESET          0x200
> +#define DAFB_LUT            0x213
> +
> +
> +/*
> + * Quadra sense codes taken from Apple Technical Note HW26:
> + * "Macintosh Quadra Built-In Video". The sense codes and

https://developer.apple.com/library/archive/technotes/hw/hw_26.html

> + * extended sense codes have different meanings:
> + *
> + * Sense:
> + *    bit 2: SENSE2 (pin 10)
> + *    bit 1: SENSE1 (pin 7)
> + *    bit 0: SENSE0 (pin 4)
> + *
> + * 0 = pin tied to ground
> + * 1 = pin unconnected
> + *
> + * Extended Sense:
> + *    bit 2: pins 4-10
> + *    bit 1: pins 10-7
> + *    bit 0: pins 7-4
> + *
> + * 0 = pins tied together
> + * 1 = pins unconnected
> + *
> + * Reads from the sense register appear to be active low, i.e. a 1 indicates
> + * that the pin is tied to ground, a 0 indicates the pin is disconnected.
> + *
> + * Writes to the sense register appear to activate pulldowns i.e. a 1 enables
> + * a pulldown on a particular pin.
> + *
> + * The MacOS toolbox appears to use a series of reads and writes to first
> + * determine if extended sense is to be used, and then check which pins are
> + * tied together in order to determine the display type.
> + */
> +
> +typedef struct MacFbSense {
> +    uint8_t type;
> +    uint8_t sense;
> +    uint8_t ext_sense;
> +} MacFbSense;
> +
> +static MacFbSense macfb_sense_table[] = {
> +    { MACFB_DISPLAY_APPLE_21_COLOR, 0x0, 0 },
> +    { MACFB_DISPLAY_APPLE_PORTRAIT, 0x1, 0 },
> +    { MACFB_DISPLAY_APPLE_12_RGB, 0x2, 0 },
> +    { MACFB_DISPLAY_APPLE_2PAGE_MONO, 0x3, 0 },
> +    { MACFB_DISPLAY_NTSC_UNDERSCAN, 0x4, 0 },
> +    { MACFB_DISPLAY_NTSC_OVERSCAN, 0x4, 0 },
> +    { MACFB_DISPLAY_APPLE_12_MONO, 0x6, 0 },
> +    { MACFB_DISPLAY_APPLE_13_RGB, 0x6, 0 },
> +    { MACFB_DISPLAY_16_COLOR, 0x7, 0x3 },
> +    { MACFB_DISPLAY_PAL1_UNDERSCAN, 0x7, 0x0 },
> +    { MACFB_DISPLAY_PAL1_OVERSCAN, 0x7, 0x0 },
> +    { MACFB_DISPLAY_PAL2_UNDERSCAN, 0x7, 0x6 },
> +    { MACFB_DISPLAY_PAL2_OVERSCAN, 0x7, 0x6 },
> +    { MACFB_DISPLAY_VGA, 0x7, 0x5 },
> +    { MACFB_DISPLAY_SVGA, 0x7, 0x5 },

Perhaps it could be interesting to also have the "no external monitor" entry?
But generally not to have monitor prevents the boot of MacOS.

...

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Laurent Vivier]

Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> Since the available resolutions and colour depths are determined by the attached
> display type, add a qdev property to allow the display type to be specified.
> 
> The main resolutions of interest are high resolution 1152x870 with 8-bit colour
> and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
> to allow high resolution mode if specified and otherwise fall back to SVGA.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c         | 6 +++++-
>  hw/m68k/q800.c             | 5 +++++
>  include/hw/display/macfb.h | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 5c95aa4a11..023d1f0cd1 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
>      MacFbSense *macfb_sense;
>      uint8_t sense;
>  
> -    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
> +    macfb_sense = &macfb_sense_table[s->type];
>      if (macfb_sense->sense == 0x7) {
>          /* Extended sense */
>          sense = 0;
> @@ -545,6 +545,8 @@ static Property macfb_sysbus_properties[] = {
>      DEFINE_PROP_UINT32("width", MacfbSysBusState, macfb.width, 640),
>      DEFINE_PROP_UINT32("height", MacfbSysBusState, macfb.height, 480),
>      DEFINE_PROP_UINT8("depth", MacfbSysBusState, macfb.depth, 8),
> +    DEFINE_PROP_UINT8("display", MacfbSysBusState, macfb.type,
> +                      MACFB_DISPLAY_VGA),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -552,6 +554,8 @@ static Property macfb_nubus_properties[] = {
>      DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
>      DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
>      DEFINE_PROP_UINT8("depth", MacfbNubusState, macfb.depth, 8),
> +    DEFINE_PROP_UINT8("display", MacfbNubusState, macfb.type,
> +                      MACFB_DISPLAY_VGA),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 09b3366024..5223b880bc 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -421,6 +421,11 @@ static void q800_init(MachineState *machine)
>      qdev_prop_set_uint32(dev, "width", graphic_width);
>      qdev_prop_set_uint32(dev, "height", graphic_height);
>      qdev_prop_set_uint8(dev, "depth", graphic_depth);
> +    if (graphic_width == 1152 && graphic_height == 870 && graphic_depth == 8) {
> +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_APPLE_21_COLOR);
> +    } else {
> +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_VGA);
> +    }
>      qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
>  
>      cs = CPU(cpu);
> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> index febf4ce0e8..e95a97ebdc 100644
> --- a/include/hw/display/macfb.h
> +++ b/include/hw/display/macfb.h
> @@ -46,6 +46,7 @@ typedef struct MacfbState {
>      uint8_t color_palette[256 * 3];
>      uint32_t width, height; /* in pixels */
>      uint8_t depth;
> +    uint8_t type;
>  
>      uint32_t sense;
>  } MacfbState;
> 

I think the display modes should be documentend somewhere to be directly usable by the user and get
ride of the graphic_XXX variables (and -g).

Perhaps it could also be merged with the previous one.

Thanks,
Laurent

Re: [PATCH 12/12] q800: wire macfb IRQ to separate video interrupt on VIA2

[Laurent Vivier]

Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> Whilst the in-built Quadra 800 framebuffer exists within the Nubus address
> space for slot 9, it has its own dedicated interrupt on VIA2. Force the
> macfb device to occupy slot 9 in the q800 machine and wire its IRQ to the
> separate video interrupt since this is what is expected by the MacOS
> interrupt handler.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index df3fd3711e..fd4855047e 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -407,8 +407,10 @@ static void q800_init(MachineState *machine)
>                      MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
>                      MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
> -
> -    for (i = 0; i < VIA2_NUBUS_IRQ_NB; i++) {
> +    qdev_connect_gpio_out(dev, 9,
> +                          qdev_get_gpio_in_named(via2_dev, "nubus-irq",
> +                          VIA2_NUBUS_IRQ_INTVIDEO));
> +    for (i = 1; i < VIA2_NUBUS_IRQ_NB; i++) {
>          qdev_connect_gpio_out(dev, 9 + i,
>                                qdev_get_gpio_in_named(via2_dev, "nubus-irq",
>                                                       VIA2_NUBUS_IRQ_9 + i));
> @@ -419,6 +421,7 @@ static void q800_init(MachineState *machine)
>      /* framebuffer in nubus slot #9 */
>  
>      dev = qdev_new(TYPE_NUBUS_MACFB);
> +    qdev_prop_set_uint32(dev, "slot", 9);
>      qdev_prop_set_uint32(dev, "width", graphic_width);
>      qdev_prop_set_uint32(dev, "height", graphic_height);
>      qdev_prop_set_uint8(dev, "depth", graphic_depth);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 06/12] macfb: implement mode sense to allow display type to be detected

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

On Mon, 4 Oct 2021, Laurent Vivier wrote:
> Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
>> The MacOS toolbox ROM uses the monitor sense to detect the display type and then
>> offer a fixed set of resolutions and colour depths accordingly. Implement the
>> monitor sense using information found in Apple Technical Note HW26: "Macintosh
>> Quadra Built-In Video" along with some local experiments.
>>
>> Since the default configuration is 640 x 480 with 8-bit colour then hardcode
>> the sense register to return MACFB_DISPLAY_VGA for now.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/display/macfb.c         | 117 ++++++++++++++++++++++++++++++++++++-
>>  hw/display/trace-events    |   2 +
>>  include/hw/display/macfb.h |  20 +++++++
>>  3 files changed, 137 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 62c2727a5b..5c95aa4a11 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -28,8 +28,66 @@
>>  #define MACFB_PAGE_SIZE 4096
>>  #define MACFB_VRAM_SIZE (4 * MiB)
>>
>> -#define DAFB_RESET      0x200
>> -#define DAFB_LUT        0x213
>> +#define DAFB_MODE_SENSE     0x1c
>> +#define DAFB_RESET          0x200
>> +#define DAFB_LUT            0x213
>> +
>> +
>> +/*
>> + * Quadra sense codes taken from Apple Technical Note HW26:
>> + * "Macintosh Quadra Built-In Video". The sense codes and
>
> https://developer.apple.com/library/archive/technotes/hw/hw_26.html

URLs may change or go away so I think it's better to reference by title in 
comments, then one can find it by that whereas a stale URL is not much 
help a few years from now.

Regards,
BALATON Zoltan

>> + * extended sense codes have different meanings:
>> + *
>> + * Sense:
>> + *    bit 2: SENSE2 (pin 10)
>> + *    bit 1: SENSE1 (pin 7)
>> + *    bit 0: SENSE0 (pin 4)
>> + *
>> + * 0 = pin tied to ground
>> + * 1 = pin unconnected
>> + *
>> + * Extended Sense:
>> + *    bit 2: pins 4-10
>> + *    bit 1: pins 10-7
>> + *    bit 0: pins 7-4
>> + *
>> + * 0 = pins tied together
>> + * 1 = pins unconnected
>> + *
>> + * Reads from the sense register appear to be active low, i.e. a 1 indicates
>> + * that the pin is tied to ground, a 0 indicates the pin is disconnected.
>> + *
>> + * Writes to the sense register appear to activate pulldowns i.e. a 1 enables
>> + * a pulldown on a particular pin.
>> + *
>> + * The MacOS toolbox appears to use a series of reads and writes to first
>> + * determine if extended sense is to be used, and then check which pins are
>> + * tied together in order to determine the display type.
>> + */
>> +
>> +typedef struct MacFbSense {
>> +    uint8_t type;
>> +    uint8_t sense;
>> +    uint8_t ext_sense;
>> +} MacFbSense;
>> +
>> +static MacFbSense macfb_sense_table[] = {
>> +    { MACFB_DISPLAY_APPLE_21_COLOR, 0x0, 0 },
>> +    { MACFB_DISPLAY_APPLE_PORTRAIT, 0x1, 0 },
>> +    { MACFB_DISPLAY_APPLE_12_RGB, 0x2, 0 },
>> +    { MACFB_DISPLAY_APPLE_2PAGE_MONO, 0x3, 0 },
>> +    { MACFB_DISPLAY_NTSC_UNDERSCAN, 0x4, 0 },
>> +    { MACFB_DISPLAY_NTSC_OVERSCAN, 0x4, 0 },
>> +    { MACFB_DISPLAY_APPLE_12_MONO, 0x6, 0 },
>> +    { MACFB_DISPLAY_APPLE_13_RGB, 0x6, 0 },
>> +    { MACFB_DISPLAY_16_COLOR, 0x7, 0x3 },
>> +    { MACFB_DISPLAY_PAL1_UNDERSCAN, 0x7, 0x0 },
>> +    { MACFB_DISPLAY_PAL1_OVERSCAN, 0x7, 0x0 },
>> +    { MACFB_DISPLAY_PAL2_UNDERSCAN, 0x7, 0x6 },
>> +    { MACFB_DISPLAY_PAL2_OVERSCAN, 0x7, 0x6 },
>> +    { MACFB_DISPLAY_VGA, 0x7, 0x5 },
>> +    { MACFB_DISPLAY_SVGA, 0x7, 0x5 },
>
> Perhaps it could be interesting to also have the "no external monitor" entry?
> But generally not to have monitor prevents the boot of MacOS.
>
> ...
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>
>

Re: [PATCH 06/12] macfb: implement mode sense to allow display type to be detected

[Laurent Vivier]

Le 04/10/2021 à 12:32, BALATON Zoltan a écrit :
> On Mon, 4 Oct 2021, Laurent Vivier wrote:
>> Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
>>> The MacOS toolbox ROM uses the monitor sense to detect the display type and then
>>> offer a fixed set of resolutions and colour depths accordingly. Implement the
>>> monitor sense using information found in Apple Technical Note HW26: "Macintosh
>>> Quadra Built-In Video" along with some local experiments.
>>>
>>> Since the default configuration is 640 x 480 with 8-bit colour then hardcode
>>> the sense register to return MACFB_DISPLAY_VGA for now.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/display/macfb.c         | 117 ++++++++++++++++++++++++++++++++++++-
>>>  hw/display/trace-events    |   2 +
>>>  include/hw/display/macfb.h |  20 +++++++
>>>  3 files changed, 137 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>>> index 62c2727a5b..5c95aa4a11 100644
>>> --- a/hw/display/macfb.c
>>> +++ b/hw/display/macfb.c
>>> @@ -28,8 +28,66 @@
>>>  #define MACFB_PAGE_SIZE 4096
>>>  #define MACFB_VRAM_SIZE (4 * MiB)
>>>
>>> -#define DAFB_RESET      0x200
>>> -#define DAFB_LUT        0x213
>>> +#define DAFB_MODE_SENSE     0x1c
>>> +#define DAFB_RESET          0x200
>>> +#define DAFB_LUT            0x213
>>> +
>>> +
>>> +/*
>>> + * Quadra sense codes taken from Apple Technical Note HW26:
>>> + * "Macintosh Quadra Built-In Video". The sense codes and
>>
>> https://developer.apple.com/library/archive/technotes/hw/hw_26.html
> 
> URLs may change or go away so I think it's better to reference by title in comments, then one can
> find it by that whereas a stale URL is not much help a few years from now.

This URL is very stable (hosted by Apple for 30 years now...) and providing it could help to find a
copy of the document in an internet archive. I had some difficulties to find it using the title or
the TN number.

Thanks,
Laurent

Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Laurent Vivier]

On 04/10/2021 11:24, Laurent Vivier wrote:
> Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
>> Since the available resolutions and colour depths are determined by the attached
>> display type, add a qdev property to allow the display type to be specified.
>>
>> The main resolutions of interest are high resolution 1152x870 with 8-bit colour
>> and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
>> to allow high resolution mode if specified and otherwise fall back to SVGA.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c         | 6 +++++-
>>   hw/m68k/q800.c             | 5 +++++
>>   include/hw/display/macfb.h | 1 +
>>   3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 5c95aa4a11..023d1f0cd1 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
>>       MacFbSense *macfb_sense;
>>       uint8_t sense;
>>   
>> -    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
>> +    macfb_sense = &macfb_sense_table[s->type];
>>       if (macfb_sense->sense == 0x7) {
>>           /* Extended sense */
>>           sense = 0;
>> @@ -545,6 +545,8 @@ static Property macfb_sysbus_properties[] = {
>>       DEFINE_PROP_UINT32("width", MacfbSysBusState, macfb.width, 640),
>>       DEFINE_PROP_UINT32("height", MacfbSysBusState, macfb.height, 480),
>>       DEFINE_PROP_UINT8("depth", MacfbSysBusState, macfb.depth, 8),
>> +    DEFINE_PROP_UINT8("display", MacfbSysBusState, macfb.type,
>> +                      MACFB_DISPLAY_VGA),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -552,6 +554,8 @@ static Property macfb_nubus_properties[] = {
>>       DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
>>       DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
>>       DEFINE_PROP_UINT8("depth", MacfbNubusState, macfb.depth, 8),
>> +    DEFINE_PROP_UINT8("display", MacfbNubusState, macfb.type,
>> +                      MACFB_DISPLAY_VGA),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index 09b3366024..5223b880bc 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -421,6 +421,11 @@ static void q800_init(MachineState *machine)
>>       qdev_prop_set_uint32(dev, "width", graphic_width);
>>       qdev_prop_set_uint32(dev, "height", graphic_height);
>>       qdev_prop_set_uint8(dev, "depth", graphic_depth);
>> +    if (graphic_width == 1152 && graphic_height == 870 && graphic_depth == 8) {
>> +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_APPLE_21_COLOR);
>> +    } else {
>> +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_VGA);
>> +    }
>>       qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
>>   
>>       cs = CPU(cpu);
>> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
>> index febf4ce0e8..e95a97ebdc 100644
>> --- a/include/hw/display/macfb.h
>> +++ b/include/hw/display/macfb.h
>> @@ -46,6 +46,7 @@ typedef struct MacfbState {
>>       uint8_t color_palette[256 * 3];
>>       uint32_t width, height; /* in pixels */
>>       uint8_t depth;
>> +    uint8_t type;
>>   
>>       uint32_t sense;
>>   } MacfbState;
>>
> 
> I think the display modes should be documentend somewhere to be directly usable by the user and get
> ride of the graphic_XXX variables (and -g).

By reading following patch I can see it's not really needed anymore, so:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 08/12] macfb: add common monitor modes supported by the MacOS toolbox ROM

[Laurent Vivier]

On 02/10/2021 13:00, Mark Cave-Ayland wrote:
> The monitor modes table is found by experimenting with the Monitors Control
> Panel in MacOS and analysing the reads/writes. From this it can be found that
> the mode is controlled by writes to the DAFB_MODE_CTRL1 and DAFB_MODE_CTRL2
> registers.
> 
> Implement the first block of DAFB registers as a register array including the
> existing sense register, the newly discovered control registers above, and also
> the DAFB_MODE_VADDR1 and DAFB_MODE_VADDR2 registers which are used by NetBSD to
> determine the current video mode.
> 
> These experiments also show that the offset of the start of video RAM and the
> stride can change depending upon the monitor mode, so update macfb_draw_graphic()
> and both the BI_MAC_VADDR and BI_MAC_VROW bootinfo for the q800 machine
> accordingly.
> 
> Finally update macfb_common_realize() so that only the resolution and depth
> supported by the display type can be specified on the command line.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c         | 125 ++++++++++++++++++++++++++++++++-----
>   hw/display/trace-events    |   1 +
>   hw/m68k/q800.c             |  11 ++--
>   include/hw/display/macfb.h |  16 ++++-
>   4 files changed, 132 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 023d1f0cd1..6a69334565 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -22,12 +22,16 @@
>   #include "migration/vmstate.h"
>   #include "trace.h"
>   
> -#define VIDEO_BASE 0x00001000
> +#define VIDEO_BASE 0x0
>   #define DAFB_BASE  0x00800000
>   
>   #define MACFB_PAGE_SIZE 4096
>   #define MACFB_VRAM_SIZE (4 * MiB)
>   
> +#define DAFB_MODE_VADDR1    0x0
> +#define DAFB_MODE_VADDR2    0x4
> +#define DAFB_MODE_CTRL1     0x8
> +#define DAFB_MODE_CTRL2     0xc
>   #define DAFB_MODE_SENSE     0x1c
>   #define DAFB_RESET          0x200
>   #define DAFB_LUT            0x213
> @@ -89,6 +93,22 @@ static MacFbSense macfb_sense_table[] = {
>       { MACFB_DISPLAY_SVGA, 0x7, 0x5 },
>   };
>   
> +static MacFbMode macfb_mode_table[] = {
> +    { MACFB_DISPLAY_VGA, 1, 0x100, 0x71e, 640, 480, 0x400, 0x1000 },
> +    { MACFB_DISPLAY_VGA, 2, 0x100, 0x70e, 640, 480, 0x400, 0x1000 },
> +    { MACFB_DISPLAY_VGA, 4, 0x100, 0x706, 640, 480, 0x400, 0x1000 },
> +    { MACFB_DISPLAY_VGA, 8, 0x100, 0x702, 640, 480, 0x400, 0x1000 },
> +    { MACFB_DISPLAY_VGA, 24, 0x100, 0x7ff, 640, 480, 0x1000, 0x1000 },
> +    { MACFB_DISPLAY_VGA, 1, 0xd0 , 0x70e, 800, 600, 0x340, 0xe00 },
> +    { MACFB_DISPLAY_VGA, 2, 0xd0 , 0x706, 800, 600, 0x340, 0xe00 },
> +    { MACFB_DISPLAY_VGA, 4, 0xd0 , 0x702, 800, 600, 0x340, 0xe00 },
> +    { MACFB_DISPLAY_VGA, 8, 0xd0,  0x700, 800, 600, 0x340, 0xe00 },
> +    { MACFB_DISPLAY_VGA, 24, 0x340, 0x100, 800, 600, 0xd00, 0xe00 },
> +    { MACFB_DISPLAY_APPLE_21_COLOR, 1, 0x90, 0x506, 1152, 870, 0x240, 0x80 },
> +    { MACFB_DISPLAY_APPLE_21_COLOR, 2, 0x90, 0x502, 1152, 870, 0x240, 0x80 },
> +    { MACFB_DISPLAY_APPLE_21_COLOR, 4, 0x90, 0x500, 1152, 870, 0x240, 0x80 },
> +    { MACFB_DISPLAY_APPLE_21_COLOR, 8, 0x120, 0x5ff, 1152, 870, 0x480, 0x80 },
> +};
>   
>   typedef void macfb_draw_line_func(MacfbState *s, uint8_t *d, uint32_t addr,
>                                     int width);
> @@ -246,7 +266,7 @@ static void macfb_draw_graphic(MacfbState *s)
>       ram_addr_t page;
>       uint32_t v = 0;
>       int y, ymin;
> -    int macfb_stride = (s->depth * s->width + 7) / 8;
> +    int macfb_stride = s->mode->stride;
>       macfb_draw_line_func *macfb_draw_line;
>   
>       switch (s->depth) {
> @@ -278,7 +298,7 @@ static void macfb_draw_graphic(MacfbState *s)
>                                                DIRTY_MEMORY_VGA);
>   
>       ymin = -1;
> -    page = 0;
> +    page = s->mode->offset;
>       for (y = 0; y < s->height; y++, page += macfb_stride) {
>           if (macfb_check_dirty(s, snap, page, macfb_stride)) {
>               uint8_t *data_display;
> @@ -322,25 +342,26 @@ static uint32_t macfb_sense_read(MacfbState *s)
>           sense = 0;
>           if (!(macfb_sense->ext_sense & 1)) {
>               /* Pins 7-4 together */
> -            if (~s->sense & 3) {
> -                sense = (~s->sense & 7) | 3;
> +            if (~s->regs[DAFB_MODE_SENSE >> 2] & 3) {
> +                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 3;
>               }
>           }
>           if (!(macfb_sense->ext_sense & 2)) {
>               /* Pins 10-7 together */
> -            if (~s->sense & 6) {
> -                sense = (~s->sense & 7) | 6;
> +            if (~s->regs[DAFB_MODE_SENSE >> 2] & 6) {
> +                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 6;
>               }
>           }
>           if (!(macfb_sense->ext_sense & 4)) {
>               /* Pins 4-10 together */
> -            if (~s->sense & 5) {
> -                sense = (~s->sense & 7) | 5;
> +            if (~s->regs[DAFB_MODE_SENSE >> 2] & 5) {
> +                sense = (~s->regs[DAFB_MODE_SENSE >> 2] & 7) | 5;
>               }
>           }
>       } else {
>           /* Normal sense */
> -        sense = (~macfb_sense->sense & 7) | (~s->sense & 7);
> +        sense = (~macfb_sense->sense & 7) |
> +                (~s->regs[DAFB_MODE_SENSE >> 2] & 7);
>       }
>   
>       trace_macfb_sense_read(sense);
> @@ -349,12 +370,64 @@ static uint32_t macfb_sense_read(MacfbState *s)
>   
>   static void macfb_sense_write(MacfbState *s, uint32_t val)
>   {
> -    s->sense = val;
> +    s->regs[DAFB_MODE_SENSE >> 2] = val;
>   
>       trace_macfb_sense_write(val);
>       return;
>   }
>   
> +static void macfb_update_mode(MacfbState *s)
> +{
> +    s->width = s->mode->width;
> +    s->height = s->mode->height;
> +    s->depth = s->mode->depth;
> +
> +    trace_macfb_update_mode(s->width, s->height, s->depth);
> +    macfb_invalidate_display(s);
> +}
> +
> +static void macfb_mode_write(MacfbState *s)
> +{
> +    MacFbMode *macfb_mode;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(macfb_mode_table); i++) {
> +        macfb_mode = &macfb_mode_table[i];
> +
> +        if (s->type != macfb_mode->type) {
> +            continue;
> +        }
> +
> +        if ((s->regs[DAFB_MODE_CTRL1 >> 2] & 0xff) ==
> +             (macfb_mode->mode_ctrl1 & 0xff) &&
> +            (s->regs[DAFB_MODE_CTRL2 >> 2] & 0xff) ==
> +             (macfb_mode->mode_ctrl2 & 0xff)) {
> +            s->mode = macfb_mode;
> +            macfb_update_mode(s);
> +            break;
> +        }
> +    }
> +}
> +
> +static MacFbMode *macfb_find_mode(MacfbDisplayType display_type,
> +                                  uint16_t width, uint16_t height,
> +                                  uint8_t depth)
> +{
> +    MacFbMode *macfb_mode;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(macfb_mode_table); i++) {
> +        macfb_mode = &macfb_mode_table[i];
> +
> +        if (display_type == macfb_mode->type && width == macfb_mode->width &&
> +                height == macfb_mode->height && depth == macfb_mode->depth) {
> +            return macfb_mode;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>   static void macfb_update_display(void *opaque)
>   {
>       MacfbState *s = opaque;
> @@ -396,6 +469,12 @@ static uint64_t macfb_ctrl_read(void *opaque,
>       uint64_t val = 0;
>   
>       switch (addr) {
> +    case DAFB_MODE_VADDR1:
> +    case DAFB_MODE_VADDR2:
> +    case DAFB_MODE_CTRL1:
> +    case DAFB_MODE_CTRL2:
> +        val = s->regs[addr >> 2];
> +        break;
>       case DAFB_MODE_SENSE:
>           val = macfb_sense_read(s);
>           break;
> @@ -412,6 +491,17 @@ static void macfb_ctrl_write(void *opaque,
>   {
>       MacfbState *s = opaque;
>       switch (addr) {
> +    case DAFB_MODE_VADDR1:
> +    case DAFB_MODE_VADDR2:
> +        s->regs[addr >> 2] = val;
> +        break;
> +    case DAFB_MODE_CTRL1 ... DAFB_MODE_CTRL1 + 3:
> +    case DAFB_MODE_CTRL2 ... DAFB_MODE_CTRL2 + 3:
> +        s->regs[addr >> 2] = val;
> +        if (val) {
> +            macfb_mode_write(s);
> +        }
> +        break;
>       case DAFB_MODE_SENSE:
>           macfb_sense_write(s, val);
>           break;
> @@ -442,7 +532,7 @@ static const MemoryRegionOps macfb_ctrl_ops = {
>   
>   static int macfb_post_load(void *opaque, int version_id)
>   {
> -    macfb_invalidate_display(opaque);
> +    macfb_mode_write(opaque);
>       return 0;
>   }
>   
> @@ -455,7 +545,7 @@ static const VMStateDescription vmstate_macfb = {
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
>           VMSTATE_UINT32(palette_current, MacfbState),
> -        VMSTATE_UINT32(sense, MacfbState),
> +        VMSTATE_UINT32_ARRAY(regs, MacfbState, MACFB_NUM_REGS),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> @@ -469,9 +559,10 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>   {
>       DisplaySurface *surface;
>   
> -    if (s->depth != 1 && s->depth != 2 && s->depth != 4 && s->depth != 8 &&
> -        s->depth != 16 && s->depth != 24) {
> -        error_setg(errp, "unknown guest depth %d", s->depth);
> +    s->mode = macfb_find_mode(s->type, s->width, s->height, s->depth);
> +    if (!s->mode) {
> +        error_setg(errp, "unknown display mode: width %d, height %d, depth %d",
> +                   s->width, s->height, s->depth);
>           return;
>       }
>   
> @@ -492,6 +583,8 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>       s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>       s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>       memory_region_set_coalescing(&s->mem_vram);
> +
> +    macfb_update_mode(s);
>   }
>   
>   static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 6e378036ab..75574e5976 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -173,3 +173,4 @@ macfb_ctrl_read(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " valu
>   macfb_ctrl_write(uint64_t addr, uint64_t value, int size) "addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
>   macfb_sense_read(uint32_t value) "video sense: 0x%"PRIx32
>   macfb_sense_write(uint32_t value) "video sense: 0x%"PRIx32
> +macfb_update_mode(uint32_t width, uint32_t height, uint8_t depth) "setting mode to width %"PRId32 " height %"PRId32 " size %d"
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 5223b880bc..df3fd3711e 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -74,7 +74,7 @@
>    * is needed by the kernel to have early display and
>    * thus provided by the bootloader
>    */
> -#define VIDEO_BASE            0xf9001000
> +#define VIDEO_BASE            0xf9000000
>   
>   #define MAC_CLOCK  3686418
>   
> @@ -221,6 +221,7 @@ static void q800_init(MachineState *machine)
>       uint8_t *prom;
>       const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
>       int i, checksum;
> +    MacFbMode *macfb_mode;
>       ram_addr_t ram_size = machine->ram_size;
>       const char *kernel_filename = machine->kernel_filename;
>       const char *initrd_filename = machine->initrd_filename;
> @@ -428,6 +429,8 @@ static void q800_init(MachineState *machine)
>       }
>       qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
>   
> +    macfb_mode = (NUBUS_MACFB(dev)->macfb).mode;
> +
>       cs = CPU(cpu);
>       if (linux_boot) {
>           uint64_t high;
> @@ -450,12 +453,12 @@ static void q800_init(MachineState *machine)
>           BOOTINFO1(cs->as, parameters_base,
>                     BI_MAC_MEMSIZE, ram_size >> 20); /* in MB */
>           BOOTINFO2(cs->as, parameters_base, BI_MEMCHUNK, 0, ram_size);
> -        BOOTINFO1(cs->as, parameters_base, BI_MAC_VADDR, VIDEO_BASE);
> +        BOOTINFO1(cs->as, parameters_base, BI_MAC_VADDR,
> +                  VIDEO_BASE + macfb_mode->offset);
>           BOOTINFO1(cs->as, parameters_base, BI_MAC_VDEPTH, graphic_depth);
>           BOOTINFO1(cs->as, parameters_base, BI_MAC_VDIM,
>                     (graphic_height << 16) | graphic_width);
> -        BOOTINFO1(cs->as, parameters_base, BI_MAC_VROW,
> -                  (graphic_width * graphic_depth + 7) / 8);
> +        BOOTINFO1(cs->as, parameters_base, BI_MAC_VROW, macfb_mode->stride);
>           BOOTINFO1(cs->as, parameters_base, BI_MAC_SCCBASE, SCC_BASE);
>   
>           rom = g_malloc(sizeof(*rom));
> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> index e95a97ebdc..0aff0d84d2 100644
> --- a/include/hw/display/macfb.h
> +++ b/include/hw/display/macfb.h
> @@ -35,6 +35,19 @@ typedef enum  {
>       MACFB_DISPLAY_SVGA = 14,
>   } MacfbDisplayType;
>   
> +typedef struct MacFbMode {
> +    uint8_t type;
> +    uint8_t depth;
> +    uint32_t mode_ctrl1;
> +    uint32_t mode_ctrl2;
> +    uint32_t width;
> +    uint32_t height;
> +    uint32_t stride;
> +    uint32_t offset;
> +} MacFbMode;
> +
> +#define MACFB_NUM_REGS      8
> +
>   typedef struct MacfbState {
>       MemoryRegion mem_vram;
>       MemoryRegion mem_ctrl;
> @@ -48,7 +61,8 @@ typedef struct MacfbState {
>       uint8_t depth;
>       uint8_t type;
>   
> -    uint32_t sense;
> +    uint32_t regs[MACFB_NUM_REGS];
> +    MacFbMode *mode;
>   } MacfbState;
>   
>   #define TYPE_MACFB "sysbus-macfb"
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 09/12] macfb: fix up 1-bit pixel encoding

[Laurent Vivier]

On 02/10/2021 13:00, Mark Cave-Ayland wrote:
> The MacOS driver expects the RGB values for the pixel to be in entries 0 and 1
> of the colour palette.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 6a69334565..0c9e181b9b 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -128,7 +128,9 @@ static void macfb_draw_line1(MacfbState *s, uint8_t *d, uint32_t addr,
>       for (x = 0; x < width; x++) {
>           int bit = x & 7;
>           int idx = (macfb_read_byte(s, addr) >> (7 - bit)) & 1;
> -        r = g = b  = ((1 - idx) << 7);
> +        r = s->color_palette[idx * 3];
> +        g = s->color_palette[idx * 3 + 1];
> +        b = s->color_palette[idx * 3 + 2];
>           addr += (bit == 7);
>   
>           *(uint32_t *)d = rgb_to_pixel32(r, g, b);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 10/12] macfb: fix 24-bit RGB pixel encoding

[Laurent Vivier]

On 02/10/2021 13:00, Mark Cave-Ayland wrote:
> According to Apple Technical Note HW26: "Macintosh Quadra Built-In Video" the
> in-built framebuffer encodes each 24-bit pixel into 4 bytes. Adjust the 24-bit
> RGB pixel encoding accordingly which agrees with the encoding expected by MacOS
> when changing into 24-bit colour mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 0c9e181b9b..29f6ad8eba 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -224,10 +224,10 @@ static void macfb_draw_line24(MacfbState *s, uint8_t *d, uint32_t addr,
>       int x;
>   
>       for (x = 0; x < width; x++) {
> -        r = macfb_read_byte(s, addr);
> -        g = macfb_read_byte(s, addr + 1);
> -        b = macfb_read_byte(s, addr + 2);
> -        addr += 3;
> +        r = macfb_read_byte(s, addr + 1);
> +        g = macfb_read_byte(s, addr + 2);
> +        b = macfb_read_byte(s, addr + 3);
> +        addr += 4;
>   
>           *(uint32_t *)d = rgb_to_pixel32(r, g, b);
>           d += 4;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 11/12] macfb: add vertical blank interrupt

[Laurent Vivier]

Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> The MacOS driver expects a 60.15Hz vertical blank interrupt to be generated by
> the framebuffer which in turn schedules the mouse driver via the Vertical Retrace
> Manager.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c         | 81 ++++++++++++++++++++++++++++++++++++++
>  include/hw/display/macfb.h |  8 ++++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 29f6ad8eba..60a203e67b 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -33,9 +33,16 @@
>  #define DAFB_MODE_CTRL1     0x8
>  #define DAFB_MODE_CTRL2     0xc
>  #define DAFB_MODE_SENSE     0x1c
> +#define DAFB_INTR_MASK      0x104
> +#define DAFB_INTR_STAT      0x108
> +#define DAFB_INTR_CLEAR     0x10c
>  #define DAFB_RESET          0x200
>  #define DAFB_LUT            0x213
>  
> +#define DAFB_INTR_VBL   0x4
> +
> +/* Vertical Blank period (60.15Hz) */
> +#define DAFB_INTR_VBL_PERIOD_NS 16625800
>  
>  /*
>   * Quadra sense codes taken from Apple Technical Note HW26:
> @@ -449,6 +456,32 @@ static void macfb_update_display(void *opaque)
>      macfb_draw_graphic(s);
>  }
>  
> +static void macfb_update_irq(MacfbState *s)
> +{
> +    uint32_t irq_state = s->irq_state & s->irq_mask;
> +
> +    if (irq_state) {
> +        qemu_irq_raise(s->irq);
> +    } else {
> +        qemu_irq_lower(s->irq);
> +    }
> +}
> +
> +static void macfb_vbl_timer(void *opaque)
> +{
> +    MacfbState *s = opaque;
> +    int64_t next_vbl;
> +
> +    s->irq_state |= DAFB_INTR_VBL;
> +    macfb_update_irq(s);
> +
> +    /* 60 Hz irq */
> +    next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                DAFB_INTR_VBL_PERIOD_NS) /
> +                DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
> +    timer_mod(s->vbl_timer, next_vbl);

perhaps you can move this to a function and call it here and below?

> +}
> +
>  static void macfb_reset(MacfbState *s)
>  {
>      int i;
> @@ -477,6 +510,9 @@ static uint64_t macfb_ctrl_read(void *opaque,
>      case DAFB_MODE_CTRL2:
>          val = s->regs[addr >> 2];
>          break;
> +    case DAFB_INTR_STAT:
> +        val = s->irq_state;
> +        break;
>      case DAFB_MODE_SENSE:
>          val = macfb_sense_read(s);
>          break;
> @@ -492,6 +528,8 @@ static void macfb_ctrl_write(void *opaque,
>                               unsigned int size)
>  {
>      MacfbState *s = opaque;
> +    int64_t next_vbl;
> +
>      switch (addr) {
>      case DAFB_MODE_VADDR1:
>      case DAFB_MODE_VADDR2:
> @@ -507,8 +545,25 @@ static void macfb_ctrl_write(void *opaque,
>      case DAFB_MODE_SENSE:
>          macfb_sense_write(s, val);
>          break;
> +    case DAFB_INTR_MASK:
> +        s->irq_mask = val;
> +        if (val & DAFB_INTR_VBL) {
> +            next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                        DAFB_INTR_VBL_PERIOD_NS) /
> +                        DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
> +            timer_mod(s->vbl_timer, next_vbl);
> +        } else {
> +            timer_del(s->vbl_timer);
> +        }
> +        break;
> +    case DAFB_INTR_CLEAR:
> +        s->irq_state &= ~DAFB_INTR_VBL;
> +        macfb_update_irq(s);
> +        break;
>      case DAFB_RESET:
>          s->palette_current = 0;
> +        s->irq_state &= ~DAFB_INTR_VBL;
> +        macfb_update_irq(s);
>          break;
>      case DAFB_LUT:
>          s->color_palette[s->palette_current++] = val;
> @@ -586,6 +641,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>      s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>      memory_region_set_coalescing(&s->mem_vram);
>  
> +    s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
>      macfb_update_mode(s);
>  }
>  
> @@ -601,6 +657,16 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>  
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
> +
> +    qdev_init_gpio_out(dev, &ms->irq, 1);
> +}
> +
> +static void macfb_nubus_set_irq(void *opaque, int n, int level)
> +{
> +    MacfbNubusState *s = NUBUS_MACFB(opaque);
> +    NubusDevice *nd = NUBUS_DEVICE(s);
> +
> +    nubus_set_irq(nd, level);
>  }
>  
>  static void macfb_nubus_realize(DeviceState *dev, Error **errp)
> @@ -622,6 +688,19 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>  
>      memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>      memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
> +
> +    ms->irq = qemu_allocate_irq(macfb_nubus_set_irq, s, 0);
> +}
> +
> +static void macfb_nubus_unrealize(DeviceState *dev)
> +{
> +    MacfbNubusState *s = NUBUS_MACFB(dev);
> +    MacfbNubusDeviceClass *ndc = NUBUS_MACFB_GET_CLASS(dev);
> +    MacfbState *ms = &s->macfb;
> +
> +    ndc->parent_unrealize(dev);
> +
> +    qemu_free_irq(ms->irq);
>  }
>  
>  static void macfb_sysbus_reset(DeviceState *d)
> @@ -672,6 +751,8 @@ static void macfb_nubus_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, macfb_nubus_realize,
>                                      &ndc->parent_realize);
> +    device_class_set_parent_unrealize(dc, macfb_nubus_unrealize,
> +                                      &ndc->parent_unrealize);
>      dc->desc = "Nubus Macintosh framebuffer";
>      dc->reset = macfb_nubus_reset;
>      dc->vmsd = &vmstate_macfb;
> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> index 0aff0d84d2..e52775aa21 100644
> --- a/include/hw/display/macfb.h
> +++ b/include/hw/display/macfb.h
> @@ -14,7 +14,9 @@
>  #define MACFB_H
>  
>  #include "exec/memory.h"
> +#include "hw/irq.h"
>  #include "ui/console.h"
> +#include "qemu/timer.h"
>  #include "qom/object.h"
>  
>  typedef enum  {
> @@ -63,6 +65,11 @@ typedef struct MacfbState {
>  
>      uint32_t regs[MACFB_NUM_REGS];
>      MacFbMode *mode;
> +
> +    uint32_t irq_state;
> +    uint32_t irq_mask;
> +    QEMUTimer *vbl_timer;
> +    qemu_irq irq;
>  } MacfbState;
>  
>  #define TYPE_MACFB "sysbus-macfb"
> @@ -81,6 +88,7 @@ struct MacfbNubusDeviceClass {
>      DeviceClass parent_class;
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>  };
>  
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[Mark Cave-Ayland]

On 02/10/2021 12:36, BALATON Zoltan wrote:

> On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
>> Make sure any errors that occur within the macfb realize chain are detected
>> and handled correctly to prevent crashes and to ensure that error messages are
>> reported back to the user.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/display/macfb.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 76808b69cc..2b747a8de8 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
> 
> There's one more in macfb_common_realize() after:
> 
> memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram", 
> MACFB_VRAM_SIZE, errp);
> 
> otherwise
> 
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Ah yes, IIRC from the last patchset the outcome of the discussion with Markus was 
that these functions should use &error_abort. I'll make the same change here for v2.


ATB,

Mark.

Re: [PATCH 01/12] macfb: handle errors that occur during realize

[Mark Cave-Ayland]

On 02/10/2021 14:47, Philippe Mathieu-Daudé wrote:

> On 10/2/21 12:59, Mark Cave-Ayland wrote:
>> Make sure any errors that occur within the macfb realize chain are detected
>> and handled correctly to prevent crashes and to ensure that error messages are
>> reported back to the user.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 76808b69cc..2b747a8de8 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>>       MacfbState *ms = &s->macfb;
>>   
>>       macfb_common_realize(dev, ms, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
> 
> See a related discussion:
> https://lore.kernel.org/qemu-devel/87bl47ll9l.fsf@dusky.pond.sub.org/

Interesting, thanks for the link. I will update macfb_common_realize() to return a 
boolean for v2.


ATB,

Mark.

Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Mark Cave-Ayland]

On 02/10/2021 15:04, Philippe Mathieu-Daudé wrote:

> On 10/2/21 13:00, Mark Cave-Ayland wrote:
>> Since the available resolutions and colour depths are determined by the attached
>> display type, add a qdev property to allow the display type to be specified.
>>
>> The main resolutions of interest are high resolution 1152x870 with 8-bit colour
>> and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
>> to allow high resolution mode if specified and otherwise fall back to SVGA.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c         | 6 +++++-
>>   hw/m68k/q800.c             | 5 +++++
>>   include/hw/display/macfb.h | 1 +
>>   3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 5c95aa4a11..023d1f0cd1 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
>>       MacFbSense *macfb_sense;
>>       uint8_t sense;
>>   
> 
> What about:
> 
>         assert(s->type < ARRAY_SIZE(macfb_sense_table));
> 
>> -    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
>> +    macfb_sense = &macfb_sense_table[s->type];
> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Agreed, I will add this in for v2.


ATB,

Mark.

Re: [PATCH 03/12] macfb: fix overflow of color_palette array

[Mark Cave-Ayland]

On 04/10/2021 09:53, Laurent Vivier wrote:

> Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
>> The palette_current index counter has a maximum size of 256 * 3 to cover a full
>> color palette of 256 RGB entries. Linux assumes that the palette_current index
>> wraps back around to zero after writing 256 RGB entries so ensure that
>> palette_current is reset at this point to prevent data corruption within
>> MacfbState.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 815870f2fe..f4e789d0d7 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -307,6 +307,9 @@ static void macfb_ctrl_write(void *opaque,
>>           if (s->palette_current % 3) {
>>               macfb_invalidate_display(s);
>>           }
>> +        if (s->palette_current >= sizeof(s->color_palette)) {
>> +            s->palette_current = 0;
>> +        }
>>           break;
>>       }
>>   }
>>
> 
> What about "s->palette_current = (s->palette_current + 1) % sizeof(s->color_palette)"?
> 
> Thanks,
> Laurent

Sure, I can update that for v2.


ATB,

Mark.

Re: [PATCH 11/12] macfb: add vertical blank interrupt

[Mark Cave-Ayland]

On 04/10/2021 17:32, Laurent Vivier wrote:

> Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
>> The MacOS driver expects a 60.15Hz vertical blank interrupt to be generated by
>> the framebuffer which in turn schedules the mouse driver via the Vertical Retrace
>> Manager.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c         | 81 ++++++++++++++++++++++++++++++++++++++
>>   include/hw/display/macfb.h |  8 ++++
>>   2 files changed, 89 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 29f6ad8eba..60a203e67b 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -33,9 +33,16 @@
>>   #define DAFB_MODE_CTRL1     0x8
>>   #define DAFB_MODE_CTRL2     0xc
>>   #define DAFB_MODE_SENSE     0x1c
>> +#define DAFB_INTR_MASK      0x104
>> +#define DAFB_INTR_STAT      0x108
>> +#define DAFB_INTR_CLEAR     0x10c
>>   #define DAFB_RESET          0x200
>>   #define DAFB_LUT            0x213
>>   
>> +#define DAFB_INTR_VBL   0x4
>> +
>> +/* Vertical Blank period (60.15Hz) */
>> +#define DAFB_INTR_VBL_PERIOD_NS 16625800
>>   
>>   /*
>>    * Quadra sense codes taken from Apple Technical Note HW26:
>> @@ -449,6 +456,32 @@ static void macfb_update_display(void *opaque)
>>       macfb_draw_graphic(s);
>>   }
>>   
>> +static void macfb_update_irq(MacfbState *s)
>> +{
>> +    uint32_t irq_state = s->irq_state & s->irq_mask;
>> +
>> +    if (irq_state) {
>> +        qemu_irq_raise(s->irq);
>> +    } else {
>> +        qemu_irq_lower(s->irq);
>> +    }
>> +}
>> +
>> +static void macfb_vbl_timer(void *opaque)
>> +{
>> +    MacfbState *s = opaque;
>> +    int64_t next_vbl;
>> +
>> +    s->irq_state |= DAFB_INTR_VBL;
>> +    macfb_update_irq(s);
>> +
>> +    /* 60 Hz irq */
>> +    next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> +                DAFB_INTR_VBL_PERIOD_NS) /
>> +                DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
>> +    timer_mod(s->vbl_timer, next_vbl);
> 
> perhaps you can move this to a function and call it here and below?

I'd like to keep the timer_mod() outside of the function but I agree it is nicer to 
keep the next_vbl logci in a single place.

>> +}
>> +
>>   static void macfb_reset(MacfbState *s)
>>   {
>>       int i;
>> @@ -477,6 +510,9 @@ static uint64_t macfb_ctrl_read(void *opaque,
>>       case DAFB_MODE_CTRL2:
>>           val = s->regs[addr >> 2];
>>           break;
>> +    case DAFB_INTR_STAT:
>> +        val = s->irq_state;
>> +        break;
>>       case DAFB_MODE_SENSE:
>>           val = macfb_sense_read(s);
>>           break;
>> @@ -492,6 +528,8 @@ static void macfb_ctrl_write(void *opaque,
>>                                unsigned int size)
>>   {
>>       MacfbState *s = opaque;
>> +    int64_t next_vbl;
>> +
>>       switch (addr) {
>>       case DAFB_MODE_VADDR1:
>>       case DAFB_MODE_VADDR2:
>> @@ -507,8 +545,25 @@ static void macfb_ctrl_write(void *opaque,
>>       case DAFB_MODE_SENSE:
>>           macfb_sense_write(s, val);
>>           break;
>> +    case DAFB_INTR_MASK:
>> +        s->irq_mask = val;
>> +        if (val & DAFB_INTR_VBL) {
>> +            next_vbl = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> +                        DAFB_INTR_VBL_PERIOD_NS) /
>> +                        DAFB_INTR_VBL_PERIOD_NS * DAFB_INTR_VBL_PERIOD_NS;
>> +            timer_mod(s->vbl_timer, next_vbl);
>> +        } else {
>> +            timer_del(s->vbl_timer);
>> +        }
>> +        break;
>> +    case DAFB_INTR_CLEAR:
>> +        s->irq_state &= ~DAFB_INTR_VBL;
>> +        macfb_update_irq(s);
>> +        break;
>>       case DAFB_RESET:
>>           s->palette_current = 0;
>> +        s->irq_state &= ~DAFB_INTR_VBL;
>> +        macfb_update_irq(s);
>>           break;
>>       case DAFB_LUT:
>>           s->color_palette[s->palette_current++] = val;
>> @@ -586,6 +641,7 @@ static void macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>>       s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>>       memory_region_set_coalescing(&s->mem_vram);
>>   
>> +    s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
>>       macfb_update_mode(s);
>>   }
>>   
>> @@ -601,6 +657,16 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>>   
>>       sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>>       sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
>> +
>> +    qdev_init_gpio_out(dev, &ms->irq, 1);
>> +}
>> +
>> +static void macfb_nubus_set_irq(void *opaque, int n, int level)
>> +{
>> +    MacfbNubusState *s = NUBUS_MACFB(opaque);
>> +    NubusDevice *nd = NUBUS_DEVICE(s);
>> +
>> +    nubus_set_irq(nd, level);
>>   }
>>   
>>   static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>> @@ -622,6 +688,19 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>>   
>>       memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>>       memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
>> +
>> +    ms->irq = qemu_allocate_irq(macfb_nubus_set_irq, s, 0);
>> +}
>> +
>> +static void macfb_nubus_unrealize(DeviceState *dev)
>> +{
>> +    MacfbNubusState *s = NUBUS_MACFB(dev);
>> +    MacfbNubusDeviceClass *ndc = NUBUS_MACFB_GET_CLASS(dev);
>> +    MacfbState *ms = &s->macfb;
>> +
>> +    ndc->parent_unrealize(dev);
>> +
>> +    qemu_free_irq(ms->irq);
>>   }
>>   
>>   static void macfb_sysbus_reset(DeviceState *d)
>> @@ -672,6 +751,8 @@ static void macfb_nubus_class_init(ObjectClass *klass, void *data)
>>   
>>       device_class_set_parent_realize(dc, macfb_nubus_realize,
>>                                       &ndc->parent_realize);
>> +    device_class_set_parent_unrealize(dc, macfb_nubus_unrealize,
>> +                                      &ndc->parent_unrealize);
>>       dc->desc = "Nubus Macintosh framebuffer";
>>       dc->reset = macfb_nubus_reset;
>>       dc->vmsd = &vmstate_macfb;
>> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
>> index 0aff0d84d2..e52775aa21 100644
>> --- a/include/hw/display/macfb.h
>> +++ b/include/hw/display/macfb.h
>> @@ -14,7 +14,9 @@
>>   #define MACFB_H
>>   
>>   #include "exec/memory.h"
>> +#include "hw/irq.h"
>>   #include "ui/console.h"
>> +#include "qemu/timer.h"
>>   #include "qom/object.h"
>>   
>>   typedef enum  {
>> @@ -63,6 +65,11 @@ typedef struct MacfbState {
>>   
>>       uint32_t regs[MACFB_NUM_REGS];
>>       MacFbMode *mode;
>> +
>> +    uint32_t irq_state;
>> +    uint32_t irq_mask;
>> +    QEMUTimer *vbl_timer;
>> +    qemu_irq irq;
>>   } MacfbState;
>>   
>>   #define TYPE_MACFB "sysbus-macfb"
>> @@ -81,6 +88,7 @@ struct MacfbNubusDeviceClass {
>>       DeviceClass parent_class;
>>   
>>       DeviceRealize parent_realize;
>> +    DeviceUnrealize parent_unrealize;
>>   };
>>   
>>   
>>
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

ATB,

Mark.