If a dimension is not set, have the machine init code set
the default value by calling the ppc_graphic_dimensions()
helper, common to all PowerPC machines. Declare local
variables to avoid using the global ones.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ppc/ppc.h    |  2 ++
 hw/ppc/mac_newworld.c   | 10 ++++++----
 hw/ppc/mac_oldworld.c   | 10 ++++++----
 hw/ppc/ppc.c            |  8 ++++++++
 hw/ppc/prep.c           |  4 ++++
 hw/ppc/spapr.c          |  4 ++++
 system/globals-target.c |  6 ------
 7 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index cb51d704c6d..14cc09ab22b 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -102,6 +102,8 @@ enum {
     ARCH_MAC99_U3,
 };
 
+void ppc_graphic_dimensions(int *width, int *height, int *depth);
+
 #define FW_CFG_PPC_WIDTH        (FW_CFG_ARCH_LOCAL + 0x00)
 #define FW_CFG_PPC_HEIGHT       (FW_CFG_ARCH_LOCAL + 0x01)
 #define FW_CFG_PPC_DEPTH        (FW_CFG_ARCH_LOCAL + 0x02)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 7275563a155..daf0029c01a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -156,6 +156,7 @@ static void ppc_core99_init(MachineState *machine)
     DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
     hwaddr nvram_addr = 0xFFF04000;
     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
+    int graphic_width, graphic_height, graphic_depth;
 
     /* init CPUs */
     for (i = 0; i < machine->smp.cpus; i++) {
@@ -432,10 +433,6 @@ static void ppc_core99_init(MachineState *machine)
 
     pci_vga_init(pci_bus);
 
-    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
-        graphic_depth = 15;
-    }
-
     pci_init_nic_devices(pci_bus, mc->default_nic);
 
     /* The NewWorld NVRAM is not located in the MacIO device */
@@ -480,6 +477,11 @@ static void ppc_core99_init(MachineState *machine)
     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
 
+    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
+    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
+        graphic_depth = 15;
+    }
+
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index e679d338985..ea1f778877c 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -108,6 +108,7 @@ static void ppc_heathrow_init(MachineState *machine)
     DriveInfo *dinfo, *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     void *fw_cfg;
     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
+    int graphic_width, graphic_height, graphic_depth;
 
     /* init CPUs */
     for (i = 0; i < machine->smp.cpus; i++) {
@@ -288,10 +289,6 @@ static void ppc_heathrow_init(MachineState *machine)
         pci_create_simple(pci_bus, -1, "pci-ohci");
     }
 
-    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
-        graphic_depth = 15;
-    }
-
     /* No PCI init: the BIOS will do it */
 
     dev = qdev_new(TYPE_FW_CFG_MEM);
@@ -321,6 +318,11 @@ static void ppc_heathrow_init(MachineState *machine)
     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
 
+    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
+    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
+        graphic_depth = 15;
+    }
+
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index a512d4fa647..d7b4466d701 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -29,6 +29,7 @@
 #include "qemu/timer.h"
 #include "exec/cpu-interrupt.h"
 #include "system/cpus.h"
+#include "system/system.h"
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/error-report.h"
@@ -1557,3 +1558,10 @@ void ppc_irq_reset(PowerPCCPU *cpu)
         kvmppc_set_interrupt(cpu, PPC_INTERRUPT_EXT, 0);
     }
 }
+
+void ppc_graphic_dimensions(int *width, int *height, int *depth)
+{
+    *width = graphic_width ?: 800;
+    *height = graphic_height ?: 600;
+    *depth = graphic_depth ?: 32;
+}
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index c4efd1d3908..7077b047b25 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -43,6 +43,7 @@
 #include "exec/target_page.h"
 #include "system/kvm.h"
 #include "system/reset.h"
+#include "system/system.h"
 #include "trace.h"
 #include "elf.h"
 #include "qemu/units.h"
@@ -250,6 +251,7 @@ static void ibm_40p_init(MachineState *machine)
     uint32_t kernel_base = 0, initrd_base = 0;
     long kernel_size = 0, initrd_size = 0;
     char boot_device;
+    int graphic_width, graphic_height, graphic_depth;
 
     if (kvm_enabled()) {
         error_report("machine %s does not support the KVM accelerator",
@@ -412,6 +414,8 @@ static void ibm_40p_init(MachineState *machine)
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_PREP);
 
+    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
+
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 274f38785f2..c50c7da34d5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1143,6 +1143,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
 {
     MachineState *machine = MACHINE(spapr);
     int chosen;
+    int graphic_width, graphic_height, graphic_depth;
 
     _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
 
@@ -1177,6 +1178,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
         if (machine->boot_config.has_menu && machine->boot_config.menu) {
             _FDT((fdt_setprop_cell(fdt, chosen, "qemu,boot-menu", true)));
         }
+
+        ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
+
         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-width", graphic_width));
         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphic_height));
         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic_depth));
diff --git a/system/globals-target.c b/system/globals-target.c
index 17a27a06218..ffa6c308b59 100644
--- a/system/globals-target.c
+++ b/system/globals-target.c
@@ -9,12 +9,6 @@
 #include "qemu/osdep.h"
 #include "system/system.h"
 
