[PATCH 1/7] winemac.drv: Add macdrv_get_gpus() helper.

[Zhiyi Zhang]

Signed-off-by: Zhiyi Zhang 
---
Maybe Mac driver would draw more attention :)

dlls/winemac.drv/cocoa_display.m | 168 +++++++++++++++++++++++++++++++++++++++
 dlls/winemac.drv/macdrv_cocoa.h  |  17 ++++
 2 files changed, 185 insertions(+)

diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
index 93a0fbca35..569cd32352 100644
--- a/dlls/winemac.drv/cocoa_display.m
+++ b/dlls/winemac.drv/cocoa_display.m
@@ -19,6 +19,7 @@
  */

#import <AppKit/AppKit.h>
+#import <Metal/Metal.h>
 #include "macdrv_cocoa.h"

@@ -103,3 +104,170 @@ void macdrv_free_displays(struct macdrv_display* displays)
 {
     free(displays);
 }
+
+/***********************************************************************
+ *              get_entry_property_uint32
+ *
+ * Get an IO registry entry property of type uint32 and store it in value parameter.
+ *
+ * Return 0 on failure.
+ */
+static uint32_t get_entry_property_uint32(io_registry_entry_t entry, CFStringRef property_name, uint32_t* value)
+{
+    CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
+                                                     kIORegistryIterateRecursively | kIORegistryIterateParents);
+    if (!data)
+        return 0;
+
+    const uint32_t* bytes = (const uint32_t*)(CFDataGetBytePtr(data));
+    *value = *bytes;
+
+    CFRelease(data);
+    return 1;
+}
+
+/***********************************************************************
+ *              get_entry_property_string
+ *
+ * Get an IO registry entry property of type string and write it in buffer parameter.
+ *
+ * Return the size written into the buffer.
+ */
+static size_t get_entry_property_string(io_registry_entry_t entry, CFStringRef property_name, char* buffer,
+                                        size_t buffer_size)
+{
+    size_t length;
+
+    CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
+                                                     kIORegistryIterateRecursively | kIORegistryIterateParents);
+    if (!data)
+        return 0;
+
+    length = CFDataGetLength(data);
+    if (length > buffer_size)
+        return 0;
+
+    memcpy(buffer, CFDataGetBytePtr(data), length);
+    CFRelease(data);
+    return length;
+}
+
+/***********************************************************************
+ *              macdrv_get_gpu_info_from_entry
+ *
+ * Get GPU information from an IO registry entry.
+ */
+static void macdrv_get_gpu_info_from_entry(struct macdrv_gpu* gpu, io_registry_entry_t entry)
+{
+    get_entry_property_uint32(entry, CFSTR("vendor-id"), &gpu->vendor_id);
+    get_entry_property_uint32(entry, CFSTR("device-id"), &gpu->device_id);
+    get_entry_property_uint32(entry, CFSTR("subsystem-id"), &gpu->subsys_id);
+    get_entry_property_uint32(entry, CFSTR("revision-id"), &gpu->revision_id);
+    get_entry_property_string(entry, CFSTR("model"), gpu->name, sizeof(gpu->name));
+}
+
+/***********************************************************************
+ *              macdrv_get_gpu_info_from_display_id
+ *
+ * Get GPU information from a display id.
+ *
+ * This is a fallback for 32bit build since Metal doesn't support 32bit.
+ * Thus registryID can't be used.
+ */
+void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)
+{
+    io_registry_entry_t entry = CGDisplayIOServicePort(display_id);
+    macdrv_get_gpu_info_from_entry(gpu, entry);
+}
+
+/***********************************************************************
+ *              macdrv_get_gpu_info_from_registry_id
+ *
+ * Get GPU information from a Metal device registry id.
+ */
+void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)
+{
+    /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
+    io_registry_entry_t entry =
+        IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
+    if (!entry)
+        return;
+
+    io_registry_entry_t parent;
+    if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)
+    {
+        macdrv_get_gpu_info_from_entry(gpu, entry);
+        IOObjectRelease(parent);
+    }
+    IOObjectRelease(entry);
+}
+
+/***********************************************************************
+ *              macdrv_get_gpus
+ *
+ * Get a list of GPU currently in the system. The first GPU is primary.
+ * Call macdrv_free_gpus() when you are done using the data.
+ *
+ * Return -1 on failure with parameters unchanged.
+ */
+int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
+{
+    struct macdrv_gpu* gpus;
+    CGDirectDisplayID primary_display;
+    id<MTLDevice> primary_device;
+    int primary_index = 0;
+    int gpu_count;
+    int i;
+    NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
+
+    NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
+    gpu_count = devices.count ? devices.count : 1;
+    gpus = calloc(gpu_count, sizeof(*gpus));
+    if (!gpus)
+        return -1;
+
+    primary_display = CGMainDisplayID();
+    primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
+
+    /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
+    if (!devices.count)
+    {
+        gpus[0].id = 0;
+        macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
+    }
+    else
+    {
+        for (i = 0; i < devices.count; i++)
+        {
+            gpus[i].id = devices[i].registryID;
+            macdrv_get_gpu_info_from_registry_id(&gpus[i], gpus[i].id);
+
+            if (gpus[i].id == primary_device.registryID)
+                primary_index = i;
+        }
+
+        /* Make sure the first GPU is primary */
+        if (primary_index)
+        {
+            struct macdrv_gpu tmp;
+            tmp = gpus[0];
+            gpus[0] = gpus[primary_index];
+            gpus[primary_index] = tmp;
+        }
+    }
+
+    [pool release];
+    *new_gpus = gpus;
+    *count = gpu_count;
+    return 0;
+}
+
+/***********************************************************************
+ *              macdrv_free_gpus
+ *
+ * Free a GPU list allocated from macdrv_get_gpus()
+ */
+void macdrv_free_gpus(struct macdrv_gpu* gpus)
+{
+    free(gpus);
+}
diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h
index 6165dc5874..a6d8ec91d4 100644
--- a/dlls/winemac.drv/macdrv_cocoa.h
+++ b/dlls/winemac.drv/macdrv_cocoa.h
@@ -257,10 +257,27 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)

/* display */
+
+/* Represent a physical GPU in the PCI slots */
+struct macdrv_gpu
+{
+    /* Metal device registry id. Zero on 32bit build */
+    uint64_t id;
+    /* Name, in UTF-8 encoding */
+    char name[128];
+    /* PCI ID */
+    uint32_t vendor_id;
+    uint32_t device_id;
+    uint32_t subsys_id;
+    uint32_t revision_id;
+};
+
 extern int macdrv_get_displays(struct macdrv_display** displays, int* count) DECLSPEC_HIDDEN;
 extern void macdrv_free_displays(struct macdrv_display* displays) DECLSPEC_HIDDEN;
 extern int macdrv_set_display_mode(const struct macdrv_display* display,
                                    CGDisplayModeRef display_mode) DECLSPEC_HIDDEN;
