[PATCH 3/7] winemac.drv: Add macdrv_get_adapters() helper.

[Zhiyi Zhang]

Signed-off-by: Zhiyi Zhang 
---
 dlls/winemac.drv/cocoa_display.m | 72 ++++++++++++++++++++++++++++++++++++++++
 dlls/winemac.drv/macdrv_cocoa.h  | 15 +++++++++
 2 files changed, 87 insertions(+)

diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
index 569cd32352..d95cda59c9 100644
--- a/dlls/winemac.drv/cocoa_display.m
+++ b/dlls/winemac.drv/cocoa_display.m
@@ -271,3 +271,75 @@ void macdrv_free_gpus(struct macdrv_gpu* gpus)
 {
     free(gpus);
 }
+
+/***********************************************************************
+ *              macdrv_get_adapters
+ *
+ * Get a list of adapters under gpu_id. The first adapter is primary if GPU is primary.
+ * Call macdrv_free_adapters() when you are done using the data.
+ *
+ * Return -1 on failure with parameters unchanged.
+ */
+int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** new_adapters, int* count)
+{
+    struct macdrv_adapter* adapters;
+    CGDirectDisplayID display_ids[16];
+    uint32_t display_id_count;
+    uint32_t i;
+    int primary_index = 0;
+    int adapter_count = 0;
+
+    if (CGGetOnlineDisplayList(sizeof(display_ids) / sizeof(display_ids[0]), display_ids, &display_id_count)
+        != kCGErrorSuccess)
+        return -1;
+
+    /* Actual adapter count may be less */
+    adapters = calloc(display_id_count, sizeof(*adapters));
+    if (!adapters)
+        return -1;
+
+    for (i = 0; i < display_id_count; i++)
+    {
+        /* Mirrored displays are under the same adapter with primary display, so they doesn't increase adapter count */
+        if (CGDisplayMirrorsDisplay(display_ids[i]) != kCGNullDirectDisplay)
+            continue;
+
+        id<MTLDevice> device = CGDirectDisplayCopyCurrentMetalDevice(display_ids[i]);
+        if (!device || device.registryID == gpu_id)
+        {
+            adapters[adapter_count].id = display_ids[i];
+            if (CGDisplayIsMain(display_ids[i]))
+            {
+                adapters[adapter_count].state_flags |= DISPLAY_DEVICE_PRIMARY_DEVICE;
+                primary_index = adapter_count;
+            }
+            if (CGDisplayIsActive(display_ids[i]) || CGDisplayIsInMirrorSet(display_ids[i]))
+                adapters[adapter_count].state_flags |= DISPLAY_DEVICE_ATTACHED_TO_DESKTOP;
+
+            adapter_count++;
+        }
+    }
+
+    /* Make sure the first adapter is primary if the GPU is primary */
+    if (primary_index)
+    {
+        struct macdrv_adapter tmp;
+        tmp = adapters[0];
+        adapters[0] = adapters[primary_index];
+        adapters[primary_index] = tmp;
+    }
+
+    *new_adapters = adapters;
+    *count = adapter_count;
+    return 0;
+}
+
+/***********************************************************************
+ *              macdrv_free_adapters
+ *
+ * Free an adapter list allocated from macdrv_get_adapters()
+ */
+void macdrv_free_adapters(struct macdrv_adapter* adapters)
+{
+    free(adapters);
+}
diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h
index a6d8ec91d4..a57b94c125 100644
--- a/dlls/winemac.drv/macdrv_cocoa.h
+++ b/dlls/winemac.drv/macdrv_cocoa.h
@@ -258,6 +258,10 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)

/* display */

+/* Used DISPLAY_DEVICE.StateFlags for adapters */
+#define DISPLAY_DEVICE_ATTACHED_TO_DESKTOP      0x00000001
+#define DISPLAY_DEVICE_PRIMARY_DEVICE           0x00000004
+
 /* Represent a physical GPU in the PCI slots */
 struct macdrv_gpu
 {
@@ -272,12 +276,23 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)
     uint32_t revision_id;
 };

