[PATCH 01/10] dxgi/tests: Add IDXGIOutput ownership tests.

[Zhiyi Zhang]

On 5/17/19 4:28 AM, Henri Verbeet wrote:
> On Wed, 15 May 2019 at 18:13, Zhiyi Zhang <zzhang at codeweavers.com> wrote:
>> +    if (!pD3DKMTCheckVidPnExclusiveOwnership || pD3DKMTCheckVidPnExclusiveOwnership(NULL) == STATUS_PROCEDURE_NOT_FOUND
>> +        || !pD3DKMTCloseAdapter || !pD3DKMTOpenAdapterFromGdiDisplayName)
> Formatting. (Double indent for line continuations.)
Eh, I keep forgetting this when writing d3d code. Are you using some kind of coding style format file?
For example, clang-format? If it's, could you share it so I won't forget about it next time?
>
>> +    lstrcpyW(open_adapter_gdi_desc.DeviceName, display1W);
>> +    status = pD3DKMTOpenAdapterFromGdiDisplayName(&open_adapter_gdi_desc);
>> +    ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status);
>> +
> Wouldn't it make more sense to use D3DKMTOpenAdapterFromLuid(), so
> that we can use the adapter corresponding to the device?
The problem with D3DKMTOpenAdapterFromLuid is that currently there is no way to uniquely
correlate dxgi adapter, gdi adapter, opengl adapter and vulkan adapter, as previously discussed
when trying to support LUID. But I think it could be partially implemented for dxgi adapter <-> gdi adapter,
seeing that dxgi currently use EnumDisplayDevice to enumerate adapters.

Or may be I could use D3DKMTOpenAdapterFromGdiDisplayName only when testing on wine?
>
>> +    /* Swapchain in fullscreen mode */
>> +    hr = IDXGISwapChain_SetFullscreenState(swapchain, TRUE, NULL);
>> +    /* DXGI_ERROR_NOT_CURRENTLY_AVAILABLE on some machines. DXGI_ERROR_UNSUPPORTED on Win 7 testbot. */
>> +    if (hr == DXGI_ERROR_NOT_CURRENTLY_AVAILABLE || broken(hr == DXGI_ERROR_UNSUPPORTED))
>> +    {
>> +        skip("Failed to change fullscreen state.\n");
>> +        goto done;
>> +    }
>> +    todo_wine_if(is_d3d12) ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
>> +    Sleep(timeout);
> Is that Sleep() really required?
Yes. I think I have it commented somewhere in the patch series. I reordered then so it must be in later patches.

The Sleep() is used so that the tests can get consistent results. Without the Sleep(), the tests suffers from timing issues,
getting different results sometimes. For example, on Windows, it takes a few seconds for a fullscreen window to actually
exited the fullscreen mode. For other places, I expect some IPC involved, so the state is not updated in real time.
I tried to remove as many Sleep() calls as I can. So yes, any Sleep() calls you are seeing are necessary.

>
>> +    /* Calling IDXGIOutput_ReleaseOwnership makes it unoccluded. So we can be confident about IDXGIOutput_TakeOwnership
>> +     * was called in IDXGISwapChain_SetFullscreenState */
>> +    if (output) IDXGIOutput_ReleaseOwnership(output);
>> +    Sleep(timeout);
> Likewise.
>
>> +    status = pD3DKMTCheckVidPnExclusiveOwnership(&check_ownership_desc);
>> +    ok(status == STATUS_SUCCESS || status == STATUS_GRAPHICS_PRESENT_UNOCCLUDED, "Got unexpected status %#x.\n",
>> +       status);
>> +
>> +    if (output) hr = IDXGIOutput_TakeOwnership(output, device, FALSE);
>> +    todo_wine ok(hr == (is_d3d12 ? E_NOINTERFACE : DXGI_ERROR_NOT_CURRENTLY_AVAILABLE), "Got unexpected hr %#x.\n", hr);
>> +    ok(status == STATUS_SUCCESS || status == STATUS_GRAPHICS_PRESENT_UNOCCLUDED, "Got unexpected status %#x.\n",
>> +       status);
> This ok() seems redundant.
>
>> +    /* Swapchain in windowed mode */
>> +    hr = IDXGISwapChain_SetFullscreenState(swapchain, FALSE, NULL);
>> +    todo_wine_if(is_d3d12) ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
>> +    Sleep(timeout);
> And another.