[PATCH v3 04/14] hw/display/apple-gfx: Adds configurable mode list

Phil Dennis-Jordan posted 14 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v3 04/14] hw/display/apple-gfx: Adds configurable mode list
Posted by Phil Dennis-Jordan 1 month, 3 weeks ago
This change adds a property 'display_modes' on the graphics device
which permits specifying a list of display modes. (screen resolution
and refresh rate)

PCI variant of apple-gfx only for the moment.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx-pci.m |  43 ++++++++++-
 hw/display/apple-gfx.h     |  17 ++++-
 hw/display/apple-gfx.m     | 151 ++++++++++++++++++++++++++++++++++---
 3 files changed, 198 insertions(+), 13 deletions(-)

diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index 9370258ee46..ea86a1f4a21 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -16,6 +16,7 @@
 #include "apple-gfx.h"
 #include "hw/pci/pci_device.h"
 #include "hw/pci/msi.h"
+#include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "trace.h"
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
@@ -101,6 +102,46 @@ static void apple_gfx_pci_reset(Object *obj, ResetType type)
     [s->common.pgdev reset];
 }
 
+static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+    apple_gfx_get_display_modes(mode_list, v, name, errp);
+}
+
+static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+    apple_gfx_set_display_modes(mode_list, v, name, errp);
+}
+
+const PropertyInfo apple_gfx_pci_prop_display_modes = {
+    .name  = "display_modes",
+    .description =
+        "Colon-separated list of display modes; "
+        "<width>x<height>@<refresh-rate>; the first mode is considered "
+        "'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",
+    .get   = apple_gfx_pci_get_display_modes,
+    .set   = apple_gfx_pci_set_display_modes,
+};
+
+#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
+    DEFINE_PROP(_name, _state, _field, apple_gfx_pci_prop_display_modes, \
+                AppleGFXDisplayModeList)
+
+static Property apple_gfx_pci_properties[] = {
+    DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
+                              common.display_modes),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -118,7 +159,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
     pci->class_id = PCI_CLASS_DISPLAY_OTHER;
     pci->realize = apple_gfx_pci_realize;
 
-    // TODO: Property for setting mode list
+    device_class_set_props(dc, apple_gfx_pci_properties);
 }
 
 static TypeInfo apple_gfx_pci_types[] = {
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 995ecf7f4a7..baad4a98652 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -5,14 +5,28 @@
 #define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
 
 #include "qemu/typedefs.h"
+#include "qemu/osdep.h"
 
 typedef struct AppleGFXState AppleGFXState;
 
+typedef struct AppleGFXDisplayMode {
+    uint16_t width_px;
+    uint16_t height_px;
+    uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
+typedef struct AppleGFXDisplayModeList {
+    GArray *modes;
+} AppleGFXDisplayModeList;
+
 void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name);
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp);
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp);
 
 #ifdef __OBJC__
 
-#include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
 #include <dispatch/dispatch.h>