+/* Represent an adapter in EnumDisplayDevices context */
+struct macdrv_adapter
+{
+    /* ID to uniquely identify an adapter. Currently it's a CGDirectDisplayID */
+    uint32_t id;
+    /* as StateFlags in DISPLAY_DEVICE struct */
+    uint32_t state_flags;
+};
+
 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;
+extern int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** adapters, int* count) DECLSPEC_HIDDEN;
+extern void macdrv_free_adapters(struct macdrv_adapter* adapters) DECLSPEC_HIDDEN;

/* event */

[Ken Thomases]

On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> Signed-off-by: Zhiyi Zhang
>
> dlls/winemac.drv/cocoa_display.m | 72 ++++++++++++++++++++++++++++++++++++++++
> dlls/winemac.drv/macdrv_cocoa.h  | 15 +++++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> index 569cd32352..d95cda59c9 100644
> --- a/dlls/winemac.drv/cocoa_display.m
> +++ b/dlls/winemac.drv/cocoa_display.m
> @@ -271,3 +271,75 @@ void macdrv_free_gpus(struct macdrv_gpu* gpus)
> {
>     free(gpus);
> }
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_get_adapters
> -
> -
> -
> - Get a list of adapters under gpu_id. The first adapter is primary if GPU is primary.
> -
> - Call macdrv_free_adapters() when you are done using the data.
> -
> -
> -
> - Return -1 on failure with parameters unchanged.
> - */
>
> +int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** new_adapters, int* count)
> +{
> - struct macdrv_adapter* adapters;
> - CGDirectDisplayID display_ids[16];
> - uint32_t display_id_count;
> - uint32_t i;
> - int primary_index = 0;
> - int adapter_count = 0;
> -
> - if (CGGetOnlineDisplayList(sizeof(display_ids) / sizeof(display_ids[0]), display_ids, &display_id_count)
> -
>    != kCGErrorSuccess)
> -
>    return -1;
> -
> - /* Actual adapter count may be less */
> - adapters = calloc(display_id_count, sizeof(*adapters));
> - if (!adapters)
> -
>    return -1;
> -
> - for (i = 0; i < display_id_count; i++)
> - {
> -
>    /* Mirrored displays are under the same adapter with primary display, so they doesn't increase adapter count */
> -
>    if (CGDisplayMirrorsDisplay(display_ids[i]) != kCGNullDirectDisplay)
> -
>        continue;
> -
> -
>    id<MTLDevice> device = CGDirectDisplayCopyCurrentMetalDevice(display_ids[i]);

You leak this device.
> -
>    if (!device || device.registryID == gpu_id)
> -
>    {
> -
>        adapters[adapter_count].id = display_ids[i];
> -
>        if (CGDisplayIsMain(display_ids[i]))
> -
>        {
> -
>            adapters[adapter_count].state_flags |= DISPLAY_DEVICE_PRIMARY_DEVICE;
> -
>            primary_index = adapter_count;
> -
>        }
> -
>        if (CGDisplayIsActive(display_ids[i]) || CGDisplayIsInMirrorSet(display_ids[i]))

Why are you counting an inactive display that's in a mirror set here?  You skipped display IDs which are mirroring another display, above.  So, when can CGDisplayIsInMirrorSet() return true here?
> -
>            adapters[adapter_count].state_flags |= DISPLAY_DEVICE_ATTACHED_TO_DESKTOP;
> -
> -
>        adapter_count++;
> -
>    }
> - }
> -
> - /* Make sure the first adapter is primary if the GPU is primary */
> - if (primary_index)
> - {
> -
>    struct macdrv_adapter tmp;
> -
>    tmp = adapters[0];
> -
>    adapters[0] = adapters[primary_index];
> -
>    adapters[primary_index] = tmp;
> - }
> -
> - *new_adapters = adapters;
> - *count = adapter_count;
> - return 0;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>          macdrv_free_adapters
> -
> -
> -
> - Free an adapter list allocated from macdrv_get_adapters()
> - */
>
> +void macdrv_free_adapters(struct macdrv_adapter* adapters)
> +{
> - free(adapters);
>
> +}
> diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h
> index a6d8ec91d4..a57b94c125 100644
> --- a/dlls/winemac.drv/macdrv_cocoa.h
> +++ b/dlls/winemac.drv/macdrv_cocoa.h
> @@ -258,6 +258,10 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)
>
> /* display */
>
> +/* Used DISPLAY_DEVICE.StateFlags for adapters */
> +#define DISPLAY_DEVICE_ATTACHED_TO_DESKTOP      0x00000001
> +#define DISPLAY_DEVICE_PRIMARY_DEVICE           0x00000004
> -
>
> /* Represent a physical GPU in the PCI slots */
> struct macdrv_gpu
> {
> @@ -272,12 +276,23 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)
>     uint32_t revision_id;
> };
>
> +/* Represent an adapter in EnumDisplayDevices context */
> +struct macdrv_adapter
> +{
> - /* ID to uniquely identify an adapter. Currently it's a CGDirectDisplayID */
> - uint32_t id;

There's an annoying fact about display IDs: when automatic graphics switching switches, all of the displays driven by the GPUs involved change.  So, display IDs don't actually identify a display, they identify framebuffers (or display-GPU combinations).

Among other things, that means that comparing display IDs is not always reliable.  Also, the display IDs output from macdrv_get_displays() may be different from those seen by the various Core Graphics functions because, I think, NSScreen uses a canonicalized display ID that _doesn't_ change with automatic graphics switching.  (That may only be true of macOS 10.10+ which added new guarantees about NSScreen's semantics.)