-#if defined(TARGET_SPARC) || defined(TARGET_M68K)
 int graphic_width;
 int graphic_height;
 int graphic_depth;
-#else
-int graphic_width = 800;
-int graphic_height = 600;
-int graphic_depth = 32;
-#endif
-- 
2.52.0

On 14/02/2026 02:12, Philippe Mathieu-Daudé wrote:

> If a dimension is not set, have the machine init code set
> the default value by calling the ppc_graphic_dimensions()
> helper, common to all PowerPC machines. Declare local
> variables to avoid using the global ones.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/ppc/ppc.h    |  2 ++
>   hw/ppc/mac_newworld.c   | 10 ++++++----
>   hw/ppc/mac_oldworld.c   | 10 ++++++----
>   hw/ppc/ppc.c            |  8 ++++++++
>   hw/ppc/prep.c           |  4 ++++
>   hw/ppc/spapr.c          |  4 ++++
>   system/globals-target.c |  6 ------
>   7 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index cb51d704c6d..14cc09ab22b 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -102,6 +102,8 @@ enum {
>       ARCH_MAC99_U3,
>   };
>   
> +void ppc_graphic_dimensions(int *width, int *height, int *depth);
> +
>   #define FW_CFG_PPC_WIDTH        (FW_CFG_ARCH_LOCAL + 0x00)
>   #define FW_CFG_PPC_HEIGHT       (FW_CFG_ARCH_LOCAL + 0x01)
>   #define FW_CFG_PPC_DEPTH        (FW_CFG_ARCH_LOCAL + 0x02)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 7275563a155..daf0029c01a 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -156,6 +156,7 @@ static void ppc_core99_init(MachineState *machine)
>       DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
>       hwaddr nvram_addr = 0xFFF04000;
>       uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
> +    int graphic_width, graphic_height, graphic_depth;
>   
>       /* init CPUs */
>       for (i = 0; i < machine->smp.cpus; i++) {
> @@ -432,10 +433,6 @@ static void ppc_core99_init(MachineState *machine)
>   
>       pci_vga_init(pci_bus);
>   
> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> -        graphic_depth = 15;
> -    }
> -
>       pci_init_nic_devices(pci_bus, mc->default_nic);
>   
>       /* The NewWorld NVRAM is not located in the MacIO device */
> @@ -480,6 +477,11 @@ static void ppc_core99_init(MachineState *machine)
>       fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>   
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> +        graphic_depth = 15;
> +    }
> +
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index e679d338985..ea1f778877c 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -108,6 +108,7 @@ static void ppc_heathrow_init(MachineState *machine)
>       DriveInfo *dinfo, *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>       void *fw_cfg;
>       uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
> +    int graphic_width, graphic_height, graphic_depth;
>   
>       /* init CPUs */
>       for (i = 0; i < machine->smp.cpus; i++) {
> @@ -288,10 +289,6 @@ static void ppc_heathrow_init(MachineState *machine)
>           pci_create_simple(pci_bus, -1, "pci-ohci");
>       }
>   
> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> -        graphic_depth = 15;
> -    }
> -
>       /* No PCI init: the BIOS will do it */
>   
>       dev = qdev_new(TYPE_FW_CFG_MEM);
> @@ -321,6 +318,11 @@ static void ppc_heathrow_init(MachineState *machine)
>       fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>   
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> +        graphic_depth = 15;
> +    }
> +
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index a512d4fa647..d7b4466d701 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -29,6 +29,7 @@
>   #include "qemu/timer.h"
>   #include "exec/cpu-interrupt.h"
>   #include "system/cpus.h"
> +#include "system/system.h"
>   #include "qemu/log.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/error-report.h"
> @@ -1557,3 +1558,10 @@ void ppc_irq_reset(PowerPCCPU *cpu)
>           kvmppc_set_interrupt(cpu, PPC_INTERRUPT_EXT, 0);
>       }
>   }
> +
> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
> +{
> +    *width = graphic_width ?: 800;
> +    *height = graphic_height ?: 600;
> +    *depth = graphic_depth ?: 32;
> +}
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index c4efd1d3908..7077b047b25 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -43,6 +43,7 @@
>   #include "exec/target_page.h"
>   #include "system/kvm.h"
>   #include "system/reset.h"
> +#include "system/system.h"
>   #include "trace.h"
>   #include "elf.h"
>   #include "qemu/units.h"
> @@ -250,6 +251,7 @@ static void ibm_40p_init(MachineState *machine)
>       uint32_t kernel_base = 0, initrd_base = 0;
>       long kernel_size = 0, initrd_size = 0;
>       char boot_device;
> +    int graphic_width, graphic_height, graphic_depth;
>   
>       if (kvm_enabled()) {
>           error_report("machine %s does not support the KVM accelerator",
> @@ -412,6 +414,8 @@ static void ibm_40p_init(MachineState *machine)
>       fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_PREP);
>   
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>       fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 274f38785f2..c50c7da34d5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1143,6 +1143,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>   {
>       MachineState *machine = MACHINE(spapr);
>       int chosen;
> +    int graphic_width, graphic_height, graphic_depth;
>   
>       _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>   
> @@ -1177,6 +1178,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>           if (machine->boot_config.has_menu && machine->boot_config.menu) {
>               _FDT((fdt_setprop_cell(fdt, chosen, "qemu,boot-menu", true)));
>           }
> +
> +        ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +
>           _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-width", graphic_width));
>           _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphic_height));
>           _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic_depth));
> diff --git a/system/globals-target.c b/system/globals-target.c
> index 17a27a06218..ffa6c308b59 100644
> --- a/system/globals-target.c
> +++ b/system/globals-target.c
> @@ -9,12 +9,6 @@
>   #include "qemu/osdep.h"
>   #include "system/system.h"
>   
> -#if defined(TARGET_SPARC) || defined(TARGET_M68K)
>   int graphic_width;
>   int graphic_height;
>   int graphic_depth;
> -#else
> -int graphic_width = 800;
> -int graphic_height = 600;
> -int graphic_depth = 32;
> -#endif

