qemu-block
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 04/15] hw/display/apple-gfx: Adds configurable mode list

From: Akihiko Odaki
Subject: Re: [PATCH v6 04/15] hw/display/apple-gfx: Adds configurable mode list
Date: Tue, 5 Nov 2024 17:31:50 +0900
User-agent: Mozilla Thunderbird 
On 2024/11/04 0:00, 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)

The property is an array of a custom type to make the syntax slightly
less awkward to use, for example:

-device '{"driver":"apple-gfx-pci", "display-modes":["1920x1080@60", 
"3840x2160@60"]}'

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---

v4:

  * Switched to the native array property type, which recently gained
         command line support.
  * The property has also been added to the -mmio variant.
  * Tidied up the code a little.

v5:

  * Better error handling and buffer management in property parsing and
    output.

v6:

  * Switched to using NSMutableArray for the mode list to avoid need for
    allocating a temporary array - previously done with alloca.

  hw/display/apple-gfx-mmio.m |   8 +++
  hw/display/apple-gfx-pci.m  |   9 ++-
  hw/display/apple-gfx.h      |  12 ++++
  hw/display/apple-gfx.m      | 137 +++++++++++++++++++++++++++++++-----
  hw/display/trace-events     |   2 +
  5 files changed, 148 insertions(+), 20 deletions(-)

diff --git a/hw/display/apple-gfx-mmio.m b/hw/display/apple-gfx-mmio.m
index 66d81d38326..ddc8a2fc4f3 100644
--- a/hw/display/apple-gfx-mmio.m
+++ b/hw/display/apple-gfx-mmio.m
@@ -258,6 +258,12 @@ static void apple_gfx_mmio_reset(Object *obj, ResetType 
type)
      [s->common.pgdev reset];
  }
+static Property apple_gfx_mmio_properties[] = { 
+    DEFINE_PROP_ARRAY("display-modes", AppleGFXMMIOState,
+                      common.num_display_modes, common.display_modes,
+                      qdev_prop_display_mode, AppleGFXDisplayMode),
+    DEFINE_PROP_END_OF_LIST(),
+};
static void apple_gfx_mmio_class_init(ObjectClass *klass, void *data) 
  {
@@ -267,6 +273,8 @@ static void apple_gfx_mmio_class_init(ObjectClass *klass, 
void *data)
      rc->phases.hold = apple_gfx_mmio_reset;
      dc->hotpluggable = false;
      dc->realize = apple_gfx_mmio_realize;
+
+    device_class_set_props(dc, apple_gfx_mmio_properties);
  }
static TypeInfo apple_gfx_mmio_types[] = { 
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index 870d0c41e66..e2fa2567c99 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -113,6 +113,13 @@ static void apple_gfx_pci_reset(Object *obj, ResetType 
type)
      [s->common.pgdev reset];
  }
+static Property apple_gfx_pci_properties[] = { 
+    DEFINE_PROP_ARRAY("display-modes", AppleGFXPCIState,
+                      common.num_display_modes, common.display_modes,
+                      qdev_prop_display_mode, AppleGFXDisplayMode),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
  static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);
@@ -129,7 +136,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 91b55299338..3868ea87391 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -16,6 +16,7 @@
  #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
  #include "qemu/typedefs.h"
  #include "exec/memory.h"
+#include "hw/qdev-properties.h"
  #include "ui/surface.h"
@class PGDeviceDescriptor; 
@@ -27,6 +28,7 @@
typedef QTAILQ_HEAD(, PGTask_s) PGTaskList; +struct AppleGFXDisplayMode; 
  typedef struct AppleGFXState {
      /* Initialised on init/realize() */
      MemoryRegion iomem_gfx;
@@ -36,6 +38,8 @@ typedef struct AppleGFXState {
      id<MTLDevice> mtl;
      id<MTLCommandQueue> mtl_queue;
      dispatch_queue_t render_queue;
+    struct AppleGFXDisplayMode *display_modes;
+    uint32_t num_display_modes;
/* List `tasks` is protected by task_mutex */ 
      QemuMutex task_mutex;
@@ -54,6 +58,12 @@ typedef struct AppleGFXState {
      bool cursor_show;
  } AppleGFXState;
+typedef struct AppleGFXDisplayMode { 
+    uint16_t width_px;
+    uint16_t height_px;
+    uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
  void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* 
obj_name);
  void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc,
                                Error **errp);
@@ -61,5 +71,7 @@ uintptr_t apple_gfx_host_address_for_gpa_range(uint64_t 
guest_physical,
                                                 uint64_t length, bool 
read_only,
                                                 MemoryRegion 
**mapping_in_region);
+extern const PropertyInfo qdev_prop_display_mode; 
+
  #endif
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m 
index bce08e57c3f..c4acefccee8 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -31,9 +31,10 @@
  #include "sysemu/dma.h"
  #include "ui/console.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 },
  };