There are functions CGDisplayCreateUUIDFromDisplayID() and CGDisplayGetDisplayIDFromUUID(), in the ColorSync framework of all places, that could be used to properly compare displays for equality.  At least, so claims an answer on Stack Overflow.  Those functions are pretty much undocumented.

It maybe doesn't matter, since automatic graphics switch will provoke a DISPLAYS_CHANGED Mac driver event, which (in a later patch) will re-init all of this.
> - /* as StateFlags in DISPLAY_DEVICE struct */
> - uint32_t state_flags;
>
> +};
> -
>
> 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;
> +extern int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** adapters, int* count) DECLSPEC_HIDDEN;
> +extern void macdrv_free_adapters(struct macdrv_adapter* adapters) DECLSPEC_HIDDEN;

[Zhiyi Zhang]

On 4/23/19 12:22 PM, Ken Thomases wrote:
> On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > Signed-off-by: Zhiyi Zhang
> >
> > dlls/winemac.drv/cocoa_display.m | 72 ++++++++++++++++++++++++++++++++++++++++
> > dlls/winemac.drv/macdrv_cocoa.h  | 15 +++++++++
> > 2 files changed, 87 insertions(+)
> >
> > diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> > index 569cd32352..d95cda59c9 100644
> > --- a/dlls/winemac.drv/cocoa_display.m
> > +++ b/dlls/winemac.drv/cocoa_display.m
> > @@ -271,3 +271,75 @@ void macdrv_free_gpus(struct macdrv_gpu* gpus)
> > {
> >     free(gpus);
> > }
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_get_adapters
> > -
> > -
> > -
> > - Get a list of adapters under gpu_id. The first adapter is primary if GPU is primary.
> > -
> > - Call macdrv_free_adapters() when you are done using the data.
> > -
> > -
> > -
> > - Return -1 on failure with parameters unchanged.
> > - */
> >
> > +int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** new_adapters, int* count)
> > +{
> > - struct macdrv_adapter* adapters;
> > - CGDirectDisplayID display_ids[16];
> > - uint32_t display_id_count;
> > - uint32_t i;
> > - int primary_index = 0;
> > - int adapter_count = 0;
> > -
> > - if (CGGetOnlineDisplayList(sizeof(display_ids) / sizeof(display_ids[0]), display_ids, &display_id_count)
> > -
> >    != kCGErrorSuccess)
> > -
> >    return -1;
> > -
> > - /* Actual adapter count may be less */
> > - adapters = calloc(display_id_count, sizeof(*adapters));
> > - if (!adapters)
> > -
> >    return -1;
> > -
> > - for (i = 0; i < display_id_count; i++)
> > - {
> > -
> >    /* Mirrored displays are under the same adapter with primary display, so they doesn't increase adapter count */
> > -
> >    if (CGDisplayMirrorsDisplay(display_ids[i]) != kCGNullDirectDisplay)
> > -
> >        continue;
> > -
> > -
> >    id<MTLDevice> device = CGDirectDisplayCopyCurrentMetalDevice(display_ids[i]);
>
> You leak this device.

I need to put in

NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
[pool release];

right?
> > -
> >    if (!device || device.registryID == gpu_id)
> > -
> >    {
> > -
> >        adapters[adapter_count].id = display_ids[i];
> > -
> >        if (CGDisplayIsMain(display_ids[i]))
> > -
> >        {
> > -
> >            adapters[adapter_count].state_flags |= DISPLAY_DEVICE_PRIMARY_DEVICE;
> > -
> >            primary_index = adapter_count;
> > -
> >        }
> > -
> >        if (CGDisplayIsActive(display_ids[i]) || CGDisplayIsInMirrorSet(display_ids[i]))
>
> Why are you counting an inactive display that's in a mirror set here?  You skipped display IDs which are mirroring another display, above.  So, when can CGDisplayIsInMirrorSet() return true here?

Right, the CGDisplayIsInMirrorSet(display_ids[i]) check is not needed.
> > -
> >            adapters[adapter_count].state_flags |= DISPLAY_DEVICE_ATTACHED_TO_DESKTOP;
> > -
> > -
> >        adapter_count++;
> > -
> >    }
> > - }
> > -
> > - /* Make sure the first adapter is primary if the GPU is primary */
> > - if (primary_index)
> > - {
> > -
> >    struct macdrv_adapter tmp;
> > -
> >    tmp = adapters[0];
> > -
> >    adapters[0] = adapters[primary_index];
> > -
> >    adapters[primary_index] = tmp;
> > - }
> > -
> > - *new_adapters = adapters;
> > - *count = adapter_count;
> > - return 0;
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >          macdrv_free_adapters
> > -
> > -
> > -
> > - Free an adapter list allocated from macdrv_get_adapters()
> > - */
> >
> > +void macdrv_free_adapters(struct macdrv_adapter* adapters)
> > +{
> > - free(adapters);
> >
> > +}
> > diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h
> > index a6d8ec91d4..a57b94c125 100644
> > --- a/dlls/winemac.drv/macdrv_cocoa.h
> > +++ b/dlls/winemac.drv/macdrv_cocoa.h
> > @@ -258,6 +258,10 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)
> >
> > /* display */
> >
> > +/* Used DISPLAY_DEVICE.StateFlags for adapters */
> > +#define DISPLAY_DEVICE_ATTACHED_TO_DESKTOP      0x00000001
> > +#define DISPLAY_DEVICE_PRIMARY_DEVICE           0x00000004
> > -
> >
> > /* Represent a physical GPU in the PCI slots */
> > struct macdrv_gpu
> > {
> > @@ -272,12 +276,23 @@ static inline CGPoint cgpoint_win_from_mac(CGPoint point)
> >     uint32_t revision_id;
> > };
> >
> > +/* Represent an adapter in EnumDisplayDevices context */
> > +struct macdrv_adapter
> > +{
> > - /* ID to uniquely identify an adapter. Currently it's a CGDirectDisplayID */
> > - uint32_t id;
>
> There's an annoying fact about display IDs: when automatic graphics switching switches, all of the displays driven by the GPUs involved change.  So, display IDs don't actually identify a display, they identify framebuffers (or display-GPU combinations).
>
> Among other things, that means that comparing display IDs is not always reliable.  Also, the display IDs output from macdrv_get_displays() may be different from those seen by the various Core Graphics functions because, I think, NSScreen uses a canonicalized display ID that _doesn't_ change with automatic graphics switching.  (That may only be true of macOS 10.10+ which added new guarantees about NSScreen's semantics.)
>
> There are functions CGDisplayCreateUUIDFromDisplayID() and CGDisplayGetDisplayIDFromUUID(), in the ColorSync framework of all places, that could be used to properly compare displays for equality.  At least, so claims an answer on Stack Overflow.  Those functions are pretty much undocumented.
>
> It maybe doesn't matter, since automatic graphics switch will provoke a DISPLAYS_CHANGED Mac driver event, which (in a later patch) will re-init all of this.