Given that these machines are so different from each other, I don't think this really 
warrants a separate ppc_graphic_dimensions() function. I think just inlining the 
defaults logic as you've done already for other displays will be fine.


ATB,

Mark.

On Sat, 14 Feb 2026, Philippe Mathieu-Daudé wrote:
> If a dimension is not set, have the machine init code set
> the default value by calling the ppc_graphic_dimensions()
> helper, common to all PowerPC machines. Declare local
> variables to avoid using the global ones.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/ppc/ppc.h    |  2 ++
> hw/ppc/mac_newworld.c   | 10 ++++++----
> hw/ppc/mac_oldworld.c   | 10 ++++++----
> hw/ppc/ppc.c            |  8 ++++++++
> hw/ppc/prep.c           |  4 ++++
> hw/ppc/spapr.c          |  4 ++++
> system/globals-target.c |  6 ------
> 7 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index cb51d704c6d..14cc09ab22b 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -102,6 +102,8 @@ enum {
>     ARCH_MAC99_U3,
> };
>
> +void ppc_graphic_dimensions(int *width, int *height, int *depth);
> +
> #define FW_CFG_PPC_WIDTH        (FW_CFG_ARCH_LOCAL + 0x00)
> #define FW_CFG_PPC_HEIGHT       (FW_CFG_ARCH_LOCAL + 0x01)
> #define FW_CFG_PPC_DEPTH        (FW_CFG_ARCH_LOCAL + 0x02)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 7275563a155..daf0029c01a 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -156,6 +156,7 @@ static void ppc_core99_init(MachineState *machine)
>     DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
>     hwaddr nvram_addr = 0xFFF04000;
>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
> +    int graphic_width, graphic_height, graphic_depth;
>
>     /* init CPUs */
>     for (i = 0; i < machine->smp.cpus; i++) {
> @@ -432,10 +433,6 @@ static void ppc_core99_init(MachineState *machine)
>
>     pci_vga_init(pci_bus);
>
> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> -        graphic_depth = 15;
> -    }
> -
>     pci_init_nic_devices(pci_bus, mc->default_nic);
>
>     /* The NewWorld NVRAM is not located in the MacIO device */
> @@ -480,6 +477,11 @@ static void ppc_core99_init(MachineState *machine)
>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> +        graphic_depth = 15;
> +    }
> +
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index e679d338985..ea1f778877c 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -108,6 +108,7 @@ static void ppc_heathrow_init(MachineState *machine)
>     DriveInfo *dinfo, *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>     void *fw_cfg;
>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
> +    int graphic_width, graphic_height, graphic_depth;
>
>     /* init CPUs */
>     for (i = 0; i < machine->smp.cpus; i++) {
> @@ -288,10 +289,6 @@ static void ppc_heathrow_init(MachineState *machine)
>         pci_create_simple(pci_bus, -1, "pci-ohci");
>     }
>
> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> -        graphic_depth = 15;
> -    }
> -
>     /* No PCI init: the BIOS will do it */
>
>     dev = qdev_new(TYPE_FW_CFG_MEM);
> @@ -321,6 +318,11 @@ static void ppc_heathrow_init(MachineState *machine)
>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8) {
> +        graphic_depth = 15;
> +    }
> +
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index a512d4fa647..d7b4466d701 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -29,6 +29,7 @@
> #include "qemu/timer.h"
> #include "exec/cpu-interrupt.h"
> #include "system/cpus.h"
> +#include "system/system.h"
> #include "qemu/log.h"
> #include "qemu/main-loop.h"
> #include "qemu/error-report.h"
> @@ -1557,3 +1558,10 @@ void ppc_irq_reset(PowerPCCPU *cpu)
>         kvmppc_set_interrupt(cpu, PPC_INTERRUPT_EXT, 0);
>     }
> }
> +
> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
> +{
> +    *width = graphic_width ?: 800;
> +    *height = graphic_height ?: 600;
> +    *depth = graphic_depth ?: 32;
> +}

I think there's no reason to have this common to all ppc machines. If we 
change this we could also make it per machine like for other targets. 
There's nothing common in these machines that dictate these defaults. It 
seems it used to be an arbitrary default in QEMU that some machines later 
deviated from. So might as well just keep it as global for now after you 
removed the SPARC and M68K special cases. If the ppc machines are the last 
users of these global defaults you can just move them to hw/ppc/ppc.{h,c} 
for now and let ppc people clean this up eventually.