+extern int macdrv_get_gpus(struct macdrv_gpu** gpus, int* count) DECLSPEC_HIDDEN;
+extern void macdrv_free_gpus(struct macdrv_gpu* gpus) DECLSPEC_HIDDEN;

/* event */

[Chip Davis]

April 22, 2019 7:14 AM, "Zhiyi Zhang"  wrote:
> diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> index 93a0fbca35..569cd32352 100644
> --- a/dlls/winemac.drv/cocoa_display.m
> +++ b/dlls/winemac.drv/cocoa_display.m
> @@ -19,6 +19,7 @@
>  */
>
> #import <AppKit/AppKit.h>
> +#import <Metal/Metal.h>

This needs to be guarded with HAVE_METAL_METAL_H.
> #include "macdrv_cocoa.h"
>
> @@ -103,3 +104,170 @@ void macdrv_free_displays(struct macdrv_display* displays)

[...]
> +/***********************************************************************
> -
> -
>          macdrv_get_gpus
> -
> -
> -
> - Get a list of GPU currently in the system. The first GPU is primary.
> -
> - Call macdrv_free_gpus() when you are done using the data.
> -
> -
> -
> - Return -1 on failure with parameters unchanged.
> - */
>
> +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> +{
> - struct macdrv_gpu* gpus;
> - CGDirectDisplayID primary_display;
> - id<MTLDevice> primary_device;

This too.
> - int primary_index = 0;
> - int gpu_count;
> - int i;
> - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> -
> - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();

And this. You may want to consider an alternative method of finding all the GPUs, to avoid a bunch of #ifdefs.
> - gpu_count = devices.count ? devices.count : 1;
> - gpus = calloc(gpu_count, sizeof(*gpus));
> - if (!gpus)
> -
>    return -1;
> -
> - primary_display = CGMainDisplayID();
> - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
> -
> - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> - if (!devices.count)
> - {
> -
>    gpus[0].id = 0;
> -
>    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
> - }
> - else
> - {
> -
>    for (i = 0; i < devices.count; i++)
> -
>    {
> -
>        gpus[i].id = devices[i].registryID;

The 'registryID' property is only guaranteed to be present on 10.13 and up. You'll need to check for it before using it, or else you'll take an exception.

Chip

[Zhiyi Zhang]

On 4/23/19 12:15 AM, Chip Davis wrote:
> April 22, 2019 7:14 AM, "Zhiyi Zhang"  wrote:
> > diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> > index 93a0fbca35..569cd32352 100644
> > --- a/dlls/winemac.drv/cocoa_display.m
> > +++ b/dlls/winemac.drv/cocoa_display.m
> > @@ -19,6 +19,7 @@
> >  */
> >
> > #import <AppKit/AppKit.h>
> > +#import <Metal/Metal.h>
>
> This needs to be guarded with HAVE_METAL_METAL_H.
> > #include "macdrv_cocoa.h"
> >
> > @@ -103,3 +104,170 @@ void macdrv_free_displays(struct macdrv_display* displays)
>
> [...]
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_gpus
> > -
> > -
> > -
> > - Get a list of GPU currently in the system. The first GPU is primary.
> > -
> > - Call macdrv_free_gpus() when you are done using the data.
> > -
> > -
> > -
> > - Return -1 on failure with parameters unchanged.
> > - */
> >
> > +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> > +{
> > - struct macdrv_gpu* gpus;
> > - CGDirectDisplayID primary_display;
> > - id<MTLDevice> primary_device;
>
> This too.
> > - int primary_index = 0;
> > - int gpu_count;
> > - int i;
> > - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> > -
> > - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
>
> And this. You may want to consider an alternative method of finding all the GPUs, to avoid a bunch of #ifdefs.
> > - gpu_count = devices.count ? devices.count : 1;
> > - gpus = calloc(gpu_count, sizeof(*gpus));
> > - if (!gpus)
> > -
> >    return -1;
> > -
> > - primary_display = CGMainDisplayID();
> > - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
> > -
> > - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> > - if (!devices.count)
> > - {
> > -
> >    gpus[0].id = 0;
> > -
> >    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
> > - }
> > - else
> > - {
> > -
> >    for (i = 0; i < devices.count; i++)
> > -
> >    {
> > -
> >        gpus[i].id = devices[i].registryID;
>
> The 'registryID' property is only guaranteed to be present on 10.13 and up. You'll need to check for it before using it, or else you'll take an exception.

Thanks. I will look into if I need Metal support at all.
> Chip

[Ken Thomases]

On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> Signed-off-by: Zhiyi Zhang
>
> Maybe Mac driver would draw more attention :)
>
> dlls/winemac.drv/cocoa_display.m | 168 +++++++++++++++++++++++++++++++++++++++
> dlls/winemac.drv/macdrv_cocoa.h  |  17 ++++
> 2 files changed, 185 insertions(+)
>
> diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> index 93a0fbca35..569cd32352 100644
> --- a/dlls/winemac.drv/cocoa_display.m
> +++ b/dlls/winemac.drv/cocoa_display.m
> @@ -19,6 +19,7 @@
>  */
>
> #import <AppKit/AppKit.h>
> +#import <Metal/Metal.h>
> #include "macdrv_cocoa.h"
>
> @@ -103,3 +104,170 @@ void macdrv_free_displays(struct macdrv_display* displays)
> {
>     free(displays);
> }
> -
>
> +/***********************************************************************
> -
> -
>          get_entry_property_uint32
> -
> -
> -
> - Get an IO registry entry property of type uint32 and store it in value parameter.
> -
> -
> -
> - Return 0 on failure.

Nothing actually uses the return value.
> - */
>
> +static uint32_t get_entry_property_uint32(io_registry_entry_t entry, CFStringRef property_name, uint32_t* value)
> +{
> - CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
> -
>                                                 kIORegistryIterateRecursively | kIORegistryIterateParents);
> - if (!data)
> -
>    return 0;

You should check that `data` is actually a CFData reference by comparing CFGetTypeID(data) == CFDataGetTypeID().  You should also check that its length == sizeof(uint32_t).
> -
> - const uint32_t* bytes = (const uint32_t*)(CFDataGetBytePtr(data));
> - *value = *bytes;

Probably better to use CFDataGetBytes() to copy the bytes out to value.  The byte pointer might not be aligned properly and that's not really kosher.
> -
> - CFRelease(data);
> - return 1;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          get_entry_property_string
> -
> -
> -
> - Get an IO registry entry property of type string and write it in buffer parameter.
> -
> -
> -
> - Return the size written into the buffer.

Again, nothing uses the return value.
> - */
>
> +static size_t get_entry_property_string(io_registry_entry_t entry, CFStringRef property_name, char* buffer,
> -
>                                    size_t buffer_size)
>
> +{
> - size_t length;
> -
> - CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
> -
>                                                 kIORegistryIterateRecursively | kIORegistryIterateParents);
> - if (!data)
> -
>    return 0;

Again, verify that it's a CFData.
> -
> - length = CFDataGetLength(data);
> - if (length > buffer_size)
> -
>    return 0;

This early return leaks `data`.
> -
> - memcpy(buffer, CFDataGetBytePtr(data), length);

Again, probably best to use CFDataGetBytes().

The character buffer in `data` is not guaranteed to be null-terminated.  For one of the GPUs in my Mac, it's not.  So, the length check should account for an extra byte and then you should manually null-terminate `buffer`.
> - CFRelease(data);
> - return length;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_get_gpu_info_from_entry
> -
> -
> -
> - Get GPU information from an IO registry entry.
> - */
>
> +static void macdrv_get_gpu_info_from_entry(struct macdrv_gpu* gpu, io_registry_entry_t entry)
> +{
> - get_entry_property_uint32(entry, CFSTR("vendor-id"), &gpu->vendor_id);
> - get_entry_property_uint32(entry, CFSTR("device-id"), &gpu->device_id);
> - get_entry_property_uint32(entry, CFSTR("subsystem-id"), &gpu->subsys_id);
> - get_entry_property_uint32(entry, CFSTR("revision-id"), &gpu->revision_id);
> - get_entry_property_string(entry, CFSTR("model"), gpu->name, sizeof(gpu->name));
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_get_gpu_info_from_display_id
> -
> -
> -
> - Get GPU information from a display id.
> -
> -
> -
> - This is a fallback for 32bit build since Metal doesn't support 32bit.
> -
> - Thus registryID can't be used.
> - */
>
> +void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)

This should be static.
> +{
> - io_registry_entry_t entry = CGDisplayIOServicePort(display_id);

On my Mac, this entry seems to be an IOFramebuffer entry a couple of levels below the GPU device entry.  You should probably traverse the parent chain to look for an IOPCIDevice entry.

To the extent that this works, it's probably because of the kIORegistryIterateParents option in the property search.  I worry that an intervening entry with a same-named property might break things.
> - macdrv_get_gpu_info_from_entry(gpu, entry);
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_get_gpu_info_from_registry_id
> -
> -
> -
> - Get GPU information from a Metal device registry id.
> - */
>
> +void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)

Should be static.
> +{
> - /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
> - io_registry_entry_t entry =
> -
>    IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
> - if (!entry)
> -
>    return;
> -
> - io_registry_entry_t parent;
> - if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)

If you implement the search for an IOPCIDevice ancestor I recommended above, you should probably reuse it here, too. I'm not sure how reliable it is that the GPU is the immediate parent.
> - {
> -
>    macdrv_get_gpu_info_from_entry(gpu, entry);

In any case, you didn't use `parent` in this call so looking it up was pointless. ;)  Again, this probably worked because of kIORegistryIterateParents.
> -
>    IOObjectRelease(parent);
> - }
> - IOObjectRelease(entry);
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_get_gpus
> -
> -
> -
> - Get a list of GPU currently in the system. The first GPU is primary.

"list of GPUs" (plural)
> -
> - Call macdrv_free_gpus() when you are done using the data.
> -
> -
> -
> - Return -1 on failure with parameters unchanged.
> - */
>
> +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> +{
> - struct macdrv_gpu* gpus;
> - CGDirectDisplayID primary_display;
> - id<MTLDevice> primary_device;
> - int primary_index = 0;
> - int gpu_count;
> - int i;
> - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> -
> - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();

You leak this array.  You need to either [devices release] at the end or [devices autorelease] here.
> - gpu_count = devices.count ? devices.count : 1;
> - gpus = calloc(gpu_count, sizeof(*gpus));
> - if (!gpus)
> -
>    return -1;
> -
> - primary_display = CGMainDisplayID();
> - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);

Similarly, you leak this device object.

There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.

I'm not sure there's a good fix, though.  Metal doesn't let us list which devices could theoretically drive which displays. MTLDevice does have the lowPower property, but that doesn't quite tell us what we need to know.

I've read somewhere that calling MTLCreateSystemDefaultDevice() will provoke a switch to the discrete GPU.  We could do that to determine what GPU will be primary if we're doing Metal or OpenGL rendering, but I don't know if our process will allow a switch back to the integrate GPU once we release that device or if it's marked as needing the discrete GPU for the rest of its life.
> -
> - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> - if (!devices.count)
> - {
> -
>    gpus[0].id = 0;
> -
>    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);

Hmm.  Why only report one GPU for this case?  Given that we can look up an IOPCIDevice entry from a display ID, we could enumerate all of the display IDs and accumulate all of the unique GPUs.
> - }
> - else
> - {
> -
>    for (i = 0; i < devices.count; i++)
> -
>    {
> -
>        gpus[i].id = devices[i].registryID;
> -
>        macdrv_get_gpu_info_from_registry_id(&gpus[i], gpus[i].id);
> -
> -
>        if (gpus[i].id == primary_device.registryID)
> -
>            primary_index = i;
> -
>    }
> -
> -
>    /* Make sure the first GPU is primary */
> -
>    if (primary_index)
> -
>    {
> -
>        struct macdrv_gpu tmp;
> -
>        tmp = gpus[0];
> -
>        gpus[0] = gpus[primary_index];
> -
>        gpus[primary_index] = tmp;
> -
>    }
> - }
> -
> - [pool release];
> - *new_gpus = gpus;
> - *count = gpu_count;
> - return 0;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_free_gpus
> -
> -
> -
> - Free a GPU list allocated from macdrv_get_gpus()
> - */
>
> +void macdrv_free_gpus(struct macdrv_gpu* gpus)
> +{
> - free(gpus);
>
> +}

[Zhiyi Zhang]

On 4/23/19 11:56 AM, Ken Thomases wrote:
> On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > Signed-off-by: Zhiyi Zhang
> >
> > Maybe Mac driver would draw more attention :)
> >
> > dlls/winemac.drv/cocoa_display.m | 168 +++++++++++++++++++++++++++++++++++++++
> > dlls/winemac.drv/macdrv_cocoa.h  |  17 ++++
> > 2 files changed, 185 insertions(+)
> >
> > diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> > index 93a0fbca35..569cd32352 100644
> > --- a/dlls/winemac.drv/cocoa_display.m
> > +++ b/dlls/winemac.drv/cocoa_display.m
> > @@ -19,6 +19,7 @@
> >  */
> >
> > #import <AppKit/AppKit.h>
> > +#import <Metal/Metal.h>
> > #include "macdrv_cocoa.h"
> >
> > @@ -103,3 +104,170 @@ void macdrv_free_displays(struct macdrv_display* displays)
> > {
> >     free(displays);
> > }
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          get_entry_property_uint32
> > -
> > -
> > -
> > - Get an IO registry entry property of type uint32 and store it in value parameter.
> > -
> > -
> > -
> > - Return 0 on failure.
>
> Nothing actually uses the return value.
> > - */
> >
> > +static uint32_t get_entry_property_uint32(io_registry_entry_t entry, CFStringRef property_name, uint32_t* value)
> > +{
> > - CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
> > -
> >                                                 kIORegistryIterateRecursively | kIORegistryIterateParents);
> > - if (!data)
> > -
> >    return 0;
>
> You should check that `data` is actually a CFData reference by comparing CFGetTypeID(data) == CFDataGetTypeID().  You should also check that its length == sizeof(uint32_t).
> > -
> > - const uint32_t* bytes = (const uint32_t*)(CFDataGetBytePtr(data));
> > - *value = *bytes;
>
> Probably better to use CFDataGetBytes() to copy the bytes out to value.  The byte pointer might not be aligned properly and that's not really kosher.
> > -
> > - CFRelease(data);
> > - return 1;
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          get_entry_property_string
> > -
> > -
> > -
> > - Get an IO registry entry property of type string and write it in buffer parameter.
> > -
> > -
> > -
> > - Return the size written into the buffer.
>
> Again, nothing uses the return value.
> > - */
> >
> > +static size_t get_entry_property_string(io_registry_entry_t entry, CFStringRef property_name, char* buffer,
> > -
> >                                    size_t buffer_size)
> >
> > +{
> > - size_t length;
> > -
> > - CFDataRef data = IORegistryEntrySearchCFProperty(entry, kIOServicePlane, property_name, kCFAllocatorDefault,
> > -
> >                                                 kIORegistryIterateRecursively | kIORegistryIterateParents);
> > - if (!data)
> > -
> >    return 0;
>
> Again, verify that it's a CFData.
> > -
> > - length = CFDataGetLength(data);
> > - if (length > buffer_size)
> > -
> >    return 0;
>
> This early return leaks `data`.
> > -
> > - memcpy(buffer, CFDataGetBytePtr(data), length);
>
> Again, probably best to use CFDataGetBytes().
>
> The character buffer in `data` is not guaranteed to be null-terminated.  For one of the GPUs in my Mac, it's not.  So, the length check should account for an extra byte and then you should manually null-terminate `buffer`.
> > - CFRelease(data);
> > - return length;
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_gpu_info_from_entry
> > -
> > -
> > -
> > - Get GPU information from an IO registry entry.
> > - */
> >
> > +static void macdrv_get_gpu_info_from_entry(struct macdrv_gpu* gpu, io_registry_entry_t entry)
> > +{
> > - get_entry_property_uint32(entry, CFSTR("vendor-id"), &gpu->vendor_id);
> > - get_entry_property_uint32(entry, CFSTR("device-id"), &gpu->device_id);
> > - get_entry_property_uint32(entry, CFSTR("subsystem-id"), &gpu->subsys_id);
> > - get_entry_property_uint32(entry, CFSTR("revision-id"), &gpu->revision_id);
> > - get_entry_property_string(entry, CFSTR("model"), gpu->name, sizeof(gpu->name));
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_gpu_info_from_display_id
> > -
> > -
> > -
> > - Get GPU information from a display id.
> > -
> > -
> > -
> > - This is a fallback for 32bit build since Metal doesn't support 32bit.
> > -
> > - Thus registryID can't be used.
> > - */
> >
> > +void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)
>
> This should be static.
> > +{
> > - io_registry_entry_t entry = CGDisplayIOServicePort(display_id);
>
> On my Mac, this entry seems to be an IOFramebuffer entry a couple of levels below the GPU device entry.  You should probably traverse the parent chain to look for an IOPCIDevice entry.
>
> To the extent that this works, it's probably because of the kIORegistryIterateParents option in the property search.  I worry that an intervening entry with a same-named property might break things.

Yes, it relies on kIORegistryIterateParents in the property search. The parent of the an framebuffer is the GPU.
> > - macdrv_get_gpu_info_from_entry(gpu, entry);
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_gpu_info_from_registry_id
> > -
> > -
> > -
> > - Get GPU information from a Metal device registry id.
> > - */
> >
> > +void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)
>
> Should be static.
> > +{
> > - /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
> > - io_registry_entry_t entry =
> > -
> >    IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
> > - if (!entry)
> > -
> >    return;
> > -
> > - io_registry_entry_t parent;
> > - if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)
>
> If you implement the search for an IOPCIDevice ancestor I recommended above, you should probably reuse it here, too. I'm not sure how reliable it is that the GPU is the immediate parent.

Parent is already the GPU here. MoltenVK uses this method too. I am not sure GPU is the immediate parent too but I don't think Apple would change this often?
Anyway, searching for an IOPCIDevice entry makes sense, I will use that.
> > - {
> > -
> >    macdrv_get_gpu_info_from_entry(gpu, entry);

Ehh, this should be macdrv_get_gpu_info_from_entry(gpu, parent);
> In any case, you didn't use `parent` in this call so looking it up was pointless. ;)  Again, this probably worked because of kIORegistryIterateParents.
> > -
> >    IOObjectRelease(parent);
> > - }
> > - IOObjectRelease(entry);
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_gpus
> > -
> > -
> > -
> > - Get a list of GPU currently in the system. The first GPU is primary.
>
> "list of GPUs" (plural)
> > -
> > - Call macdrv_free_gpus() when you are done using the data.
> > -
> > -
> > -
> > - Return -1 on failure with parameters unchanged.
> > - */
> >
> > +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> > +{
> > - struct macdrv_gpu* gpus;
> > - CGDirectDisplayID primary_display;
> > - id<MTLDevice> primary_device;
> > - int primary_index = 0;
> > - int gpu_count;
> > - int i;
> > - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> > -
> > - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
>
> You leak this array.  You need to either [devices release] at the end or [devices autorelease] here.

Thanks. I thought putting a [[NSAutoreleasePool alloc] init] should be enough to tell object-c to auto release it memory.
> > - gpu_count = devices.count ? devices.count : 1;
> > - gpus = calloc(gpu_count, sizeof(*gpus));
> > - if (!gpus)
> > -
> >    return -1;
> > -
> > - primary_display = CGMainDisplayID();
> > - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
>
> Similarly, you leak this device object.
>
> There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.

The primary GPU here is the one that drives the main display, which is where the dock is shown. The primary concept is the Windows one, where you choose a monitor to be primary.
> I'm not sure there's a good fix, though.  Metal doesn't let us list which devices could theoretically drive which displays. MTLDevice does have the lowPower property, but that doesn't quite tell us what we need to know.
>
> I've read somewhere that calling MTLCreateSystemDefaultDevice() will provoke a switch to the discrete GPU.  We could do that to determine what GPU will be primary if we're doing Metal or OpenGL rendering, but I don't know if our process will allow a switch back to the integrate GPU once we release that device or if it's marked as needing the discrete GPU for the rest of its life.
> > -
> > - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> > - if (!devices.count)
> > - {
> > -
> >    gpus[0].id = 0;
> > -
> >    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
>
> Hmm.  Why only report one GPU for this case?  Given that we can look up an IOPCIDevice entry from a display ID, we could enumerate all of the display IDs and accumulate all of the unique GPUs.

This also means we might not need Metal support at all. Is io_registry_entry_t can be used to comparison or it is just a temporary handle? I am not sure.
> > - }
> > - else
> > - {
> > -
> >    for (i = 0; i < devices.count; i++)
> > -
> >    {
> > -
> >        gpus[i].id = devices[i].registryID;
> > -
> >        macdrv_get_gpu_info_from_registry_id(&gpus[i], gpus[i].id);
> > -
> > -
> >        if (gpus[i].id == primary_device.registryID)
> > -
> >            primary_index = i;
> > -
> >    }
> > -
> > -
> >    /* Make sure the first GPU is primary */
> > -
> >    if (primary_index)
> > -
> >    {
> > -
> >        struct macdrv_gpu tmp;
> > -
> >        tmp = gpus[0];
> > -
> >        gpus[0] = gpus[primary_index];
> > -
> >        gpus[primary_index] = tmp;
> > -
> >    }
> > - }
> > -
> > - [pool release];
> > - *new_gpus = gpus;
> > - *count = gpu_count;
> > - return 0;
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_free_gpus
> > -
> > -
> > -
> > - Free a GPU list allocated from macdrv_get_gpus()
> > - */
> >
> > +void macdrv_free_gpus(struct macdrv_gpu* gpus)
> > +{
> > - free(gpus);
> >
> > +}

[Ken Thomases]

On Apr 23, 2019, at 2:22 AM, Zhiyi Zhang  wrote:
> On 4/23/19 11:56 AM, Ken Thomases wrote:
> > On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > > +/***********************************************************************
> > > -
> > > -
> > >          macdrv_get_gpu_info_from_display_id
> > > -
> > > -
> > > -
> > > - Get GPU information from a display id.
> > > -
> > > -
> > > -
> > > - This is a fallback for 32bit build since Metal doesn't support 32bit.
> > > -
> > > - Thus registryID can't be used.
> > > - */
> > >
> > > +void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)
> >
> > This should be static.
> > > +{
> > > - io_registry_entry_t entry = CGDisplayIOServicePort(display_id);
> >
> > On my Mac, this entry seems to be an IOFramebuffer entry a couple of levels below the GPU device entry.  You should probably traverse the parent chain to look for an IOPCIDevice entry.
> >
> > To the extent that this works, it's probably because of the kIORegistryIterateParents option in the property search.  I worry that an intervening entry with a same-named property might break things.
>
> Yes, it relies on kIORegistryIterateParents in the property search. The parent of the an framebuffer is the GPU.

Actually, for my Mac, the GPU is not the immediate parent of the framebuffer.  It's the grandparent.
> > > - macdrv_get_gpu_info_from_entry(gpu, entry);
> > >
> > > +}
> > > -
> > >
> > > +/***********************************************************************
> > > -
> > > -
> > >          macdrv_get_gpu_info_from_registry_id
> > > -
> > > -
> > > -
> > > - Get GPU information from a Metal device registry id.
> > > - */
> > >
> > > +void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)
> >
> > Should be static.
> > > +{
> > > - /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
> > > - io_registry_entry_t entry =
> > > -
> > >    IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
> > > - if (!entry)
> > > -
> > >    return;
> > > -
> > > - io_registry_entry_t parent;
> > > - if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)
> >
> > If you implement the search for an IOPCIDevice ancestor I recommended above, you should probably reuse it here, too. I'm not sure how reliable it is that the GPU is the immediate parent.
>
> Parent is already the GPU here. MoltenVK uses this method too. I am not sure GPU is the immediate parent too but I don't think Apple would change this often?

I'm not sure.  To some extent, this may be up to the drivers.
> Anyway, searching for an IOPCIDevice entry makes sense, I will use that.
> > > - {
> > > -
> > >    macdrv_get_gpu_info_from_entry(gpu, entry);
>
> Ehh, this should be macdrv_get_gpu_info_from_entry(gpu, parent);
> > In any case, you didn't use `parent` in this call so looking it up was pointless. ;)  Again, this probably worked because of kIORegistryIterateParents.
> > > -
> > >    IOObjectRelease(parent);
> > > - }
> > > - IOObjectRelease(entry);
> > >
> > > +}
> > > -
> > >
> > > +/***********************************************************************
> > > -
> > > -
> > >          macdrv_get_gpus
> > > -
> > > -
> > > -
> > > - Get a list of GPU currently in the system. The first GPU is primary.
> >
> > "list of GPUs" (plural)
> > > -
> > > - Call macdrv_free_gpus() when you are done using the data.
> > > -
> > > -
> > > -
> > > - Return -1 on failure with parameters unchanged.
> > > - */
> > >
> > > +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> > > +{
> > > - struct macdrv_gpu* gpus;
> > > - CGDirectDisplayID primary_display;
> > > - id<MTLDevice> primary_device;
> > > - int primary_index = 0;
> > > - int gpu_count;
> > > - int i;
> > > - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> > > -
> > > - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
> >
> > You leak this array.  You need to either [devices release] at the end or [devices autorelease] here.
>
> Thanks. I thought putting a [[NSAutoreleasePool alloc] init] should be enough to tell object-c to auto release it memory.
> > > - gpu_count = devices.count ? devices.count : 1;
> > > - gpus = calloc(gpu_count, sizeof(*gpus));
> > > - if (!gpus)
> > > -
> > >    return -1;
> > > -
> > > - primary_display = CGMainDisplayID();
> > > - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
> >
> > Similarly, you leak this device object.
> >
> > There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.
>
> The primary GPU here is the one that drives the main display, which is where the dock is shown. The primary concept is the Windows one, where you choose a monitor to be primary.

Right, but with automatic graphics switching there are two GPUs that could potentially drive the main display.

On thinking about this some more, the ideal approach would be to treat the two potential GPUs driving the main display as a single GPU (with the characteristics of the discrete GPU).  That is, we should hide the integrated GPU from Windows apps for the case where automatic graphics switching is supported.  The problem is detecting that case.  We'll probably have to do some tests on a Mac with automatic graphics switching.
> > I'm not sure there's a good fix, though.  Metal doesn't let us list which devices could theoretically drive which displays. MTLDevice does have the lowPower property, but that doesn't quite tell us what we need to know.
> >
> > I've read somewhere that calling MTLCreateSystemDefaultDevice() will provoke a switch to the discrete GPU.  We could do that to determine what GPU will be primary if we're doing Metal or OpenGL rendering, but I don't know if our process will allow a switch back to the integrate GPU once we release that device or if it's marked as needing the discrete GPU for the rest of its life.
> > > -
> > > - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> > > - if (!devices.count)
> > > - {
> > > -
> > >    gpus[0].id = 0;
> > > -
> > >    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
> >
> > Hmm.  Why only report one GPU for this case?  Given that we can look up an IOPCIDevice entry from a display ID, we could enumerate all of the display IDs and accumulate all of the unique GPUs.
>
> This also means we might not need Metal support at all.

Well, it's probably best to use Metal if it's available.  CGDisplayIOServicePort() is deprecated.
> Is io_registry_entry_t can be used to comparison or it is just a temporary handle? I am not sure.

I don't think they can be directly compared.  There's IOObjectIsEqualTo() to test if they refer to the same object.  (If that isn't sufficient for some reason, there's IORegistryEntryGetRegistryEntryID() to get IDs that can be compared.)

-Ken

[Henri Verbeet]

On Wed, 24 Apr 2019 at 19:52, Ken Thomases  wrote:
> Right, but with automatic graphics switching there are two GPUs that could potentially drive the main display.

This may be a slight tangent, but while early switchable GPUs had both
GPUs connected to the display through a multiplexer, on current setups
only the integrated/weak/low-power GPU is actually connected to the
display, and the other GPU is actually headless. The GPU connected to
the display is then responsible for getting the contents of the other
GPU onto the screen. I'm not sure how accurately that topology is
reflected by the relevant MacOS APIs.

[Chip Davis]

April 24, 2019 10:40 AM, "Henri Verbeet"  wrote:
> On Wed, 24 Apr 2019 at 19:52, Ken Thomases  wrote:
> > Right, but with automatic graphics switching there are two GPUs that could potentially drive the
> > main display.
>
> This may be a slight tangent, but while early switchable GPUs had both
> GPUs connected to the display through a multiplexer, on current setups
> only the integrated/weak/low-power GPU is actually connected to the
> display, and the other GPU is actually headless. The GPU connected to
> the display is then responsible for getting the contents of the other
> GPU onto the screen. I'm not sure how accurately that topology is
> reflected by the relevant MacOS APIs.

I'm not so sure that is in fact how it works on Mac. I remember a change to MoltenVK that was made to use MTLCreateSystemDefaultDevice() in order to force the system to switch to presenting with the discrete GPU, because that performed better when also using it for rendering.

Chip

[Henri Verbeet]

On Wed, 24 Apr 2019 at 20:17, Chip Davis  wrote:
> I'm not so sure that is in fact how it works on Mac. I remember a change to MoltenVK that was made to use MTLCreateSystemDefaultDevice() in order to force the system to switch to presenting with the discrete GPU, because that performed better when also using it for rendering.

You may be right about that, actually. I haven't paid all that much
attention to Apple for a while now, but I do remember they were one of
the few remaining vendors that still had a muxer at the time.

[Zhiyi Zhang]

On 4/24/19 11:22 PM, Ken Thomases wrote:
> On Apr 23, 2019, at 2:22 AM, Zhiyi Zhang  wrote:
> > On 4/23/19 11:56 AM, Ken Thomases wrote:
> > > On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > > > +/***********************************************************************
> > > > -
> > > > -
> > > >          macdrv_get_gpu_info_from_display_id
> > > > -
> > > > -
> > > > -
> > > > - Get GPU information from a display id.
> > > > -
> > > > -
> > > > -
> > > > - This is a fallback for 32bit build since Metal doesn't support 32bit.
> > > > -
> > > > - Thus registryID can't be used.
> > > > - */
> > > >
> > > > +void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)
> > >
> > > This should be static.
> > > > +{
> > > > - io_registry_entry_t entry = CGDisplayIOServicePort(display_id);
> > >
> > > On my Mac, this entry seems to be an IOFramebuffer entry a couple of levels below the GPU device entry.  You should probably traverse the parent chain to look for an IOPCIDevice entry.
> > >
> > > To the extent that this works, it's probably because of the kIORegistryIterateParents option in the property search.  I worry that an intervening entry with a same-named property might break things.
> >
> > Yes, it relies on kIORegistryIterateParents in the property search. The parent of the an framebuffer is the GPU.
>
> Actually, for my Mac, the GPU is not the immediate parent of the framebuffer.  It's the grandparent.

Well. Then it's not reliable then. We need to search upward to find the GPU.
> > > > - macdrv_get_gpu_info_from_entry(gpu, entry);
> > > >
> > > > +}
> > > > -
> > > >
> > > > +/***********************************************************************
> > > > -
> > > > -
> > > >          macdrv_get_gpu_info_from_registry_id
> > > > -
> > > > -
> > > > -
> > > > - Get GPU information from a Metal device registry id.
> > > > - */
> > > >
> > > > +void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)
> > >
> > > Should be static.
> > > > +{
> > > > - /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
> > > > - io_registry_entry_t entry =
> > > > -
> > > >    IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
> > > > - if (!entry)
> > > > -
> > > >    return;
> > > > -
> > > > - io_registry_entry_t parent;
> > > > - if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)
> > >
> > > If you implement the search for an IOPCIDevice ancestor I recommended above, you should probably reuse it here, too. I'm not sure how reliable it is that the GPU is the immediate parent.
> >
> > Parent is already the GPU here. MoltenVK uses this method too. I am not sure GPU is the immediate parent too but I don't think Apple would change this often?
>
> I'm not sure.  To some extent, this may be up to the drivers.
> > Anyway, searching for an IOPCIDevice entry makes sense, I will use that.
> > > > - {
> > > > -
> > > >    macdrv_get_gpu_info_from_entry(gpu, entry);
> >
> > Ehh, this should be macdrv_get_gpu_info_from_entry(gpu, parent);
> > > In any case, you didn't use `parent` in this call so looking it up was pointless. ;)  Again, this probably worked because of kIORegistryIterateParents.
> > > > -
> > > >    IOObjectRelease(parent);
> > > > - }
> > > > - IOObjectRelease(entry);
> > > >
> > > > +}
> > > > -
> > > >
> > > > +/***********************************************************************
> > > > -
> > > > -
> > > >          macdrv_get_gpus
> > > > -
> > > > -
> > > > -
> > > > - Get a list of GPU currently in the system. The first GPU is primary.
> > >
> > > "list of GPUs" (plural)
> > > > -
> > > > - Call macdrv_free_gpus() when you are done using the data.
> > > > -
> > > > -
> > > > -
> > > > - Return -1 on failure with parameters unchanged.
> > > > - */
> > > >
> > > > +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> > > > +{
> > > > - struct macdrv_gpu* gpus;
> > > > - CGDirectDisplayID primary_display;
> > > > - id<MTLDevice> primary_device;
> > > > - int primary_index = 0;
> > > > - int gpu_count;
> > > > - int i;
> > > > - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> > > > -
> > > > - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
> > >
> > > You leak this array.  You need to either [devices release] at the end or [devices autorelease] here.
> >
> > Thanks. I thought putting a [[NSAutoreleasePool alloc] init] should be enough to tell object-c to auto release it memory.
> > > > - gpu_count = devices.count ? devices.count : 1;
> > > > - gpus = calloc(gpu_count, sizeof(*gpus));
> > > > - if (!gpus)
> > > > -
> > > >    return -1;
> > > > -
> > > > - primary_display = CGMainDisplayID();
> > > > - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
> > >
> > > Similarly, you leak this device object.
> > >
> > > There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.
> >
> > The primary GPU here is the one that drives the main display, which is where the dock is shown. The primary concept is the Windows one, where you choose a monitor to be primary.
>
> Right, but with automatic graphics switching there are two GPUs that could potentially drive the main display.
>
> On thinking about this some more, the ideal approach would be to treat the two potential GPUs driving the main display as a single GPU (with the characteristics of the discrete GPU).  That is, we should hide the integrated GPU from Windows apps for the case where automatic graphics switching is supported.  The problem is detecting that case.  We'll probably have to do some tests on a Mac with automatic graphics switching.

Integrated GPU for presenting and discrete GPU for rendering?
In this case, we think it would make more sense to report the integrated GPU driving the main display. And report the discrete GPU not directly connected to a display.
The framebuffer is on the integrated GPU and discrete GPU may be turn off at any second to save power. On windows, the integrated GPU on laptops will always be in used
to act as a output sink.
> > > I'm not sure there's a good fix, though.  Metal doesn't let us list which devices could theoretically drive which displays. MTLDevice does have the lowPower property, but that doesn't quite tell us what we need to know.
> > >
> > > I've read somewhere that calling MTLCreateSystemDefaultDevice() will provoke a switch to the discrete GPU.  We could do that to determine what GPU will be primary if we're doing Metal or OpenGL rendering, but I don't know if our process will allow a switch back to the integrate GPU once we release that device or if it's marked as needing the discrete GPU for the rest of its life.
> > > > -
> > > > - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> > > > - if (!devices.count)
> > > > - {
> > > > -
> > > >    gpus[0].id = 0;
> > > > -
> > > >    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
> > >
> > > Hmm.  Why only report one GPU for this case?  Given that we can look up an IOPCIDevice entry from a display ID, we could enumerate all of the display IDs and accumulate all of the unique GPUs.
> >
> > This also means we might not need Metal support at all.
>
> Well, it's probably best to use Metal if it's available.  CGDisplayIOServicePort() is deprecated.

Emm, right, forgot about that part.
> > Is io_registry_entry_t can be used to comparison or it is just a temporary handle? I am not sure.
>
> I don't think they can be directly compared.  There's IOObjectIsEqualTo() to test if they refer to the same object.  (If that isn't sufficient for some reason, there's IORegistryEntryGetRegistryEntryID() to get IDs that can be compared.)
>
> -Ken

[Zhiyi Zhang]

On 4/24/19 11:42 PM, Zhiyi Zhang wrote:
> On 4/24/19 11:22 PM, Ken Thomases wrote:
> > On Apr 23, 2019, at 2:22 AM, Zhiyi Zhang  wrote:
> > > On 4/23/19 11:56 AM, Ken Thomases wrote:
> > > > On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > > > > +/***********************************************************************
> > > > > -
> > > > > -
> > > > >          macdrv_get_gpu_info_from_display_id
> > > > > -
> > > > > -
> > > > > -
> > > > > - Get GPU information from a display id.
> > > > > -
> > > > > -
> > > > > -
> > > > > - This is a fallback for 32bit build since Metal doesn't support 32bit.
> > > > > -
> > > > > - Thus registryID can't be used.
> > > > > - */
> > > > >
> > > > > +void macdrv_get_gpu_info_from_display_id(struct macdrv_gpu* gpu, CGDirectDisplayID display_id)
> > > >
> > > > This should be static.
> > > > > +{
> > > > > - io_registry_entry_t entry = CGDisplayIOServicePort(display_id);
> > > >
> > > > On my Mac, this entry seems to be an IOFramebuffer entry a couple of levels below the GPU device entry.  You should probably traverse the parent chain to look for an IOPCIDevice entry.
> > > >
> > > > To the extent that this works, it's probably because of the kIORegistryIterateParents option in the property search.  I worry that an intervening entry with a same-named property might break things.
> > >
> > > Yes, it relies on kIORegistryIterateParents in the property search. The parent of the an framebuffer is the GPU.
> >
> > Actually, for my Mac, the GPU is not the immediate parent of the framebuffer.  It's the grandparent.
>
> Well. Then it's not reliable then. We need to search upward to find the GPU.
> > > > > - macdrv_get_gpu_info_from_entry(gpu, entry);
> > > > >
> > > > > +}
> > > > > -
> > > > >
> > > > > +/***********************************************************************
> > > > > -
> > > > > -
> > > > >          macdrv_get_gpu_info_from_registry_id
> > > > > -
> > > > > -
> > > > > -
> > > > > - Get GPU information from a Metal device registry id.
> > > > > - */
> > > > >
> > > > > +void macdrv_get_gpu_info_from_registry_id(struct macdrv_gpu* gpu, uint64_t registry_id)
> > > >
> > > > Should be static.
> > > > > +{
> > > > > - /* entry is IOGraphicsAccelerator. Parent of the entry is the GPU */
> > > > > - io_registry_entry_t entry =
> > > > > -
> > > > >    IOServiceGetMatchingService(kIOMasterPortDefault, IORegistryEntryIDMatching(registry_id));
> > > > > - if (!entry)
> > > > > -
> > > > >    return;
> > > > > -
> > > > > - io_registry_entry_t parent;
> > > > > - if (IORegistryEntryGetParentEntry(entry, kIOServicePlane, &parent) == kIOReturnSuccess)
> > > >
> > > > If you implement the search for an IOPCIDevice ancestor I recommended above, you should probably reuse it here, too. I'm not sure how reliable it is that the GPU is the immediate parent.
> > >
> > > Parent is already the GPU here. MoltenVK uses this method too. I am not sure GPU is the immediate parent too but I don't think Apple would change this often?
> >
> > I'm not sure.  To some extent, this may be up to the drivers.
> > > Anyway, searching for an IOPCIDevice entry makes sense, I will use that.
> > > > > - {
> > > > > -
> > > > >    macdrv_get_gpu_info_from_entry(gpu, entry);
> > >
> > > Ehh, this should be macdrv_get_gpu_info_from_entry(gpu, parent);
> > > > In any case, you didn't use `parent` in this call so looking it up was pointless. ;)  Again, this probably worked because of kIORegistryIterateParents.
> > > > > -
> > > > >    IOObjectRelease(parent);
> > > > > - }
> > > > > - IOObjectRelease(entry);
> > > > >
> > > > > +}
> > > > > -
> > > > >
> > > > > +/***********************************************************************
> > > > > -
> > > > > -
> > > > >          macdrv_get_gpus
> > > > > -
> > > > > -
> > > > > -
> > > > > - Get a list of GPU currently in the system. The first GPU is primary.
> > > >
> > > > "list of GPUs" (plural)
> > > > > -
> > > > > - Call macdrv_free_gpus() when you are done using the data.
> > > > > -
> > > > > -
> > > > > -
> > > > > - Return -1 on failure with parameters unchanged.
> > > > > - */
> > > > >
> > > > > +int macdrv_get_gpus(struct macdrv_gpu** new_gpus, int* count)
> > > > > +{
> > > > > - struct macdrv_gpu* gpus;
> > > > > - CGDirectDisplayID primary_display;
> > > > > - id<MTLDevice> primary_device;
> > > > > - int primary_index = 0;
> > > > > - int gpu_count;
> > > > > - int i;
> > > > > - NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> > > > > -
> > > > > - NSArray<id<MTLDevice>>* devices = MTLCopyAllDevices();
> > > >
> > > > You leak this array.  You need to either [devices release] at the end or [devices autorelease] here.
> > >
> > > Thanks. I thought putting a [[NSAutoreleasePool alloc] init] should be enough to tell object-c to auto release it memory.
> > > > > - gpu_count = devices.count ? devices.count : 1;
> > > > > - gpus = calloc(gpu_count, sizeof(*gpus));
> > > > > - if (!gpus)
> > > > > -
> > > > >    return -1;
> > > > > -
> > > > > - primary_display = CGMainDisplayID();
> > > > > - primary_device = CGDirectDisplayCopyCurrentMetalDevice(primary_display);
> > > >
> > > > Similarly, you leak this device object.
> > > >
> > > > There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.
> > >
> > > The primary GPU here is the one that drives the main display, which is where the dock is shown. The primary concept is the Windows one, where you choose a monitor to be primary.
> >
> > Right, but with automatic graphics switching there are two GPUs that could potentially drive the main display.
> >
> > On thinking about this some more, the ideal approach would be to treat the two potential GPUs driving the main display as a single GPU (with the characteristics of the discrete GPU).  That is, we should hide the integrated GPU from Windows apps for the case where automatic graphics switching is supported.  The problem is detecting that case.  We'll probably have to do some tests on a Mac with automatic graphics switching.
>
> Integrated GPU for presenting and discrete GPU for rendering?
> In this case, we think it would make more sense to report the integrated GPU driving the main display. And report the discrete GPU not directly connected to a display.
> The framebuffer is on the integrated GPU and discrete GPU may be turn off at any second to save power. On windows, the integrated GPU on laptops will always be in used
> to act as a output sink.

Automatic graphics switching shouldn't cause a problem. Apple said[1] one display is connected to one GPU at any given time. So we can report what ever GPU driving the main display
and a device change event should re-init all of this if switching happens.

And it seems to have a multiplexer.

[1]: https://developer.apple.com/documentation/metal/choosing_gpus_on_mac/about_m...
> > > > I'm not sure there's a good fix, though.  Metal doesn't let us list which devices could theoretically drive which displays. MTLDevice does have the lowPower property, but that doesn't quite tell us what we need to know.
> > > >
> > > > I've read somewhere that calling MTLCreateSystemDefaultDevice() will provoke a switch to the discrete GPU.  We could do that to determine what GPU will be primary if we're doing Metal or OpenGL rendering, but I don't know if our process will allow a switch back to the integrate GPU once we release that device or if it's marked as needing the discrete GPU for the rest of its life.
> > > > > -
> > > > > - /* 32bit build. Metal is unsupported. Report only one GPU. Use the primary display to get GPU info */
> > > > > - if (!devices.count)
> > > > > - {
> > > > > -
> > > > >    gpus[0].id = 0;
> > > > > -
> > > > >    macdrv_get_gpu_info_from_display_id(&gpus[0], primary_display);
> > > >
> > > > Hmm.  Why only report one GPU for this case?  Given that we can look up an IOPCIDevice entry from a display ID, we could enumerate all of the display IDs and accumulate all of the unique GPUs.
> > >
> > > This also means we might not need Metal support at all.
> >
> > Well, it's probably best to use Metal if it's available.  CGDisplayIOServicePort() is deprecated.
>
> Emm, right, forgot about that part.
> > > Is io_registry_entry_t can be used to comparison or it is just a temporary handle? I am not sure.
> >
> > I don't think they can be directly compared.  There's IOObjectIsEqualTo() to test if they refer to the same object.  (If that isn't sufficient for some reason, there's IORegistryEntryGetRegistryEntryID() to get IDs that can be compared.)
> >
> > -Ken

[Ken Thomases]

On Apr 24, 2019, at 11:28 AM, Zhiyi Zhang  wrote:
> Automatic graphics switching shouldn't cause a problem. Apple said[1] one display is connected to one GPU at any given time. So we can report what ever GPU driving the main display
> and a device change event should re-init all of this if switching happens.

The problem is that if we report the integrated GPU as primary then (if someday wined3d supports selecting a specific adapter) the Windows app and wined3d may select the integrated GPU.  Then automatic graphics switching will never engage, because nothing has requested the use of the discrete GPU, and no re-init will occur.  And performance will suck (more).

On the other hand, apps other than Wine could cause the OS to switch to the discrete GPU at any time.  If Wine is using the integrated GPU and it or the Windows app are not prepared to handle the switch, then we continue to use the integrated GPU for rendering and the OS uses the discrete GPU for presenting.

Unless we're confident that all Windows apps will handle switching properly, then I think we should just always force using the discrete GPU for GL or Metal (as is currently the case).  And, if we're going to do that, then we should only report the discrete GPU and hide the integrated GPU.

-Ken

[Ken Thomases]

On Apr 24, 2019, at 10:42 AM, Zhiyi Zhang  wrote:
> On 4/24/19 11:22 PM, Ken Thomases wrote:
> > On Apr 23, 2019, at 2:22 AM, Zhiyi Zhang  wrote:
> > > On 4/23/19 11:56 AM, Ken Thomases wrote:
> > > > There's a question as to what should be considered the primary GPU.  On Macs with automatic graphics switching, the integrated GPU might be current for the main display at initialization time.  We'd then consider that primary.  If we ever support apps specifying which GPU they want to use for graphics, we probably don't want to tell them that the integrated GPU is primary.
> > >
> > > The primary GPU here is the one that drives the main display, which is where the dock is shown. The primary concept is the Windows one, where you choose a monitor to be primary.
> >
> > Right, but with automatic graphics switching there are two GPUs that could potentially drive the main display.
> >
> > On thinking about this some more, the ideal approach would be to treat the two potential GPUs driving the main display as a single GPU (with the characteristics of the discrete GPU).  That is, we should hide the integrated GPU from Windows apps for the case where automatic graphics switching is supported.  The problem is detecting that case.  We'll probably have to do some tests on a Mac with automatic graphics switching.
>
> Integrated GPU for presenting and discrete GPU for rendering?

Well, I'm not sure how it works under the hood (see Henri's and Chip's comments), but from the API perspective the discrete GPU is used for both rendering and presenting once the switch is made.  That's why I suggest reporting the discrete GPU as the one driving the main display.
> In this case, we think it would make more sense to report the integrated GPU driving the main display. And report the discrete GPU not directly connected to a display.
> The framebuffer is on the integrated GPU and discrete GPU may be turn off at any second to save power.

No, when the discrete GPU is being used, the framebuffer is on the discrete GPU, at least from the perspective of IOKit.  And the discrete GPU won't be turned off if an app is using it and that app doesn't tell the system that it's prepared to handle a switch back to the integrated GPU.  I don't know if Windows apps are expected to handle that well — maybe Direct3D apps, but not OpenGL apps? — and we currently don't tell the OS that we're capable of handling that.  So, as long as a Wine process has an OpenGL context or Metal device, the discrete GPU will be active.

Again, this is why I think we should only report the discrete GPU of an auto-switching pair.

-Ken