From: Ken Thomases <ken@codeweavers.com>
Subject: Re: [PATCH 3/7] winemac.drv: Add macdrv_get_adapters() helper.
Message-Id: <A9A5F50B-16CF-4DA1-928A-D0E7156E4E74@codeweavers.com>
Date: Wed, 24 Apr 2019 09:38:41 -0500
In-Reply-To: <0a7bb625-f683-42a3-6e3f-649dca93f803@codeweavers.com>
References: <f308abdd-2be8-951c-c005-d447cc533552@codeweavers.com> <87C0548E-20E9-4CF8-8CF0-5921E25D0363@codeweavers.com> <0a7bb625-f683-42a3-6e3f-649dca93f803@codeweavers.com>

On Apr 23, 2019, at 1:49 AM, Zhiyi Zhang <zzhang@codeweavers.com> wrote:
> 
> On 4/23/19 12:22 PM, Ken Thomases wrote:
>> On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang <zzhang@codeweavers.com> wrote:
>>> Signed-off-by: Zhiyi Zhang <zzhang@codeweavers.com>
>>> ---
>>> 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