Regards,
BALATON Zoltan

> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index c4efd1d3908..7077b047b25 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -43,6 +43,7 @@
> #include "exec/target_page.h"
> #include "system/kvm.h"
> #include "system/reset.h"
> +#include "system/system.h"
> #include "trace.h"
> #include "elf.h"
> #include "qemu/units.h"
> @@ -250,6 +251,7 @@ static void ibm_40p_init(MachineState *machine)
>     uint32_t kernel_base = 0, initrd_base = 0;
>     long kernel_size = 0, initrd_size = 0;
>     char boot_device;
> +    int graphic_width, graphic_height, graphic_depth;
>
>     if (kvm_enabled()) {
>         error_report("machine %s does not support the KVM accelerator",
> @@ -412,6 +414,8 @@ static void ibm_40p_init(MachineState *machine)
>     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_PREP);
>
> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 274f38785f2..c50c7da34d5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1143,6 +1143,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
> {
>     MachineState *machine = MACHINE(spapr);
>     int chosen;
> +    int graphic_width, graphic_height, graphic_depth;
>
>     _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>
> @@ -1177,6 +1178,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>         if (machine->boot_config.has_menu && machine->boot_config.menu) {
>             _FDT((fdt_setprop_cell(fdt, chosen, "qemu,boot-menu", true)));
>         }
> +
> +        ppc_graphic_dimensions(&graphic_width, &graphic_height, &graphic_depth);
> +
>         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-width", graphic_width));
>         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphic_height));
>         _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic_depth));
> diff --git a/system/globals-target.c b/system/globals-target.c
> index 17a27a06218..ffa6c308b59 100644
> --- a/system/globals-target.c
> +++ b/system/globals-target.c
> @@ -9,12 +9,6 @@
> #include "qemu/osdep.h"
> #include "system/system.h"
>
> -#if defined(TARGET_SPARC) || defined(TARGET_M68K)
> int graphic_width;
> int graphic_height;
> int graphic_depth;
> -#else
> -int graphic_width = 800;
> -int graphic_height = 600;
> -int graphic_depth = 32;
> -#endif
>

Hi Zoltan,

On 14/2/26 12:46, BALATON Zoltan wrote:
> On Sat, 14 Feb 2026, Philippe Mathieu-Daudé wrote:
>> If a dimension is not set, have the machine init code set
>> the default value by calling the ppc_graphic_dimensions()
>> helper, common to all PowerPC machines. Declare local
>> variables to avoid using the global ones.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/ppc/ppc.h    |  2 ++
>> hw/ppc/mac_newworld.c   | 10 ++++++----
>> hw/ppc/mac_oldworld.c   | 10 ++++++----
>> hw/ppc/ppc.c            |  8 ++++++++
>> hw/ppc/prep.c           |  4 ++++
>> hw/ppc/spapr.c          |  4 ++++
>> system/globals-target.c |  6 ------
>> 7 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
>> index cb51d704c6d..14cc09ab22b 100644
>> --- a/include/hw/ppc/ppc.h
>> +++ b/include/hw/ppc/ppc.h
>> @@ -102,6 +102,8 @@ enum {
>>     ARCH_MAC99_U3,
>> };
>>
>> +void ppc_graphic_dimensions(int *width, int *height, int *depth);
>> +
>> #define FW_CFG_PPC_WIDTH        (FW_CFG_ARCH_LOCAL + 0x00)
>> #define FW_CFG_PPC_HEIGHT       (FW_CFG_ARCH_LOCAL + 0x01)
>> #define FW_CFG_PPC_DEPTH        (FW_CFG_ARCH_LOCAL + 0x02)
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index 7275563a155..daf0029c01a 100644
>> --- a/hw/ppc/mac_newworld.c
>> +++ b/hw/ppc/mac_newworld.c
>> @@ -156,6 +156,7 @@ static void ppc_core99_init(MachineState *machine)
>>     DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
>>     hwaddr nvram_addr = 0xFFF04000;
>>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
>> +    int graphic_width, graphic_height, graphic_depth;
>>
>>     /* init CPUs */
>>     for (i = 0; i < machine->smp.cpus; i++) {
>> @@ -432,10 +433,6 @@ static void ppc_core99_init(MachineState *machine)
>>
>>     pci_vga_init(pci_bus);
>>
>> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! 
>> = 8) {
>> -        graphic_depth = 15;
>> -    }
>> -
>>     pci_init_nic_devices(pci_bus, mc->default_nic);
>>
>>     /* The NewWorld NVRAM is not located in the MacIO device */
>> @@ -480,6 +477,11 @@ static void ppc_core99_init(MachineState *machine)
>>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>>
>> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, 
>> &graphic_depth);
>> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! 
>> = 8) {
>> +        graphic_depth = 15;
>> +    }
>> +
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index e679d338985..ea1f778877c 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -108,6 +108,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>     DriveInfo *dinfo, *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>     void *fw_cfg;
>>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
>> +    int graphic_width, graphic_height, graphic_depth;
>>
>>     /* init CPUs */
>>     for (i = 0; i < machine->smp.cpus; i++) {
>> @@ -288,10 +289,6 @@ static void ppc_heathrow_init(MachineState *machine)
>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>     }
>>
>> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! 
>> = 8) {
>> -        graphic_depth = 15;
>> -    }
>> -
>>     /* No PCI init: the BIOS will do it */
>>
>>     dev = qdev_new(TYPE_FW_CFG_MEM);
>> @@ -321,6 +318,11 @@ static void ppc_heathrow_init(MachineState *machine)
>>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>>
>> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, 
>> &graphic_depth);
>> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! 
>> = 8) {
>> +        graphic_depth = 15;
>> +    }
>> +
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index a512d4fa647..d7b4466d701 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -29,6 +29,7 @@
>> #include "qemu/timer.h"
>> #include "exec/cpu-interrupt.h"
>> #include "system/cpus.h"
>> +#include "system/system.h"
>> #include "qemu/log.h"
>> #include "qemu/main-loop.h"
>> #include "qemu/error-report.h"
>> @@ -1557,3 +1558,10 @@ void ppc_irq_reset(PowerPCCPU *cpu)
>>         kvmppc_set_interrupt(cpu, PPC_INTERRUPT_EXT, 0);
>>     }
>> }
>> +
>> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
>> +{
>> +    *width = graphic_width ?: 800;
>> +    *height = graphic_height ?: 600;
>> +    *depth = graphic_depth ?: 32;
>> +}
> 
> I think there's no reason to have this common to all ppc machines. If we 
> change this we could also make it per machine like for other targets. 
> There's nothing common in these machines that dictate these defaults. It 
> seems it used to be an arbitrary default in QEMU that some machines 
> later deviated from. So might as well just keep it as global for now 
> after you removed the SPARC and M68K special cases. If the ppc machines 
> are the last users of these global defaults you can just move them to 
> hw/ppc/ppc.{h,c} for now and let ppc people clean this up eventually.