@@ -38,6 +52,7 @@ struct AppleGFXState {
     bool new_frame;
     bool cursor_show;
     QEMUCursor *cursor;
+    AppleGFXDisplayModeList display_modes;
 
     dispatch_queue_t render_queue;
     /* The following fields should only be accessed from render_queue: */
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 6ef1048d93d..358192db6a0 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -16,6 +16,9 @@
 #include "trace.h"
 #include "qemu-main.h"
 #include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -23,9 +26,10 @@
 #include <mach/mach_vm.h>
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
 
-static const PGDisplayCoord_t apple_gfx_modes[] = {
-    { .x = 1440, .y = 1080 },
-    { .x = 1280, .y = 1024 },
+static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
+    { 1920, 1080, 60 },
+    { 1440, 1080, 60 },
+    { 1280, 1024, 60 },
 };
 
 typedef struct PGTask_s { // Name matches forward declaration in PG header
@@ -264,7 +268,6 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 static void create_fb(AppleGFXState *s)
 {
     s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
-    set_mode(s, 1440, 1080);
 
     s->cursor_show = true;
 }
@@ -466,20 +469,24 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
     return disp_desc;
 }
 
-static NSArray<PGDisplayMode*>* apple_gfx_prepare_display_mode_array(void)
+static NSArray<PGDisplayMode*>* apple_gfx_create_display_mode_array(
+    const AppleGFXDisplayMode display_modes[], int display_mode_count)
 {
-    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
+    PGDisplayMode **modes = alloca(sizeof(modes[0]) * display_mode_count);
     NSArray<PGDisplayMode*>* mode_array = nil;
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+    for (i = 0; i < display_mode_count; i++) {
+        const AppleGFXDisplayMode *mode = &display_modes[i];
+        PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px };
         modes[i] =
-            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
+            [[PGDisplayMode alloc] initWithSizeInPixels:mode_size
+                                        refreshRateInHz:mode->refresh_rate_hz];
     }
 
-    mode_array = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
+    mode_array = [NSArray arrayWithObjects:modes count:display_mode_count];
 
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+    for (i = 0; i < display_mode_count; i++) {
         [modes[i] release];
         modes[i] = nil;
     }
@@ -516,6 +523,8 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
 {
     PGDisplayDescriptor *disp_desc = nil;
+    const AppleGFXDisplayMode *display_modes = apple_gfx_default_modes;
+    int num_display_modes = ARRAY_SIZE(apple_gfx_default_modes);
 
     QTAILQ_INIT(&s->tasks);
     s->render_queue = dispatch_queue_create("apple-gfx.render",
@@ -533,7 +542,127 @@ void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
     s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc
                                               port:0 serialNum:1234];
     [disp_desc release];
-    s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
+
+    if (s->display_modes.modes != NULL && s->display_modes.modes->len > 0) {
+        display_modes =
+            &g_array_index(s->display_modes.modes, AppleGFXDisplayMode, 0);
+        num_display_modes = s->display_modes.modes->len;
+    }
+    s->pgdisp.modeList =
+        apple_gfx_create_display_mode_array(display_modes, num_display_modes);
 
     create_fb(s);
 }
+
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp)
+{
+    GArray *modes = mode_list->modes;
+    /* 3 uint16s (max 5 digits) and 3 separator characters per mode + nul. */
+    size_t buffer_size = (5 + 1) * 3 * modes->len + 1;
+
+    char *buffer = alloca(buffer_size);
+    char *pos = buffer;
+
+    unsigned used = 0;
+    buffer[0] = '\0';
+    for (guint i = 0; i < modes->len; ++i)
+    {
+        AppleGFXDisplayMode *mode =
+            &g_array_index(modes, AppleGFXDisplayMode, i);
+        int rc = snprintf(pos, buffer_size - used,
+                          "%s%"PRIu16"x%"PRIu16"@%"PRIu16,
+                          i > 0 ? ":" : "",
+                          mode->width_px, mode->height_px,
+                          mode->refresh_rate_hz);
+        used += rc;
+        pos += rc;
+        assert(used < buffer_size);
+    }
+
+    pos = buffer;
+    visit_type_str(v, name, &pos, errp);
+}
+
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *endptr;
+    char *str;
+    int ret;
+    unsigned int val;
+    uint32_t num_modes;
+    GArray *modes;
+    uint32_t mode_idx;
+
+    visit_type_str(v, name, &str, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    // Count colons to estimate modes. No leading/trailing colons so start at 1.
+    num_modes = 1;
+    for (size_t i = 0; str[i] != '\0'; ++i)
+    {
+        if (str[i] == ':') {
+            ++num_modes;
+        }
+    }
+
+    modes = g_array_sized_new(false, true, sizeof(AppleGFXDisplayMode), num_modes);
+
+    endptr = str;
+    for (mode_idx = 0; mode_idx < num_modes; ++mode_idx)
+    {
+        AppleGFXDisplayMode mode = {};
+        if (mode_idx > 0)
+        {
+            if (*endptr != ':') {
+                goto separator_error;
+            }
+            ++endptr;
+        }
+
+        ret = qemu_strtoui(endptr, &endptr, 10, &val);
+        if (ret || val > UINT16_MAX || val == 0) {
+            error_setg(errp, "width of '%s' must be a decimal integer number "
+                       "of pixels in the range 1..65535", name);
+            goto out;
+        }
+        mode.width_px = val;
+        if (*endptr != 'x') {
+            goto separator_error;
+        }
+
+        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
+        if (ret || val > UINT16_MAX || val == 0) {
+            error_setg(errp, "height of '%s' must be a decimal integer number "
+                       "of pixels in the range 1..65535", name);
+            goto out;
+        }
+        mode.height_px = val;
+        if (*endptr != '@') {
+            goto separator_error;
+        }
+
+        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
+        if (ret) {
+            error_setg(errp, "refresh rate of '%s'"
+                       " must be a non-negative decimal integer (Hertz)", name);
+        }
+        mode.refresh_rate_hz = val;
+        g_array_append_val(modes, mode);
+    }
+
+    mode_list->modes = modes;
+    goto out;
+
+separator_error:
+    error_setg(errp, "Each display mode takes the format "
+               "'<width>x<height>@<rate>', modes are separated by colons. (:)");
+out:
+    g_free(str);
+    return;
+}
-- 
2.39.3 (Apple Git-145)
Re: [PATCH v3 04/14] hw/display/apple-gfx: Adds configurable mode list
Posted by Akihiko Odaki 1 month, 2 weeks ago
On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> This change adds a property 'display_modes' on the graphics device
> which permits specifying a list of display modes. (screen resolution
> and refresh rate)
> 
> PCI variant of apple-gfx only for the moment.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/display/apple-gfx-pci.m |  43 ++++++++++-
>   hw/display/apple-gfx.h     |  17 ++++-
>   hw/display/apple-gfx.m     | 151 ++++++++++++++++++++++++++++++++++---
>   3 files changed, 198 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
> index 9370258ee46..ea86a1f4a21 100644
> --- a/hw/display/apple-gfx-pci.m
> +++ b/hw/display/apple-gfx-pci.m
> @@ -16,6 +16,7 @@
>   #include "apple-gfx.h"
>   #include "hw/pci/pci_device.h"
>   #include "hw/pci/msi.h"
> +#include "hw/qdev-properties.h"
>   #include "qapi/error.h"
>   #include "trace.h"
>   #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
> @@ -101,6 +102,46 @@ static void apple_gfx_pci_reset(Object *obj, ResetType type)
>       [s->common.pgdev reset];
>   }
>   
> +static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
> +                                            const char *name, void *opaque,
> +                                            Error **errp)
> +{
> +    Property *prop = opaque;
> +    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
> +
> +    apple_gfx_get_display_modes(mode_list, v, name, errp);
> +}
> +
> +static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
> +                                            const char *name, void *opaque,
> +                                            Error **errp)
> +{
> +    Property *prop = opaque;
> +    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
> +
> +    apple_gfx_set_display_modes(mode_list, v, name, errp);
> +}
> +
> +const PropertyInfo apple_gfx_pci_prop_display_modes = {
> +    .name  = "display_modes",
> +    .description =
> +        "Colon-separated list of display modes; "
> +        "<width>x<height>@<refresh-rate>; the first mode is considered "
> +        "'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",

Please use DEFINE_PROP_ARRAY() instead of inventing your own way to 
define an array.

> +    .get   = apple_gfx_pci_get_display_modes,
> +    .set   = apple_gfx_pci_set_display_modes,
> +};
> +
> +#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
> +    DEFINE_PROP(_name, _state, _field, apple_gfx_pci_prop_display_modes, \
> +                AppleGFXDisplayModeList)
> +
> +static Property apple_gfx_pci_properties[] = {
> +    DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
> +                              common.display_modes),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -118,7 +159,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
>       pci->class_id = PCI_CLASS_DISPLAY_OTHER;
>       pci->realize = apple_gfx_pci_realize;
>   
> -    // TODO: Property for setting mode list
> +    device_class_set_props(dc, apple_gfx_pci_properties);
>   }
>   
>   static TypeInfo apple_gfx_pci_types[] = {
> diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
> index 995ecf7f4a7..baad4a98652 100644
> --- a/hw/display/apple-gfx.h
> +++ b/hw/display/apple-gfx.h
> @@ -5,14 +5,28 @@
>   #define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
>   
>   #include "qemu/typedefs.h"
> +#include "qemu/osdep.h"
>   
>   typedef struct AppleGFXState AppleGFXState;
>   
> +typedef struct AppleGFXDisplayMode {
> +    uint16_t width_px;
> +    uint16_t height_px;
> +    uint16_t refresh_rate_hz;
> +} AppleGFXDisplayMode;
> +
> +typedef struct AppleGFXDisplayModeList {
> +    GArray *modes;
> +} AppleGFXDisplayModeList;
> +
>   void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name);
> +void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
> +                                 const char *name, Error **errp);
> +void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
> +                                 const char *name, Error **errp);
>   
>   #ifdef __OBJC__
>   
> -#include "qemu/osdep.h"
>   #include "exec/memory.h"
>   #include "ui/surface.h"
>   #include <dispatch/dispatch.h>
> @@ -38,6 +52,7 @@ struct AppleGFXState {
>       bool new_frame;
>       bool cursor_show;
>       QEMUCursor *cursor;
> +    AppleGFXDisplayModeList display_modes;
>   
>       dispatch_queue_t render_queue;
>       /* The following fields should only be accessed from render_queue: */
> diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
> index 6ef1048d93d..358192db6a0 100644
> --- a/hw/display/apple-gfx.m
> +++ b/hw/display/apple-gfx.m
> @@ -16,6 +16,9 @@
>   #include "trace.h"
>   #include "qemu-main.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/cutils.h"
> +#include "qapi/visitor.h"
> +#include "qapi/error.h"
>   #include "ui/console.h"
>   #include "monitor/monitor.h"
>   #include "qapi/error.h"
> @@ -23,9 +26,10 @@
>   #include <mach/mach_vm.h>
>   #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
>   
> -static const PGDisplayCoord_t apple_gfx_modes[] = {
> -    { .x = 1440, .y = 1080 },
> -    { .x = 1280, .y = 1024 },
> +static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
> +    { 1920, 1080, 60 },
> +    { 1440, 1080, 60 },
> +    { 1280, 1024, 60 },
>   };
>   
>   typedef struct PGTask_s { // Name matches forward declaration in PG header
> @@ -264,7 +268,6 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
>   static void create_fb(AppleGFXState *s)
>   {
>       s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
> -    set_mode(s, 1440, 1080);
>   
>       s->cursor_show = true;
>   }
> @@ -466,20 +469,24 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
>       return disp_desc;
>   }
>   
> -static NSArray<PGDisplayMode*>* apple_gfx_prepare_display_mode_array(void)
> +static NSArray<PGDisplayMode*>* apple_gfx_create_display_mode_array(
> +    const AppleGFXDisplayMode display_modes[], int display_mode_count)
>   {
> -    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
> +    PGDisplayMode **modes = alloca(sizeof(modes[0]) * display_mode_count);
>       NSArray<PGDisplayMode*>* mode_array = nil;
>       int i;
>   
> -    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
> +    for (i = 0; i < display_mode_count; i++) {
> +        const AppleGFXDisplayMode *mode = &display_modes[i];
> +        PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px };
>           modes[i] =
> -            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
> +            [[PGDisplayMode alloc] initWithSizeInPixels:mode_size
> +                                        refreshRateInHz:mode->refresh_rate_hz];
>       }
>   
> -    mode_array = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
> +    mode_array = [NSArray arrayWithObjects:modes count:display_mode_count];
>   
> -    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
> +    for (i = 0; i < display_mode_count; i++) {
>           [modes[i] release];
>           modes[i] = nil;
>       }
> @@ -516,6 +523,8 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
>   void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
>   {
>       PGDisplayDescriptor *disp_desc = nil;
> +    const AppleGFXDisplayMode *display_modes = apple_gfx_default_modes;
> +    int num_display_modes = ARRAY_SIZE(apple_gfx_default_modes);
>   
>       QTAILQ_INIT(&s->tasks);
>       s->render_queue = dispatch_queue_create("apple-gfx.render",
> @@ -533,7 +542,127 @@ void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
>       s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc
>                                                 port:0 serialNum:1234];
>       [disp_desc release];
> -    s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
> +
> +    if (s->display_modes.modes != NULL && s->display_modes.modes->len > 0) {
> +        display_modes =
> +            &g_array_index(s->display_modes.modes, AppleGFXDisplayMode, 0);
> +        num_display_modes = s->display_modes.modes->len;
> +    }
> +    s->pgdisp.modeList =
> +        apple_gfx_create_display_mode_array(display_modes, num_display_modes);
>   
>       create_fb(s);
>   }
> +
> +void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
> +                                 const char *name, Error **errp)
> +{
> +    GArray *modes = mode_list->modes;
> +    /* 3 uint16s (max 5 digits) and 3 separator characters per mode + nul. */
> +    size_t buffer_size = (5 + 1) * 3 * modes->len + 1;
> +
> +    char *buffer = alloca(buffer_size);
> +    char *pos = buffer;
> +
> +    unsigned used = 0;
> +    buffer[0] = '\0';
> +    for (guint i = 0; i < modes->len; ++i)
> +    {
> +        AppleGFXDisplayMode *mode =
> +            &g_array_index(modes, AppleGFXDisplayMode, i);
> +        int rc = snprintf(pos, buffer_size - used,
> +                          "%s%"PRIu16"x%"PRIu16"@%"PRIu16,
> +                          i > 0 ? ":" : "",
> +                          mode->width_px, mode->height_px,
> +                          mode->refresh_rate_hz);
> +        used += rc;
> +        pos += rc;
> +        assert(used < buffer_size);
> +    }
> +
> +    pos = buffer;
> +    visit_type_str(v, name, &pos, errp);
> +}
> +
> +void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
> +                                 const char *name, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    const char *endptr;
> +    char *str;
> +    int ret;
> +    unsigned int val;
> +    uint32_t num_modes;
> +    GArray *modes;
> +    uint32_t mode_idx;
> +
> +    visit_type_str(v, name, &str, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    // Count colons to estimate modes. No leading/trailing colons so start at 1.
> +    num_modes = 1;
> +    for (size_t i = 0; str[i] != '\0'; ++i)
> +    {
> +        if (str[i] == ':') {
> +            ++num_modes;
> +        }
> +    }
> +
> +    modes = g_array_sized_new(false, true, sizeof(AppleGFXDisplayMode), num_modes);
> +
> +    endptr = str;
> +    for (mode_idx = 0; mode_idx < num_modes; ++mode_idx)
> +    {
> +        AppleGFXDisplayMode mode = {};
> +        if (mode_idx > 0)
> +        {
> +            if (*endptr != ':') {
> +                goto separator_error;
> +            }
> +            ++endptr;
> +        }
> +
> +        ret = qemu_strtoui(endptr, &endptr, 10, &val);
> +        if (ret || val > UINT16_MAX || val == 0) {
> +            error_setg(errp, "width of '%s' must be a decimal integer number "
> +                       "of pixels in the range 1..65535", name);
> +            goto out;
> +        }
> +        mode.width_px = val;
> +        if (*endptr != 'x') {
> +            goto separator_error;
> +        }
> +
> +        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
> +        if (ret || val > UINT16_MAX || val == 0) {
> +            error_setg(errp, "height of '%s' must be a decimal integer number "
> +                       "of pixels in the range 1..65535", name);
> +            goto out;
> +        }
> +        mode.height_px = val;
> +        if (*endptr != '@') {
> +            goto separator_error;
> +        }
> +
> +        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
> +        if (ret) {
> +            error_setg(errp, "refresh rate of '%s'"
> +                       " must be a non-negative decimal integer (Hertz)", name);
> +        }
> +        mode.refresh_rate_hz = val;
> +        g_array_append_val(modes, mode);
> +    }
> +
> +    mode_list->modes = modes;
> +    goto out;
> +
> +separator_error:
> +    error_setg(errp, "Each display mode takes the format "
> +               "'<width>x<height>@<rate>', modes are separated by colons. (:)");
> +out:
> +    g_free(str);
> +    return;
> +}
Re: [PATCH v3 04/14] hw/display/apple-gfx: Adds configurable mode list
Posted by Phil Dennis-Jordan 1 month, 2 weeks ago
On Fri, 4 Oct 2024 at 06:17, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> > This change adds a property 'display_modes' on the graphics device
> > which permits specifying a list of display modes. (screen resolution
> > and refresh rate)
> >
> > PCI variant of apple-gfx only for the moment.
> >
> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> > ---
> >   hw/display/apple-gfx-pci.m |  43 ++++++++++-
> >   hw/display/apple-gfx.h     |  17 ++++-
> >   hw/display/apple-gfx.m     | 151 ++++++++++++++++++++++++++++++++++---
> >   3 files changed, 198 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
> > index 9370258ee46..ea86a1f4a21 100644
> > --- a/hw/display/apple-gfx-pci.m
> > +++ b/hw/display/apple-gfx-pci.m
> > @@ -16,6 +16,7 @@
> >   #include "apple-gfx.h"
> >   #include "hw/pci/pci_device.h"
> >   #include "hw/pci/msi.h"
> > +#include "hw/qdev-properties.h"
> >   #include "qapi/error.h"
> >   #include "trace.h"
> >   #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
> > @@ -101,6 +102,46 @@ static void apple_gfx_pci_reset(Object *obj,
> ResetType type)
> >       [s->common.pgdev reset];
> >   }
> >
> > +static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
> > +                                            const char *name, void
> *opaque,
> > +                                            Error **errp)
> > +{
> > +    Property *prop = opaque;
> > +    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj,
> prop);
> > +
> > +    apple_gfx_get_display_modes(mode_list, v, name, errp);
> > +}
> > +
> > +static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
> > +                                            const char *name, void
> *opaque,
> > +                                            Error **errp)
> > +{
> > +    Property *prop = opaque;
> > +    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj,
> prop);
> > +
> > +    apple_gfx_set_display_modes(mode_list, v, name, errp);
> > +}
> > +
> > +const PropertyInfo apple_gfx_pci_prop_display_modes = {
> > +    .name  = "display_modes",
> > +    .description =
> > +        "Colon-separated list of display modes; "
> > +        "<width>x<height>@<refresh-rate>; the first mode is considered "
> > +        "'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",
>
> Please use DEFINE_PROP_ARRAY() instead of inventing your own way to
> define an array.
>

Looks like the ability to specify array properties on the Qemu command line
has recently been added. I'm pretty sure I still need the custom set/get
implementations for each array element (display mode) though.


> > +    .get   = apple_gfx_pci_get_display_modes,
> > +    .set   = apple_gfx_pci_set_display_modes,
> > +};
> > +
> > +#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
> > +    DEFINE_PROP(_name, _state, _field,
> apple_gfx_pci_prop_display_modes, \
> > +                AppleGFXDisplayModeList)
> > +
> > +static Property apple_gfx_pci_properties[] = {
> > +    DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
> > +                              common.display_modes),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >   static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -118,7 +159,7 @@ static void apple_gfx_pci_class_init(ObjectClass
> *klass, void *data)
> >       pci->class_id = PCI_CLASS_DISPLAY_OTHER;
> >       pci->realize = apple_gfx_pci_realize;
> >
> > -    // TODO: Property for setting mode list
> > +    device_class_set_props(dc, apple_gfx_pci_properties);
> >   }
> >
> >   static TypeInfo apple_gfx_pci_types[] = {
> > diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
> > index 995ecf7f4a7..baad4a98652 100644
> > --- a/hw/display/apple-gfx.h
> > +++ b/hw/display/apple-gfx.h
> > @@ -5,14 +5,28 @@
> >   #define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
> >
> >   #include "qemu/typedefs.h"
> > +#include "qemu/osdep.h"
> >
> >   typedef struct AppleGFXState AppleGFXState;
> >
> > +typedef struct AppleGFXDisplayMode {
> > +    uint16_t width_px;
> > +    uint16_t height_px;
> > +    uint16_t refresh_rate_hz;
> > +} AppleGFXDisplayMode;
> > +
> > +typedef struct AppleGFXDisplayModeList {
> > +    GArray *modes;
> > +} AppleGFXDisplayModeList;
> > +
> >   void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char*
> obj_name);
> > +void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list,
> Visitor *v,
> > +                                 const char *name, Error **errp);
> > +void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list,
> Visitor *v,
> > +                                 const char *name, Error **errp);
> >
> >   #ifdef __OBJC__
> >
> > -#include "qemu/osdep.h"
> >   #include "exec/memory.h"
> >   #include "ui/surface.h"
> >   #include <dispatch/dispatch.h>
> > @@ -38,6 +52,7 @@ struct AppleGFXState {
> >       bool new_frame;
> >       bool cursor_show;
> >       QEMUCursor *cursor;
> > +    AppleGFXDisplayModeList display_modes;
> >
> >       dispatch_queue_t render_queue;
> >       /* The following fields should only be accessed from render_queue:
> */
> > diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
> > index 6ef1048d93d..358192db6a0 100644
> > --- a/hw/display/apple-gfx.m
> > +++ b/hw/display/apple-gfx.m
> > @@ -16,6 +16,9 @@
> >   #include "trace.h"
> >   #include "qemu-main.h"
> >   #include "qemu/main-loop.h"
> > +#include "qemu/cutils.h"
> > +#include "qapi/visitor.h"
> > +#include "qapi/error.h"
> >   #include "ui/console.h"
> >   #include "monitor/monitor.h"
> >   #include "qapi/error.h"
> > @@ -23,9 +26,10 @@
> >   #include <mach/mach_vm.h>
> >   #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
> >
> > -static const PGDisplayCoord_t apple_gfx_modes[] = {
> > -    { .x = 1440, .y = 1080 },
> > -    { .x = 1280, .y = 1024 },
> > +static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
> > +    { 1920, 1080, 60 },
> > +    { 1440, 1080, 60 },
> > +    { 1280, 1024, 60 },
> >   };
> >
> >   typedef struct PGTask_s { // Name matches forward declaration in PG
> header
> > @@ -264,7 +268,6 @@ static void set_mode(AppleGFXState *s, uint32_t
> width, uint32_t height)
> >   static void create_fb(AppleGFXState *s)
> >   {
> >       s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
> > -    set_mode(s, 1440, 1080);
> >
> >       s->cursor_show = true;
> >   }
> > @@ -466,20 +469,24 @@ static void
> apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
> >       return disp_desc;
> >   }
> >
> > -static NSArray<PGDisplayMode*>*
> apple_gfx_prepare_display_mode_array(void)
> > +static NSArray<PGDisplayMode*>* apple_gfx_create_display_mode_array(
> > +    const AppleGFXDisplayMode display_modes[], int display_mode_count)
> >   {
> > -    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
> > +    PGDisplayMode **modes = alloca(sizeof(modes[0]) *
> display_mode_count);
> >       NSArray<PGDisplayMode*>* mode_array = nil;
> >       int i;
> >
> > -    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
> > +    for (i = 0; i < display_mode_count; i++) {
> > +        const AppleGFXDisplayMode *mode = &display_modes[i];
> > +        PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px
> };
> >           modes[i] =
> > -            [[PGDisplayMode alloc]
> initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
> > +            [[PGDisplayMode alloc] initWithSizeInPixels:mode_size
> > +
> refreshRateInHz:mode->refresh_rate_hz];
> >       }
> >
> > -    mode_array = [NSArray arrayWithObjects:modes
> count:ARRAY_SIZE(apple_gfx_modes)];
> > +    mode_array = [NSArray arrayWithObjects:modes
> count:display_mode_count];
> >
> > -    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
> > +    for (i = 0; i < display_mode_count; i++) {
> >           [modes[i] release];
> >           modes[i] = nil;
> >       }
> > @@ -516,6 +523,8 @@ static void
> apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
> >   void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor
> *desc)
> >   {
> >       PGDisplayDescriptor *disp_desc = nil;
> > +    const AppleGFXDisplayMode *display_modes = apple_gfx_default_modes;
> > +    int num_display_modes = ARRAY_SIZE(apple_gfx_default_modes);
> >
> >       QTAILQ_INIT(&s->tasks);
> >       s->render_queue = dispatch_queue_create("apple-gfx.render",
> > @@ -533,7 +542,127 @@ void apple_gfx_common_realize(AppleGFXState *s,
> PGDeviceDescriptor *desc)
> >       s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc
> >                                                 port:0 serialNum:1234];
> >       [disp_desc release];
> > -    s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
> > +
> > +    if (s->display_modes.modes != NULL && s->display_modes.modes->len >
> 0) {
> > +        display_modes =
> > +            &g_array_index(s->display_modes.modes, AppleGFXDisplayMode,
> 0);
> > +        num_display_modes = s->display_modes.modes->len;
> > +    }
> > +    s->pgdisp.modeList =
> > +        apple_gfx_create_display_mode_array(display_modes,
> num_display_modes);
> >
> >       create_fb(s);
> >   }
> > +
> > +void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list,
> Visitor *v,
> > +                                 const char *name, Error **errp)
> > +{
> > +    GArray *modes = mode_list->modes;
> > +    /* 3 uint16s (max 5 digits) and 3 separator characters per mode +
> nul. */
> > +    size_t buffer_size = (5 + 1) * 3 * modes->len + 1;
> > +
> > +    char *buffer = alloca(buffer_size);
> > +    char *pos = buffer;
> > +
> > +    unsigned used = 0;
> > +    buffer[0] = '\0';
> > +    for (guint i = 0; i < modes->len; ++i)
> > +    {
> > +        AppleGFXDisplayMode *mode =
> > +            &g_array_index(modes, AppleGFXDisplayMode, i);
> > +        int rc = snprintf(pos, buffer_size - used,
> > +                          "%s%"PRIu16"x%"PRIu16"@%"PRIu16,
> > +                          i > 0 ? ":" : "",
> > +                          mode->width_px, mode->height_px,
> > +                          mode->refresh_rate_hz);
> > +        used += rc;
> > +        pos += rc;
> > +        assert(used < buffer_size);
> > +    }
> > +
> > +    pos = buffer;
> > +    visit_type_str(v, name, &pos, errp);
> > +}
> > +
> > +void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list,
> Visitor *v,
> > +                                 const char *name, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    const char *endptr;
> > +    char *str;
> > +    int ret;
> > +    unsigned int val;
> > +    uint32_t num_modes;
> > +    GArray *modes;
> > +    uint32_t mode_idx;
> > +
> > +    visit_type_str(v, name, &str, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    // Count colons to estimate modes. No leading/trailing colons so
> start at 1.
> > +    num_modes = 1;
> > +    for (size_t i = 0; str[i] != '\0'; ++i)
> > +    {
> > +        if (str[i] == ':') {
> > +            ++num_modes;
> > +        }
> > +    }
> > +
> > +    modes = g_array_sized_new(false, true, sizeof(AppleGFXDisplayMode),
> num_modes);
> > +
> > +    endptr = str;
> > +    for (mode_idx = 0; mode_idx < num_modes; ++mode_idx)
> > +    {
> > +        AppleGFXDisplayMode mode = {};
> > +        if (mode_idx > 0)
> > +        {
> > +            if (*endptr != ':') {
> > +                goto separator_error;
> > +            }
> > +            ++endptr;
> > +        }
> > +
> > +        ret = qemu_strtoui(endptr, &endptr, 10, &val);
> > +        if (ret || val > UINT16_MAX || val == 0) {
> > +            error_setg(errp, "width of '%s' must be a decimal integer
> number "
> > +                       "of pixels in the range 1..65535", name);
> > +            goto out;
> > +        }
> > +        mode.width_px = val;
> > +        if (*endptr != 'x') {
> > +            goto separator_error;
> > +        }
> > +
> > +        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
> > +        if (ret || val > UINT16_MAX || val == 0) {
> > +            error_setg(errp, "height of '%s' must be a decimal integer
> number "
> > +                       "of pixels in the range 1..65535", name);
> > +            goto out;
> > +        }
> > +        mode.height_px = val;
> > +        if (*endptr != '@') {
> > +            goto separator_error;
> > +        }
> > +
> > +        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
> > +        if (ret) {
> > +            error_setg(errp, "refresh rate of '%s'"
> > +                       " must be a non-negative decimal integer
> (Hertz)", name);
> > +        }
> > +        mode.refresh_rate_hz = val;
> > +        g_array_append_val(modes, mode);
> > +    }
> > +
> > +    mode_list->modes = modes;
> > +    goto out;
> > +
> > +separator_error:
> > +    error_setg(errp, "Each display mode takes the format "
> > +               "'<width>x<height>@<rate>', modes are separated by
> colons. (:)");
> > +out:
> > +    g_free(str);
> > +    return;
> > +}
>
>