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-ayl...@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 <laur...@vivier.eu>