diff mbox series

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

Message ID 20211002110007.30825-8-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series macfb: fixes for booting MacOS | expand

Commit Message

Mark Cave-Ayland Oct. 2, 2021, 11 a.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Oct. 2, 2021, 2:04 p.m. UTC | #1 
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>
Laurent Vivier Oct. 4, 2021, 9:24 a.m. UTC | #2 
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
Laurent Vivier Oct. 4, 2021, 2:38 p.m. UTC | #3 
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>
Mark Cave-Ayland Oct. 4, 2021, 6:46 p.m. UTC | #4 
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.
diff mbox series

Patch

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;