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

[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;