[PATCH 0/4] MR766: tests: Add CreateShaderCacheSession pointer clearing tests.
Native's behavior is inconsistent. It sets the pointer to NULL when it fails to open an existing cache due to a version mismatch, but leaves the pointer untouched when the description fails validation. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766
From: Stefan Dösinger <stefan(a)codeweavers.com> Native's behavior is inconsistent. It sets the pointer to NULL when it fails to open an existing cache due to a version mismatch, but leaves the pointer untouched when the description fails validation. --- tests/d3d12.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/d3d12.c b/tests/d3d12.c index b8d20a581..fb151cad3 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -38299,6 +38299,7 @@ static void test_shader_cache(void) base_refcount = get_refcount(device); /* The description needs to be non-NULL and have at least the identifier set. */ + unk = (IUnknown *)0xdeadbeef; hr = ID3D12Device9_CreateShaderCacheSession(device, NULL, &IID_IUnknown, (void **)&unk); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); hr = ID3D12Device9_CreateShaderCacheSession(device, NULL, &IID_IUnknown, NULL); @@ -38309,6 +38310,7 @@ static void test_shader_cache(void) ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, NULL, (void **)&unk); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + ok(unk == (IUnknown *)0xdeadbeef, "Got unexpected pointer %p.\n", unk); desc.Identifier = test_guid; desc.Mode = D3D12_SHADER_CACHE_MODE_MEMORY; @@ -38357,10 +38359,13 @@ static void test_shader_cache(void) (void **)&session); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); ID3D12ShaderCacheSession_Release(session); + + session = (ID3D12ShaderCacheSession *)0xdeadbeef; desc.MaximumValueFileSizeBytes = 1024 * 1024 * 1024 + 1; hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, &IID_ID3D12ShaderCacheSession, (void **)&session); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + ok(session == (ID3D12ShaderCacheSession *)0xdeadbeef, "Got unexpected pointer %p.\n", unk); memset(&desc, 0, sizeof(desc)); desc.Identifier = test_guid; @@ -38381,6 +38386,7 @@ static void test_shader_cache(void) desc.Identifier = test_guid; desc.Mode = D3D12_SHADER_CACHE_MODE_MEMORY; desc.Flags = 0x1245670; + session = (ID3D12ShaderCacheSession *)0xdeadbeef; hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, &IID_ID3D12ShaderCacheSession, (void **)&session); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); @@ -38389,6 +38395,7 @@ static void test_shader_cache(void) hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, &IID_ID3D12ShaderCacheSession, (void **)&session); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + ok(session == (ID3D12ShaderCacheSession *)0xdeadbeef, "Got unexpected pointer %p.\n", unk); memset(&desc, 0, sizeof(desc)); desc.Identifier = test_guid; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766
From: Stefan Dösinger <stefan(a)codeweavers.com> --- Makefile.am | 1 + libs/vkd3d/cache.c | 81 ++++++++++++++++++++++++++++++++++++++ libs/vkd3d/vkd3d_private.h | 36 +++++++++++++++++ 3 files changed, 118 insertions(+) create mode 100644 libs/vkd3d/cache.c diff --git a/Makefile.am b/Makefile.am index 68e8642e0..86b7afdd9 100644 --- a/Makefile.am +++ b/Makefile.am @@ -351,6 +351,7 @@ libvkd3d_la_SOURCES = \ include/vkd3d_d3d12.idl \ include/vkd3d_d3dcommon.idl \ include/vkd3d_unknown.idl \ + libs/vkd3d/cache.c \ libs/vkd3d/command.c \ libs/vkd3d/device.c \ libs/vkd3d/resource.c \ diff --git a/libs/vkd3d/cache.c b/libs/vkd3d/cache.c new file mode 100644 index 000000000..7f6b712e8 --- /dev/null +++ b/libs/vkd3d/cache.c @@ -0,0 +1,81 @@ +/* + * Copyright 2024 Stefan Dösinger for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include "vkd3d_private.h" + +struct vkd3d_shader_cache +{ + unsigned int refcount; + struct vkd3d_shader_cache_info info; + char filename[]; +}; + +int vkd3d_shader_open_cache(const struct vkd3d_shader_cache_info *info, + struct vkd3d_shader_cache **cache) +{ + struct vkd3d_shader_cache *object; + size_t size = 0; + + TRACE("%p, %p.\n", info, cache); + + if (!info) + { + WARN("No cache info, returning VKD3D_ERROR_INVALID_ARGUMENT.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } + if (info->filename && !(size = strlen(info->filename))) + { + WARN("Filename is an empty string, returning VKD3D_ERROR_INVALID_ARGUMENT.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } + + size++; + object = vkd3d_calloc(1, offsetof(struct vkd3d_shader_cache, filename[size])); + if (!object) + return VKD3D_ERROR_OUT_OF_MEMORY; + + object->refcount = 1; + object->info = *info; + if (info->filename) + { + memcpy(object->filename, info->filename, size); + object->info.filename = object->filename; + } + + *cache = object; + return VKD3D_OK; +} + +unsigned int vkd3d_shader_cache_incref(struct vkd3d_shader_cache *cache) +{ + unsigned int refcount = vkd3d_atomic_increment_u32(&cache->refcount); + TRACE("cache %p refcount %u.\n", cache, refcount); + return refcount; +} + +unsigned int vkd3d_shader_cache_decref(struct vkd3d_shader_cache *cache) +{ + unsigned int refcount = vkd3d_atomic_decrement_u32(&cache->refcount); + TRACE("cache %p refcount %u.\n", cache, refcount); + + if (refcount) + return refcount; + + vkd3d_free(cache); + return 0; +} diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index d5e12ee3c..c9f957ce6 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -1927,4 +1927,40 @@ static inline void vkd3d_prepend_struct(void *header, void *structure) vkd3d_header->next = vkd3d_structure; } +struct vkd3d_shader_cache; + +enum vkd3d_shader_cache_flags +{ + VKD3D_SHADER_CACHE_FLAGS_NONE = 0x00000000, + VKD3D_SHADER_CACHE_FLAGS_NO_SERIALIZE = 0x00000001, + VKD3D_SHADER_CACHE_FLAGS_READ_ONLY = 0x00000002, + + VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_CACHE_FLAGS), +}; + +struct vkd3d_shader_cache_info +{ + /*enum vkd3d_structure_type type;*/ + const void *next; + + /** File name to open, or NULL for a memory-only cache. */ + const char *filename; + /** Maximum amount of data the cache holds in memory. */ + uint64_t mem_size; + /** Maximum amount of data written to disk. Ignored if filename is NULL. */ + uint64_t disk_size; + /** Maximum number of cache entries. */ + uint64_t max_entries; + /** Random flags, what else. */ + enum vkd3d_shader_cache_flags flags; + /** An application-chosen version number. If the version of an existing + * cache on disk does not match, the old data will be discarded. */ + uint64_t version; +}; + +int vkd3d_shader_open_cache(const struct vkd3d_shader_cache_info *info, + struct vkd3d_shader_cache **cache); +unsigned int vkd3d_shader_cache_incref(struct vkd3d_shader_cache *cache); +unsigned int vkd3d_shader_cache_decref(struct vkd3d_shader_cache *cache); + #endif /* __VKD3D_PRIVATE_H */ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766
From: Stefan Dösinger <stefan(a)codeweavers.com> --- I don't have objections against merging this into the previous patch. --- libs/vkd3d/device.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index 65db8b70b..ad164201e 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -2532,6 +2532,7 @@ struct d3d12_cache_session struct d3d12_device *device; struct vkd3d_private_store private_store; D3D12_SHADER_CACHE_SESSION_DESC desc; + struct vkd3d_shader_cache *cache; }; static inline struct d3d12_cache_session *impl_from_ID3D12ShaderCacheSession(ID3D12ShaderCacheSession *iface) @@ -2582,6 +2583,7 @@ static void d3d12_cache_session_destroy(struct d3d12_cache_session *session) TRACE("Destroying cache session %p.\n", session); + vkd3d_shader_cache_decref(session->cache); vkd3d_private_store_destroy(&session->private_store); vkd3d_free(session); @@ -2707,6 +2709,8 @@ static const struct ID3D12ShaderCacheSessionVtbl d3d12_cache_session_vtbl = static HRESULT d3d12_cache_session_init(struct d3d12_cache_session *session, struct d3d12_device *device, const D3D12_SHADER_CACHE_SESSION_DESC *desc) { + struct vkd3d_shader_cache_info cache_info = {0}; + enum vkd3d_result ret; HRESULT hr; session->ID3D12ShaderCacheSession_iface.lpVtbl = &d3d12_cache_session_vtbl; @@ -2723,6 +2727,20 @@ static HRESULT d3d12_cache_session_init(struct d3d12_cache_session *session, if (FAILED(hr = vkd3d_private_store_init(&session->private_store))) return hr; + cache_info.max_entries = session->desc.MaximumInMemoryCacheEntries; + cache_info.mem_size = session->desc.MaximumInMemoryCacheSizeBytes; + cache_info.version = session->desc.Version; + if (session->desc.Mode == D3D12_SHADER_CACHE_MODE_DISK) + FIXME("Disk caches are not yet implemented.\n"); + + ret = vkd3d_shader_open_cache(&cache_info, &session->cache); + if (ret) + { + WARN("Failed to open shader cache.\n"); + vkd3d_private_store_destroy(&session->private_store); + return hresult_from_vkd3d_result(ret); + } + d3d12_device_add_ref(session->device = device); return S_OK; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766
From: Stefan Dösinger <stefan(a)codeweavers.com> --- include/private/vkd3d_common.h | 6 +++ libs/vkd3d/device.c | 69 +++++++++++++++++++++++++++++----- tests/d3d12.c | 6 +-- 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/include/private/vkd3d_common.h b/include/private/vkd3d_common.h index 6a8694a79..10e2afd5d 100644 --- a/include/private/vkd3d_common.h +++ b/include/private/vkd3d_common.h @@ -438,6 +438,12 @@ struct vkd3d_mutex #endif }; +#ifdef _WIN32 +#define VKD3D_MUTEX_INITIALIZER {{NULL, -1, 0, 0, 0, 0}} +#else +#define VKD3D_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER +#endif + static inline void vkd3d_mutex_init(struct vkd3d_mutex *lock) { #ifdef _WIN32 diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index ad164201e..9b33be235 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -2529,12 +2529,17 @@ struct d3d12_cache_session ID3D12ShaderCacheSession ID3D12ShaderCacheSession_iface; unsigned int refcount; + struct list cache_list_entry; + struct d3d12_device *device; struct vkd3d_private_store private_store; D3D12_SHADER_CACHE_SESSION_DESC desc; struct vkd3d_shader_cache *cache; }; +static struct vkd3d_mutex cache_list_mutex = VKD3D_MUTEX_INITIALIZER; +static struct list cache_list = LIST_INIT(cache_list); + static inline struct d3d12_cache_session *impl_from_ID3D12ShaderCacheSession(ID3D12ShaderCacheSession *iface) { return CONTAINING_RECORD(iface, struct d3d12_cache_session, ID3D12ShaderCacheSession_iface); @@ -2583,6 +2588,10 @@ static void d3d12_cache_session_destroy(struct d3d12_cache_session *session) TRACE("Destroying cache session %p.\n", session); + vkd3d_mutex_lock(&cache_list_mutex); + list_remove(&session->cache_list_entry); + vkd3d_mutex_unlock(&cache_list_mutex); + vkd3d_shader_cache_decref(session->cache); vkd3d_private_store_destroy(&session->private_store); vkd3d_free(session); @@ -2710,12 +2719,14 @@ static HRESULT d3d12_cache_session_init(struct d3d12_cache_session *session, struct d3d12_device *device, const D3D12_SHADER_CACHE_SESSION_DESC *desc) { struct vkd3d_shader_cache_info cache_info = {0}; + struct d3d12_cache_session *i; enum vkd3d_result ret; HRESULT hr; session->ID3D12ShaderCacheSession_iface.lpVtbl = &d3d12_cache_session_vtbl; session->refcount = 1; session->desc = *desc; + session->cache = NULL; if (!session->desc.MaximumValueFileSizeBytes) session->desc.MaximumValueFileSizeBytes = 128 * 1024 * 1024; @@ -2727,23 +2738,60 @@ static HRESULT d3d12_cache_session_init(struct d3d12_cache_session *session, if (FAILED(hr = vkd3d_private_store_init(&session->private_store))) return hr; - cache_info.max_entries = session->desc.MaximumInMemoryCacheEntries; - cache_info.mem_size = session->desc.MaximumInMemoryCacheSizeBytes; - cache_info.version = session->desc.Version; - if (session->desc.Mode == D3D12_SHADER_CACHE_MODE_DISK) - FIXME("Disk caches are not yet implemented.\n"); + vkd3d_mutex_lock(&cache_list_mutex); + + /* We expect the number of open caches to be small. */ + LIST_FOR_EACH_ENTRY(i, &cache_list, struct d3d12_cache_session, cache_list_entry) + { + if (!memcmp(&i->desc.Identifier, &desc->Identifier, sizeof(desc->Identifier))) + { + TRACE("Found an existing cache %p from session %p.\n", i->cache, i); + if (desc->Version == i->desc.Version) + { + session->desc = i->desc; + vkd3d_shader_cache_incref(session->cache = i->cache); + break; + } + else + { + WARN("version mismatch: Existing %"PRIu64" new %"PRIu64".\n", + i->desc.Version, desc->Version); + hr = DXGI_ERROR_ALREADY_EXISTS; + goto error; + } + } + } - ret = vkd3d_shader_open_cache(&cache_info, &session->cache); - if (ret) + if (!session->cache) { - WARN("Failed to open shader cache.\n"); - vkd3d_private_store_destroy(&session->private_store); - return hresult_from_vkd3d_result(ret); + cache_info.max_entries = session->desc.MaximumInMemoryCacheEntries; + cache_info.mem_size = session->desc.MaximumInMemoryCacheSizeBytes; + cache_info.version = session->desc.Version; + if (session->desc.Mode == D3D12_SHADER_CACHE_MODE_DISK) + FIXME("Disk caches are not yet implemented.\n"); + + ret = vkd3d_shader_open_cache(&cache_info, &session->cache); + if (ret) + { + WARN("Failed to open shader cache.\n"); + hr = hresult_from_vkd3d_result(ret); + goto error; + } } + /* Yes, we add this unconditionally, even if we reused an existing cache from a different + * session. The other session might be destroyed, but the cache stays alive and can be opened + * a third time. */ + list_add_tail(&cache_list, &session->cache_list_entry); d3d12_device_add_ref(session->device = device); + vkd3d_mutex_unlock(&cache_list_mutex); return S_OK; + +error: + vkd3d_private_store_destroy(&session->private_store); + vkd3d_mutex_unlock(&cache_list_mutex); + return hr; } /* ID3D12Device */ @@ -4892,6 +4940,7 @@ static HRESULT STDMETHODCALLTYPE d3d12_device_CreateShaderCacheSession(ID3D12Dev WARN("No output pointer, returning S_FALSE.\n"); return S_FALSE; } + *session = NULL; if (!(object = vkd3d_malloc(sizeof(*object)))) return E_OUTOFMEMORY; diff --git a/tests/d3d12.c b/tests/d3d12.c index fb151cad3..ed8c8a09e 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -38438,10 +38438,8 @@ static void test_shader_cache(void) session2 = (void *)0xdeadbeef; hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, &IID_ID3D12ShaderCacheSession, (void **)&session2); - todo ok(hr == DXGI_ERROR_ALREADY_EXISTS, "Got unexpected hr %#x.\n", hr); - todo ok(!session2, "Got unexpected pointer %p.\n", session2); - if (session2) - ID3D12ShaderCacheSession_Release(session2); + ok(hr == DXGI_ERROR_ALREADY_EXISTS, "Got unexpected hr %#x.\n", hr); + ok(!session2, "Got unexpected pointer %p.\n", session2); hr = ID3D12Device9_CreateShaderCacheSession(device, &desc, &IID_IUnknown, NULL); ok(hr == S_FALSE, "NULL outptr: Got hr %#x.\n", hr); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766
```diff + if (!info) + { + WARN("No cache info, returning VKD3D_ERROR_INVALID_ARGUMENT.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } + if (info->filename && !(size = strlen(info->filename))) + { + WARN("Filename is an empty string, returning VKD3D_ERROR_INVALID_ARGUMENT.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } ```
Do we need to explicitly verify these? It feels a little superfluous.
```diff +unsigned int vkd3d_shader_cache_incref(struct vkd3d_shader_cache *cache) +{ + unsigned int refcount = vkd3d_atomic_increment_u32(&cache->refcount); + TRACE("cache %p refcount %u.\n", cache, refcount); + return refcount; +} + +unsigned int vkd3d_shader_cache_decref(struct vkd3d_shader_cache *cache) +{ + unsigned int refcount = vkd3d_atomic_decrement_u32(&cache->refcount); + TRACE("cache %p refcount %u.\n", cache, refcount); + + if (refcount) + return refcount; + + vkd3d_free(cache); + return 0; +} ```
Do we need refcounts? I may be misremembering, but I think we were moving towards only opening caches once, and the caller then being responsible for managing their lifetime, at least on the vkd3d-shader level. In any case, this is unused code in this commit.
```diff +enum vkd3d_shader_cache_flags +{ + VKD3D_SHADER_CACHE_FLAGS_NONE = 0x00000000, + VKD3D_SHADER_CACHE_FLAGS_NO_SERIALIZE = 0x00000001, + VKD3D_SHADER_CACHE_FLAGS_READ_ONLY = 0x00000002, + + VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_CACHE_FLAGS), +}; + +struct vkd3d_shader_cache_info +{ + /*enum vkd3d_structure_type type;*/ + const void *next; + + /** File name to open, or NULL for a memory-only cache. */ + const char *filename; + /** Maximum amount of data the cache holds in memory. */ + uint64_t mem_size; + /** Maximum amount of data written to disk. Ignored if filename is NULL. */ + uint64_t disk_size; + /** Maximum number of cache entries. */ + uint64_t max_entries; + /** Random flags, what else. */ + enum vkd3d_shader_cache_flags flags; + /** An application-chosen version number. If the version of an existing + * cache on disk does not match, the old data will be discarded. */ + uint64_t version; +}; ```
Somewhat similarly, much of this is unused in this MR.
``` I don't have objections against merging this into the previous patch. ```
That would help, but note that you'd still have a decent number of unused things.
```diff + /* We expect the number of open caches to be small. */ + LIST_FOR_EACH_ENTRY(i, &cache_list, struct d3d12_cache_session, cache_list_entry) + { + if (!memcmp(&i->desc.Identifier, &desc->Identifier, sizeof(desc->Identifier))) + { + TRACE("Found an existing cache %p from session %p.\n", i->cache, i); + if (desc->Version == i->desc.Version) + { + session->desc = i->desc; + vkd3d_shader_cache_incref(session->cache = i->cache); + break; + } + else + { + WARN("version mismatch: Existing %"PRIu64" new %"PRIu64".\n", + i->desc.Version, desc->Version); + hr = DXGI_ERROR_ALREADY_EXISTS; + goto error; + } + } + } ```
Should we just return the existing ID3D12ShaderCacheSession? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766#note_67272
Do we need to explicitly verify these? It feels a little superfluous.
As long as these functions are private in libvkd3d we don't necessarily need them, but once this becomes public I want to catch parameters that would cause problems - e.g. a zero-length file name makes a cache memory only. I can remove them for now and add them right before or in the same commit that moves this code to vkd3d_shader.
Do we need refcounts? I may be misremembering, but I think we were moving towards only opening caches once, and the caller then being responsible for managing their lifetime, at least on the vkd3d-shader level. In any case, this is unused code in this commit.
CreateShaderCacheSession returns different objects when reopening an existing cache. This is observable not only by comparing pointers, but also e.g. through SetPrivateData et al. I thought I already had tests for that in the test submitted in the previous MR, but I see they are missing there because they are mixed with the StoreValue/FindValue tests. I'll add some to this MR. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766#note_67326
Do we need to explicitly verify these? It feels a little superfluous.
As long as these functions are private in libvkd3d we don't necessarily need them, but once this becomes public I want to catch parameters that would cause problems - e.g. a zero-length file name makes a cache memory only.
Should it? I think we could just require info->filename to be NULL for that case.
I can remove them for now and add them right before or in the same commit that moves this code to vkd3d_shader.
That's fine in any case. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766#note_67390
Should it? I think we could just require info->filename to be NULL for that case.
My comment was phrased poorly - from the caller's side yes, info->filename = NULL will be the way to request a memory only cache. What I wanted to say is that in the case of a memory only cache cache->filename (dynamic length array at the end of struct vkd3d_shader_cache) becomes an empty string and I want to prevent cases where the caller passes info->filename = "". Although in practise I'd expect fopen("") to fail and such a cache never be created with or without an explicit check. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/766#note_67470
participants (2)
-
Henri Verbeet (＠hverbeet) -
Stefan Dösinger