What you are asking is to use 800x600x32 as the default for all
non-SPARC / non-M68K targets, so PPC don't have to change? But then
SPARC and M68k won't be able to use the '-g' CLI (which Paolo asked
me not to remove, thus this series). What is worrying you exactly?
Can you provide a patch showing how you'd prefer it to be done?

Thanks,

Phil.

On Sun, 15 Feb 2026, Philippe Mathieu-Daudé wrote:
> On 14/2/26 12:46, BALATON Zoltan wrote:
>> On Sat, 14 Feb 2026, Philippe Mathieu-Daudé wrote:
>>> If a dimension is not set, have the machine init code set
>>> the default value by calling the ppc_graphic_dimensions()
>>> helper, common to all PowerPC machines. Declare local
>>> variables to avoid using the global ones.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/ppc/ppc.h    |  2 ++
>>> hw/ppc/mac_newworld.c   | 10 ++++++----
>>> hw/ppc/mac_oldworld.c   | 10 ++++++----
>>> hw/ppc/ppc.c            |  8 ++++++++
>>> hw/ppc/prep.c           |  4 ++++
>>> hw/ppc/spapr.c          |  4 ++++
>>> system/globals-target.c |  6 ------
>>> 7 files changed, 30 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
>>> index cb51d704c6d..14cc09ab22b 100644
>>> --- a/include/hw/ppc/ppc.h
>>> +++ b/include/hw/ppc/ppc.h
>>> @@ -102,6 +102,8 @@ enum {
>>>     ARCH_MAC99_U3,
>>> };
>>> 
>>> +void ppc_graphic_dimensions(int *width, int *height, int *depth);
>>> +
>>> #define FW_CFG_PPC_WIDTH        (FW_CFG_ARCH_LOCAL + 0x00)
>>> #define FW_CFG_PPC_HEIGHT       (FW_CFG_ARCH_LOCAL + 0x01)
>>> #define FW_CFG_PPC_DEPTH        (FW_CFG_ARCH_LOCAL + 0x02)
>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>> index 7275563a155..daf0029c01a 100644
>>> --- a/hw/ppc/mac_newworld.c
>>> +++ b/hw/ppc/mac_newworld.c
>>> @@ -156,6 +156,7 @@ static void ppc_core99_init(MachineState *machine)
>>>     DeviceState *uninorth_internal_dev = NULL, *uninorth_agp_dev = NULL;
>>>     hwaddr nvram_addr = 0xFFF04000;
>>>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
>>> +    int graphic_width, graphic_height, graphic_depth;
>>> 
>>>     /* init CPUs */
>>>     for (i = 0; i < machine->smp.cpus; i++) {
>>> @@ -432,10 +433,6 @@ static void ppc_core99_init(MachineState *machine)
>>> 
>>>     pci_vga_init(pci_bus);
>>> 
>>> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! = 
>>> 8) {
>>> -        graphic_depth = 15;
>>> -    }
>>> -
>>>     pci_init_nic_devices(pci_bus, mc->default_nic);
>>> 
>>>     /* The NewWorld NVRAM is not located in the MacIO device */
>>> @@ -480,6 +477,11 @@ static void ppc_core99_init(MachineState *machine)
>>>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>>> 
>>> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, 
>>> &graphic_depth);
>>> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! = 
>>> 8) {
>>> +        graphic_depth = 15;
>>> +    }
>>> +
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>>> index e679d338985..ea1f778877c 100644
>>> --- a/hw/ppc/mac_oldworld.c
>>> +++ b/hw/ppc/mac_oldworld.c
>>> @@ -108,6 +108,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>>     DriveInfo *dinfo, *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>>     void *fw_cfg;
>>>     uint64_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TBFREQ;
>>> +    int graphic_width, graphic_height, graphic_depth;
>>> 
>>>     /* init CPUs */
>>>     for (i = 0; i < machine->smp.cpus; i++) {
>>> @@ -288,10 +289,6 @@ static void ppc_heathrow_init(MachineState *machine)
>>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>>     }
>>> 
>>> -    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! = 
>>> 8) {
>>> -        graphic_depth = 15;
>>> -    }
>>> -
>>>     /* No PCI init: the BIOS will do it */
>>> 
>>>     dev = qdev_new(TYPE_FW_CFG_MEM);
>>> @@ -321,6 +318,11 @@ static void ppc_heathrow_init(MachineState *machine)
>>>     fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ppc_boot_device);
>>> 
>>> +    ppc_graphic_dimensions(&graphic_width, &graphic_height, 
>>> &graphic_depth);
>>> +    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth ! = 
>>> 8) {
>>> +        graphic_depth = 15;
>>> +    }
>>> +
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_WIDTH, graphic_width);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_HEIGHT, graphic_height);
>>>     fw_cfg_add_i16(fw_cfg, FW_CFG_PPC_DEPTH, graphic_depth);
>>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>>> index a512d4fa647..d7b4466d701 100644
>>> --- a/hw/ppc/ppc.c
>>> +++ b/hw/ppc/ppc.c
>>> @@ -29,6 +29,7 @@
>>> #include "qemu/timer.h"
>>> #include "exec/cpu-interrupt.h"
>>> #include "system/cpus.h"
>>> +#include "system/system.h"
>>> #include "qemu/log.h"
>>> #include "qemu/main-loop.h"
>>> #include "qemu/error-report.h"
>>> @@ -1557,3 +1558,10 @@ void ppc_irq_reset(PowerPCCPU *cpu)
>>>         kvmppc_set_interrupt(cpu, PPC_INTERRUPT_EXT, 0);
>>>     }
>>> }
>>> +
>>> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
>>> +{
>>> +    *width = graphic_width ?: 800;
>>> +    *height = graphic_height ?: 600;
>>> +    *depth = graphic_depth ?: 32;
>>> +}
>> 
>> I think there's no reason to have this common to all ppc machines. If we 
>> change this we could also make it per machine like for other targets. 
>> There's nothing common in these machines that dictate these defaults. It 
>> seems it used to be an arbitrary default in QEMU that some machines later 
>> deviated from. So might as well just keep it as global for now after you 
>> removed the SPARC and M68K special cases. If the ppc machines are the last 
>> users of these global defaults you can just move them to hw/ppc/ppc.{h,c} 
>> for now and let ppc people clean this up eventually.
>
> What you are asking is to use 800x600x32 as the default for all
> non-SPARC / non-M68K targets, so PPC don't have to change? But then