Thanks for the explanation. the re-init should be able to avoid this.
> > - /* as StateFlags in DISPLAY_DEVICE struct */
> > - uint32_t state_flags;
> >
> > +};
> > -
> >
> > 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;
> > +extern int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** adapters, int* count) DECLSPEC_HIDDEN;
> > +extern void macdrv_free_adapters(struct macdrv_adapter* adapters) DECLSPEC_HIDDEN;

[Ken Thomases]

On Apr 23, 2019, at 1:49 AM, Zhiyi Zhang  wrote:
> On 4/23/19 12:22 PM, Ken Thomases wrote:
> > On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang  wrote:
> > > Signed-off-by: Zhiyi Zhang
> > >
> > > dlls/winemac.drv/cocoa_display.m | 72 ++++++++++++++++++++++++++++++++++++++++
> > > dlls/winemac.drv/macdrv_cocoa.h  | 15 +++++++++
> > > 2 files changed, 87 insertions(+)
> > >
> > > diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
> > > index 569cd32352..d95cda59c9 100644
> > > --- a/dlls/winemac.drv/cocoa_display.m
> > > +++ b/dlls/winemac.drv/cocoa_display.m
> > > @@ -271,3 +271,75 @@ void macdrv_free_gpus(struct macdrv_gpu* gpus)
> > > {
> > >    free(gpus);
> > > }
> > > -
> > >
> > > +/***********************************************************************
> > > -
> > > -
> > >          macdrv_get_adapters
> > > -
> > > -
> > > -
> > > - Get a list of adapters under gpu_id. The first adapter is primary if GPU is primary.
> > > -
> > > - Call macdrv_free_adapters() when you are done using the data.
> > > -
> > > -
> > > -
> > > - Return -1 on failure with parameters unchanged.
> > > - */
> > >
> > > +int macdrv_get_adapters(uint64_t gpu_id, struct macdrv_adapter** new_adapters, int* count)
> > > +{
> > > - struct macdrv_adapter* adapters;
> > > - CGDirectDisplayID display_ids[16];
> > > - uint32_t display_id_count;
> > > - uint32_t i;
> > > - int primary_index = 0;
> > > - int adapter_count = 0;
> > > -
> > > - if (CGGetOnlineDisplayList(sizeof(display_ids) / sizeof(display_ids[0]), display_ids, &display_id_count)
> > > -
> > >    != kCGErrorSuccess)
> > > -
> > >    return -1;
> > > -
> > > - /* Actual adapter count may be less */
> > > - adapters = calloc(display_id_count, sizeof(*adapters));
> > > - if (!adapters)
> > > -
> > >    return -1;
> > > -
> > > - for (i = 0; i < display_id_count; i++)
> > > - {
> > > -
> > >    /* Mirrored displays are under the same adapter with primary display, so they doesn't increase adapter count */
> > > -
> > >    if (CGDisplayMirrorsDisplay(display_ids[i]) != kCGNullDirectDisplay)
> > > -
> > >        continue;
> > > -
> > > -
> > >    id<MTLDevice> device = CGDirectDisplayCopyCurrentMetalDevice(display_ids[i]);
> >
> > You leak this device.
>
> I need to put in
>
> NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> [pool release];
>
> right?

That's not enough.  As mentioned in a reply to another patch, you need to do [device release] or [device autorelease].  For the latter, the autorelease pool is necessary.  In fact, the autorelease pool is a good idea in any case, because that function (or releasing the device) might autorelease things, too.

"Autorelease" is a bit of a misnomer.  It doesn't perform automatic memory management.  It might be better to think of it as "deferred release".  Cocoa maintains a stack of autorelease pools per thread.  When you create an autorelease pool, it's pushed to the top of the stack.  When you autorelease an object, it is added to a list maintained by the pool at the top of the stack.  When the pool is released, it's popped from the stack and it releases all of the objects in its list.

In some contexts, there is no autorelease pool for the thread.  In some other contexts, there may be a pool, but it won't be released in any reasonable timeframe (perhaps not until thread exit).  That's why we need to create our own pools in functions that may be called from non-Cocoa code.

-Ken