/* ------ PGTask and task operations: new/destroy/map/unmap ------ */ 
@@ -688,22 +689,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[], uint32_t display_mode_count)
  {
-    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
-    NSArray<PGDisplayMode*>* mode_array = nil;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
-        modes[i] =
-            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] 
refreshRateInHz:60.];
-    }
-
-    mode_array = [NSArray arrayWithObjects:modes 
count:ARRAY_SIZE(apple_gfx_modes)];
-
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
-        [modes[i] release];
-        modes[i] = nil;
+    uint32_t i;
+    PGDisplayMode *mode_obj;
+    NSMutableArray<PGDisplayMode *> *mode_array =
+        [[NSMutableArray alloc] initWithCapacity:display_mode_count];
+
+    for (i = 0; i < display_mode_count; i++) {
+        const AppleGFXDisplayMode *mode = &display_modes[i];
+        trace_apple_gfx_display_mode(i, mode->width_px, mode->height_px);
+        PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px };
+
+        mode_obj =
+            [[PGDisplayMode alloc] initWithSizeInPixels:mode_size
+                                        refreshRateInHz:mode->refresh_rate_hz];
+        [mode_array addObject:mode_obj];
+        [mode_obj release];
      }
return mode_array; 
@@ -739,6 +742,9 @@ void apple_gfx_common_realize(AppleGFXState *s, 
PGDeviceDescriptor *desc,
                                Error **errp)
  {
      PGDisplayDescriptor *disp_desc = nil;
+    const AppleGFXDisplayMode *display_modes = apple_gfx_default_modes;
+    int num_display_modes = ARRAY_SIZE(apple_gfx_default_modes);
+    NSArray<PGDisplayMode *> *mode_array;
if (apple_gfx_mig_blocker == NULL) { 
          error_setg(&apple_gfx_mig_blocker,
@@ -765,9 +771,102 @@ 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 != NULL && s->num_display_modes > 0) {
+        trace_apple_gfx_common_realize_modes_property(s->num_display_modes);
+        display_modes = s->display_modes;
+        num_display_modes = s->num_display_modes;
+    }
+    s->pgdisp.modeList = mode_array =
+        apple_gfx_create_display_mode_array(display_modes, num_display_modes);
+    [mode_array release];
s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s); qatomic_set(&s->cursor_show, true); 
  }
+
+/* ------ Display mode list device property ------ */
+
+static void apple_gfx_get_display_mode(Object *obj, Visitor *v,
+                                       const char *name, void *opaque,
+                                       Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayMode *mode = object_field_prop_ptr(obj, prop);
+    /* 3 uint16s (max 5 digits) + 2 separator characters + nul. */
+    char buffer[5 * 3 + 2 + 1];
+    char *pos = buffer;
+
+    int rc = snprintf(buffer, sizeof(buffer),
+                      "%"PRIu16"x%"PRIu16"@%"PRIu16,
+                      mode->width_px, mode->height_px,
+                      mode->refresh_rate_hz);
+    assert(rc < sizeof(buffer));
+
+    visit_type_str(v, name, &pos, errp);
+}
+
+static void apple_gfx_set_display_mode(Object *obj, Visitor *v,
+                                       const char *name, void *opaque,
+                                       Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayMode *mode = object_field_prop_ptr(obj, prop);
+    Error *local_err = NULL;
+    const char *endptr;
+    g_autofree char *str = NULL;
+    int ret;
+    int val;
+
+    visit_type_str(v, name, &str, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);


Do not use error_propagae(); just pass errp instead.


+        return;
+    }
+
+    endptr = str;
+
+    ret = qemu_strtoi(endptr, &endptr, 10, &val);
+    if (ret || val > UINT16_MAX || val <= 0) {
+        error_setg(errp, "width in '%s' must be a decimal integer number "
+                   "of pixels in the range 1..65535", name);
+        return;
+    }
+    mode->width_px = val;
+    if (*endptr != 'x') {
+        goto separator_error;
+    }
+
+    ret = qemu_strtoi(endptr + 1, &endptr, 10, &val);
+    if (ret || val > UINT16_MAX || val <= 0) {
+        error_setg(errp, "height in '%s' must be a decimal integer number "
+                   "of pixels in the range 1..65535", name);
+        return;
+    }
+    mode->height_px = val;
+    if (*endptr != '@') {
+        goto separator_error;
+    }
+
+    ret = qemu_strtoi(endptr + 1, &endptr, 10, &val);
+    if (ret || val > UINT16_MAX || val <= 0) {
+        error_setg(errp, "refresh rate in '%s'"
+                   " must be a positive decimal integer (Hertz)", name);


I prefer having a return statement here for semantic clarity.


+    }
+    mode->refresh_rate_hz = val;
+    return;
+
+separator_error:
+    error_setg(errp, "Each display mode takes the format "
+               "'<width>x<height>@<rate>'");
+}
+
+const PropertyInfo qdev_prop_display_mode = {
+    .name  = "display_mode",
+    .description =
+        "Display mode in pixels and Hertz, as <width>x<height>@<refresh-rate> "
+        "Example: 3840x2160@60",
+    .get   = apple_gfx_get_display_mode,
+    .set   = apple_gfx_set_display_mode,
+};
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 260eab9146d..4987cc2d357 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -212,6 +212,8 @@ apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t 
height) "bpp=%d widt
  apple_gfx_cursor_show(uint32_t show) "show=%d"
  apple_gfx_cursor_move(void) ""
  apple_gfx_common_init(const char *device_name, size_t mmio_size) "device: %s; MMIO 
size: %zu bytes"
+apple_gfx_common_realize_modes_property(uint32_t num_modes) "using %u modes 
supplied by 'display-modes' device property"
+apple_gfx_display_mode(uint32_t mode_idx, uint16_t width_px, uint16_t height_px) "mode 
%2"PRIu32": %4"PRIu16"x%4"PRIu16
# apple-gfx-mmio.m 
  apple_gfx_mmio_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" 
res=0x%"PRIx64
reply via email to
[Prev in Thread] Current Thread [Next in Thread]