This is what the original code did. But now I get that you need a way to 
tell if these were set so changed the default to 0 so the sparc and m68k 
can set their own defaults if the user did not provide values.

> SPARC and M68k won't be able to use the '-g' CLI (which Paolo asked
> me not to remove, thus this series). What is worrying you exactly?

Too much churn and introducing interdependency between ppc machines that 
should be removed later. The ppc_graphic_dimensions does not make sense as 
these are different machines with potentially different default 
resolutions (and most of these should be able to use EDID that we since 
have in QEMU).

> Can you provide a patch showing how you'd prefer it to be done?

Maybe turning these into properties could work. I could look at adding 
such properties but don't know how to change the -g option. But 
considering these are all machines that Mark maintains maybe he should do 
something about it. The easiest were if he agreed to drop -g option but he 
never agrees removing anything so then he should provide patches to clean 
this up the way he prefers. I could try to make patches but I'm afraid 
those would become wasted effort so may I only do it when there's an 
approved way that will be accepted.

Regards,
BALATON Zoltan

Il dom 15 feb 2026, 10:51 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> >>> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
> >>> +{
> >>> +    *width = graphic_width ?: 800;
> >>> +    *height = graphic_height ?: 600;
> >>> +    *depth = graphic_depth ?: 32;
> >>> +}
> >>
> >> I think there's no reason to have this common to all ppc machines.


I agree with you here that it's better to just inline this in all the
boards that need it, because there's no reason to have 800x600x32 across
such disparate machines.

> Can you provide a patch showing how you'd prefer it to be done?
>
> Maybe turning these into properties could work. I could look at adding
> such properties but don't know how to change the -g option. But
> considering these are all machines that Mark maintains maybe he should do
> something about it


I am not sure what you'd be requesting of him?

The easiest were if he agreed to drop -g option but he
> never agrees removing anything


Not sure who's he? Is it me since I asked Philippe to keep -g, or Mark? And
if it's Mark, I am not sure what you're trying to obtain by involving (and
in a pretty rude way, too) someone who's not even part of the thread.

