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

[Zhiyi Zhang]

On 4/24/19 11:22 PM, Ken Thomases wrote:
> On Apr 23, 2019, at 2:22 AM, Zhiyi Zhang <zzhang at codeweavers.com> wrote:
>> On 4/23/19 11:56 AM, Ken Thomases wrote:
>>> On Apr 22, 2019, at 7:13 AM, Zhiyi Zhang <zzhang at codeweavers.com> 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
>