so then he should provide patches to clean
> this up the way he prefers. I could try to make patches but I'm afraid
> those would become wasted effort so may I only do it when there's an
> approved way that will be accepted.
>

The -g 800x600x32 option, if desired, can become simply a shortcut for -M
gfxwidth=800,gfxheight=600,gfxdepth=32 (or
gfx.width=800,gfx.height=600,gfx.depth=32), which is more similar to how
smp or boot work). This would actually be easier to implement with
Philippe's move of all graphics_* accesses to board code, so his patches
should go in either way. And then the globals can be removed altogether.

Paolo


> Regards,
> BALATON Zoltan

On Mon, 16 Feb 2026, Paolo Bonzini wrote:
> Il dom 15 feb 2026, 10:51 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>>>>> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
>>>>> +{
>>>>> +    *width = graphic_width ?: 800;
>>>>> +    *height = graphic_height ?: 600;
>>>>> +    *depth = graphic_depth ?: 32;
>>>>> +}
>>>>
>>>> I think there's no reason to have this common to all ppc machines.
>
>
> I agree with you here that it's better to just inline this in all the
> boards that need it, because there's no reason to have 800x600x32 across
> such disparate machines.
>
>> Can you provide a patch showing how you'd prefer it to be done?
>>
>> Maybe turning these into properties could work. I could look at adding
>> such properties but don't know how to change the -g option. But
>> considering these are all machines that Mark maintains maybe he should do
>> something about it
>
>
> I am not sure what you'd be requesting of him?
>
> The easiest were if he agreed to drop -g option but he
>> never agrees removing anything
>
>
> Not sure who's he? Is it me since I asked Philippe to keep -g, or Mark? And
> if it's Mark, I am not sure what you're trying to obtain by involving (and
> in a pretty rude way, too) someone who's not even part of the thread.

Mark is cc-ed on this thread and it seems this -g option is only used by 
machines he maintains. My previous experience with patches submitted to 
things he maintains is that this most of the time leads to long 
discussions on why what I propose is wrong and how else should it be done 
instead. (I'm not saying that it's because it's me who submits the patch, 
I've seen this happening to other contributors too but it looks like Mark 
has strong views on how things should be done and he's reluctant to accept 
anything else.) Because of that I try not to touch anything that Mark 
maintains unless he says beforehand what would be acceptable. I did not 
mean it to be rude or attack him but I also think this isn't the best way 
to work either. But given these cicrumstances I think Mark should state 
his peference about this as at the end he is the maintainer who will have 
to approve it.

> so then he should provide patches to clean
>> this up the way he prefers. I could try to make patches but I'm afraid
>> those would become wasted effort so may I only do it when there's an
>> approved way that will be accepted.
>
> The -g 800x600x32 option, if desired, can become simply a shortcut for -M
> gfxwidth=800,gfxheight=600,gfxdepth=32 (or
> gfx.width=800,gfx.height=600,gfx.depth=32), which is more similar to how
> smp or boot work). This would actually be easier to implement with
> Philippe's move of all graphics_* accesses to board code, so his patches
> should go in either way. And then the globals can be removed altogether.

A problem with global option may be that resolution is not really a 
machine property but that of the display and one machine may have more 
than one display. Currently those display devices that support EDID use 
xres,yres,xmin,ymin,xmax,ymax properties and some devices like VGA have 
DEFINE_EDID_PROPERTIES to forward these to the edid generation. For others 
it can be set less intuitively with -global. (This was asked by somebody 
for ati-vga recently so I plan to add forwarding properties for that.) 
Maybe we should move into that direction and make these properties of the 
display devices and call them xres,yres too for consistency. But there may 
be some complication here with fw_cfg and OpenBIOS too so in that case 
just keeping the globals and using machine properties may be simpler.

Regards,
BALATON Zoltan

On Mon, 16 Feb 2026, BALATON Zoltan wrote:
> On Mon, 16 Feb 2026, Paolo Bonzini wrote:
>> Il dom 15 feb 2026, 10:51 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>>>>>> +void ppc_graphic_dimensions(int *width, int *height, int *depth)
>>>>>> +{
>>>>>> +    *width = graphic_width ?: 800;
>>>>>> +    *height = graphic_height ?: 600;
>>>>>> +    *depth = graphic_depth ?: 32;
>>>>>> +}
>>>>> 
>>>>> I think there's no reason to have this common to all ppc machines.
>> 
>> 
>> I agree with you here that it's better to just inline this in all the
>> boards that need it, because there's no reason to have 800x600x32 across
>> such disparate machines.
>> 
>>> Can you provide a patch showing how you'd prefer it to be done?
>>> 
>>> Maybe turning these into properties could work. I could look at adding
>>> such properties but don't know how to change the -g option. But
>>> considering these are all machines that Mark maintains maybe he should do
>>> something about it
>> 
>> 
>> I am not sure what you'd be requesting of him?
>> 
>> The easiest were if he agreed to drop -g option but he
>>> never agrees removing anything
>> 
>> 
>> Not sure who's he? Is it me since I asked Philippe to keep -g, or Mark? And
>> if it's Mark, I am not sure what you're trying to obtain by involving (and
>> in a pretty rude way, too) someone who's not even part of the thread.
>
> Mark is cc-ed on this thread and it seems this -g option is only used by 
> machines he maintains. My previous experience with patches submitted to 
> things he maintains is that this most of the time leads to long discussions 
> on why what I propose is wrong and how else should it be done instead. (I'm 
> not saying that it's because it's me who submits the patch, I've seen this 
> happening to other contributors too but it looks like Mark has strong views 
> on how things should be done and he's reluctant to accept anything else.) 
> Because of that I try not to touch anything that Mark maintains unless he 
> says beforehand what would be acceptable. I did not mean it to be rude or 
> attack him but I also think this isn't the best way to work either. But given 
> these cicrumstances I think Mark should state his peference about this as at 
> the end he is the maintainer who will have to approve it.
>
>> so then he should provide patches to clean
>>> this up the way he prefers. I could try to make patches but I'm afraid
>>> those would become wasted effort so may I only do it when there's an
>>> approved way that will be accepted.
>> 
>> The -g 800x600x32 option, if desired, can become simply a shortcut for -M
>> gfxwidth=800,gfxheight=600,gfxdepth=32 (or
>> gfx.width=800,gfx.height=600,gfx.depth=32), which is more similar to how
>> smp or boot work). This would actually be easier to implement with
>> Philippe's move of all graphics_* accesses to board code, so his patches
>> should go in either way. And then the globals can be removed altogether.
>
> A problem with global option may be that resolution is not really a machine 
> property but that of the display and one machine may have more than one 
> display. Currently those display devices that support EDID use 
> xres,yres,xmin,ymin,xmax,ymax properties and some devices like VGA have

Ther's no xmin,ymin but there's refresh_rate. However depth is missing.

> DEFINE_EDID_PROPERTIES to forward these to the edid generation. For others it 
> can be set less intuitively with -global. (This was asked by somebody for 
> ati-vga recently so I plan to add forwarding properties for that.) Maybe we 
> should move into that direction and make these properties of the display 
> devices and call them xres,yres too for consistency. But there may be some 
> complication here with fw_cfg and OpenBIOS too so in that case just keeping 
> the globals and using machine properties may be simpler.

Another problem with -g is that it only works with a few obscure machines 
and will do nothing for most of the other machines which is confusing. So 
in the longer term I think it should either be deprecated and these 
machines changed to use properties on display devices like the others or 
as you proposed last year it may need to become an option to set defaults 
for all displays which is simliar to audiodev that was turned into a 
machine property from a top level option a while ago.

Anyway these are just my comments to this topic that I wanted to share and 
then the involved parties will do whatever they want. I wanted to share 
this in the hope to add some points that I think may be relevant.

Regards,
BALATON Zoltan

On 15/02/2026 09:51, BALATON Zoltan wrote:

> Maybe turning these into properties could work. I could look at adding such 
> properties but don't know how to change the -g option. But considering these are all 
> machines that Mark maintains maybe he should do something about it. The easiest were 
> if he agreed to drop -g option but he never agrees removing anything so then he 
> should provide patches to clean this up the way he prefers. I could try to make 
> patches but I'm afraid those would become wasted effort so may I only do it when 
> there's an approved way that will be accepted.

I don't think these types of snide comment are appropriate.


ATB,

Mark.

On Mon, 16 Feb 2026, Mark Cave-Ayland wrote:
> On 15/02/2026 09:51, BALATON Zoltan wrote:
>
>> Maybe turning these into properties could work. I could look at adding such 
>> properties but don't know how to change the -g option. But considering 
>> these are all machines that Mark maintains maybe he should do something 
>> about it. The easiest were if he agreed to drop -g option but he never 
>> agrees removing anything so then he should provide patches to clean this up 
>> the way he prefers. I could try to make patches but I'm afraid those would 
>> become wasted effort so may I only do it when there's an approved way that 
>> will be accepted.
>
> I don't think these types of snide comment are appropriate.

Sorry, this is just my experience so far with patches that touch files you 
maintain. Last example is trying to change Sun machines to not use global 
vmstate that could have been done to clean this up and bring it in line 
with everyting else as these Sun machines are the last active user of this 
and have no migration guarantee but you insisted to keep it until the last 
possible minute. I did not mean to offend you, just wanted to say that 
since whatever I submit usually does not meet your preferred way it's best 
if I don't try to make patches that then would be discarded because you 
want something else and you should at lest tell in advance what would be 
your preference here.

Regards,
BALATON Zoltan

On 2/13/26 6:12 PM, Philippe Mathieu-Daudé wrote:
> If a dimension is not set, have the machine init code set
> the default value by calling the ppc_graphic_dimensions()
> helper, common to all PowerPC machines. Declare local
> variables to avoid using the global ones.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/ppc/ppc.h    |  2 ++
>   hw/ppc/mac_newworld.c   | 10 ++++++----
>   hw/ppc/mac_oldworld.c   | 10 ++++++----
>   hw/ppc/ppc.c            |  8 ++++++++
>   hw/ppc/prep.c           |  4 ++++
>   hw/ppc/spapr.c          |  4 ++++
>   system/globals-target.c |  6 ------
>   7 files changed, 30 insertions(+), 14 deletions(-)
> 

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>