Re: [PATCH v4 2/2] vkd3d: Don't allow concurrent writes to descriptors.

[Derek Lesho]

On 9/13/19 10:59 AM, Sveinar Søpler wrote:
> Would this by any chance fix something regarding this bug: https://bugs.winehq.org/show_bug.cgi?id=46410
>
> Sveinar
>
> ----- On Sep 13, 2019, at 4:19 PM, Derek Lesho [e-mail] wrote:
> > In D3D12 it is legal to concurrently overwrite descriptors, we can't handle
> > that.
> > This patch locks descriptors from being overwritten by two threads at the same
> > time.
> > Signed-off-by: Derek Lesho
> >
> > v4: Allocate the descriptor locks for a heap in a single array to avoid
> > unecessary allocations.
> >
> > libs/vkd3d/resource.c      | 207 ++++++++++++++++++++++++++-----------
> > libs/vkd3d/vkd3d_private.h |   5 +
> > 2 files changed, 152 insertions(+), 60 deletions(-)
> >
> > diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c
> > index 463f373..e6d3b51 100644
> > --- a/libs/vkd3d/resource.c
> > +++ b/libs/vkd3d/resource.c
> > @@ -1947,8 +1947,13 @@ static void d3d12_desc_destroy(struct d3d12_desc
> > *descriptor,
> > void d3d12_desc_copy(struct d3d12_desc *dst, const struct d3d12_desc *src,
> >          struct d3d12_device *device)
> > {
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> > -
> >
> > assert(dst != src);
> > -
> >
> > pthread_spin_lock(dst->lock);
> > -
> >
> > descriptor_lock = dst->lock;
> > -
> >
> > d3d12_desc_destroy(dst, device);
> >
> > *dst = *src;
> >
> > @@ -1959,6 +1964,8 @@ void d3d12_desc_copy(struct d3d12_desc *dst, const struct
> > d3d12_desc *src,
> >      {
> >          vkd3d_view_incref(src->u.view);
> >      }
> > -
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > static VkDeviceSize vkd3d_get_required_texel_buffer_alignment(const struct
> > d3d12_device *device,
> > @@ -2322,22 +2329,22 @@ void d3d12_desc_create_cbv(struct d3d12_desc
> > *descriptor,
> > {
> >      struct VkDescriptorBufferInfo *buffer_info;
> >      struct d3d12_resource *resource;
> > -
> > - d3d12_desc_destroy(descriptor, device);
> > -
> >
> > struct d3d12_desc new_desc = {};
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!desc)
> > {
> >     WARN("Constant buffer desc is NULL.\n");
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (desc->SizeInBytes & (D3D12_CONSTANT_BUFFER_DATA_PLACEMENT_ALIGNMENT - 1))
> >    {
> >        WARN("Size is not %u bytes aligned.\n",
> >        D3D12_CONSTANT_BUFFER_DATA_PLACEMENT_ALIGNMENT);
> > -
> >    return;
> > -
> >    goto done;
> > }
> > - buffer_info = &descriptor->u.vk_cbv_info;
> > - buffer_info = &new_desc.u.vk_cbv_info;
> > if (desc->BufferLocation)
> > {
> >     resource = vkd3d_gpu_va_allocator_dereference(&device->gpu_va_allocator,
> >     desc->BufferLocation);
> >
> > @@ -2353,8 +2360,18 @@ void d3d12_desc_create_cbv(struct d3d12_desc *descriptor,
> >          buffer_info->range = VKD3D_NULL_BUFFER_SIZE;
> >      }
> > - descriptor->magic = VKD3D_DESCRIPTOR_MAGIC_CBV;
> > - descriptor->vk_descriptor_type = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
> > - new_desc.magic = VKD3D_DESCRIPTOR_MAGIC_CBV;
> > - new_desc.vk_descriptor_type = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
> > -
> > - done:
> > - pthread_spin_lock(descriptor->lock);
> > - descriptor_lock = descriptor->lock;
> > -
> > - d3d12_desc_destroy(descriptor, device);
> > - *descriptor = new_desc;
> > -
> > - descriptor->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > static unsigned int
> > vkd3d_view_flags_from_d3d12_buffer_srv_flags(D3D12_BUFFER_SRV_FLAGS flags)
> > @@ -2466,23 +2483,23 @@ void d3d12_desc_create_srv(struct d3d12_desc
> > *descriptor,
> > {
> >      struct vkd3d_texture_view_desc vkd3d_desc;
> >      struct vkd3d_view *view;
> > -
> > - d3d12_desc_destroy(descriptor, device);
> > -
> >
> > struct d3d12_desc new_desc = {};
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!resource)
> > {
> > -
> >    vkd3d_create_null_srv(descriptor, device, desc);
> > -
> >    return;
> > -
> >    vkd3d_create_null_srv(&new_desc, device, desc);
> > -
> >    goto done;
> >
> > }
> >
> > if (d3d12_resource_is_buffer(resource))
> >    {
> > -
> >    vkd3d_create_buffer_srv(descriptor, device, resource, desc);
> > -
> >    return;
> > -
> >    vkd3d_create_buffer_srv(&new_desc, device, resource, desc);
> > -
> >    goto done;
> >
> > }
> >
> > if (!init_default_texture_view_desc(&vkd3d_desc, resource, desc ? desc->Format :
> >    0))
> > -
> >    return;
> > -
> >    goto done;
> >
> > vkd3d_desc.miplevel_count = VK_REMAINING_MIP_LEVELS;
> >    vkd3d_desc.allowed_swizzle = true;
> >
> > @@ -2559,11 +2576,21 @@ void d3d12_desc_create_srv(struct d3d12_desc
> > *descriptor,
> >      }
> >  if (!vkd3d_create_texture_view(device, resource->u.vk_image, &vkd3d_desc,
> >  &view))
> > -
> >    return;
> > -
> >    goto done;
> >
> > - descriptor->magic = VKD3D_DESCRIPTOR_MAGIC_SRV;
> > - descriptor->vk_descriptor_type = VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE;
> > - descriptor->u.view = view;
> > - new_desc.magic = VKD3D_DESCRIPTOR_MAGIC_SRV;
> > - new_desc.vk_descriptor_type = VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE;
> > - new_desc.u.view = view;
> > -
> > - done:
> > - pthread_spin_lock(descriptor->lock);
> > - descriptor_lock = descriptor->lock;
> > -
> > - d3d12_desc_destroy(descriptor, device);
> > - *descriptor = new_desc;
> > -
> > - descriptor->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > static unsigned int
> > vkd3d_view_flags_from_d3d12_buffer_uav_flags(D3D12_BUFFER_UAV_FLAGS flags)
> > @@ -2761,26 +2788,37 @@ void d3d12_desc_create_uav(struct d3d12_desc
> > *descriptor, struct d3d12_device *d
> >          struct d3d12_resource *resource, struct d3d12_resource *counter_resource,
> >          const D3D12_UNORDERED_ACCESS_VIEW_DESC *desc)
> > {
> > - d3d12_desc_destroy(descriptor, device);
> > -
> >
> > struct d3d12_desc new_desc = {};
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!resource)
> > {
> >     if (counter_resource)
> >         FIXME("Ignoring counter resource %p.\n", counter_resource);
> > -
> >    vkd3d_create_null_uav(descriptor, device, desc);
> > -
> >    return;
> > -
> >    vkd3d_create_null_uav(&new_desc, device, desc);
> > -
> >    goto done;
> >
> > }
> >
> > if (d3d12_resource_is_buffer(resource))
> >    {
> > -
> >    vkd3d_create_buffer_uav(descriptor, device, resource, counter_resource,
> >
> > desc);
> > -
> >    vkd3d_create_buffer_uav(&new_desc, device, resource, counter_resource,
> >
> > desc);
> >      }
> >      else
> >      {
> >          if (counter_resource)
> >              FIXME("Unexpected counter resource for texture view.\n");
> > -
> >    vkd3d_create_texture_uav(descriptor, device, resource, desc);
> > -
> >    vkd3d_create_texture_uav(&new_desc, device, resource, desc);
> > }
> > -
> > - done:
> > - pthread_spin_lock(descriptor->lock);
> > - descriptor_lock = descriptor->lock;
> > -
> > - d3d12_desc_destroy(descriptor, device);
> > - *descriptor = new_desc;
> > -
> > - descriptor->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > bool vkd3d_create_raw_buffer_view(struct d3d12_device *device,
> > @@ -2888,13 +2926,13 @@ void d3d12_desc_create_sampler(struct d3d12_desc
> > *sampler,
> >          struct d3d12_device *device, const D3D12_SAMPLER_DESC *desc)
> > {
> >      struct vkd3d_view *view;
> > -
> > - d3d12_desc_destroy(sampler, device);
> > -
> >
> > struct d3d12_desc new_sampler;
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!desc)
> > {
> >     WARN("NULL sampler desc.\n");
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (desc->AddressU == D3D12_TEXTURE_ADDRESS_MODE_BORDER
> >
> > @@ -2904,19 +2942,29 @@ void d3d12_desc_create_sampler(struct d3d12_desc
> > *sampler,
> >                  desc->BorderColor[0], desc->BorderColor[1], desc->BorderColor[2],
> >                  desc->BorderColor[3]);
> >  if (!(view = vkd3d_view_create()))
> > -
> >    return;
> > -
> >    goto done;
> >
> > if (d3d12_create_sampler(device, desc->Filter, desc->AddressU,
> >            desc->AddressV, desc->AddressW, desc->MipLODBias, desc->MaxAnisotropy,
> >            desc->ComparisonFunc, desc->MinLOD, desc->MaxLOD, &view->u.vk_sampler) < 0)
> >    {
> >        vkd3d_free(view);
> > -
> >    return;
> > -
> >    goto done;
> > }
> > - sampler->magic = VKD3D_DESCRIPTOR_MAGIC_SAMPLER;
> > - sampler->vk_descriptor_type = VK_DESCRIPTOR_TYPE_SAMPLER;
> > - sampler->u.view = view;
> > - new_sampler.magic = VKD3D_DESCRIPTOR_MAGIC_SAMPLER;
> > - new_sampler.vk_descriptor_type = VK_DESCRIPTOR_TYPE_SAMPLER;
> > - new_sampler.u.view = view;
> > -
> > - done:
> > - pthread_spin_lock(sampler->lock);
> > - descriptor_lock = sampler->lock;
> > -
> > - d3d12_desc_destroy(sampler, device);
> > - *sampler = new_sampler;
> > -
> > - sampler->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > HRESULT vkd3d_create_static_sampler(struct d3d12_device *device,
> > @@ -2950,22 +2998,22 @@ void d3d12_rtv_desc_create_rtv(struct d3d12_rtv_desc
> > *rtv_desc, struct d3d12_dev
> > {
> >      struct vkd3d_texture_view_desc vkd3d_desc;
> >      struct vkd3d_view *view;
> > -
> > - d3d12_rtv_desc_destroy(rtv_desc, device);
> > -
> >
> > struct d3d12_rtv_desc new_rtv_desc = {};
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!resource)
> > {
> >     FIXME("NULL resource RTV not implemented.\n");
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (!init_default_texture_view_desc(&vkd3d_desc, resource, desc ? desc->Format :
> >    0))
> > -
> >    return;
> > -
> >    goto done;
> >
> > if (vkd3d_desc.format->vk_aspect_mask != VK_IMAGE_ASPECT_COLOR_BIT)
> >    {
> >        WARN("Trying to create RTV for depth/stencil format %#x.\n",
> >        vkd3d_desc.format->dxgi_format);
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (desc)
> >
> > @@ -3013,16 +3061,26 @@ void d3d12_rtv_desc_create_rtv(struct d3d12_rtv_desc
> > *rtv_desc, struct d3d12_dev
> >      assert(d3d12_resource_is_texture(resource));
> >  if (!vkd3d_create_texture_view(device, resource->u.vk_image, &vkd3d_desc,
> >  &view))
> > -
> >    return;
> > -
> >    goto done;
> >
> > - rtv_desc->magic = VKD3D_DESCRIPTOR_MAGIC_RTV;
> > - rtv_desc->sample_count =
> >
> > vk_samples_from_dxgi_sample_desc(&resource->desc.SampleDesc);
> > - rtv_desc->format = vkd3d_desc.format;
> > - rtv_desc->width = d3d12_resource_desc_get_width(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - rtv_desc->height = d3d12_resource_desc_get_height(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - rtv_desc->layer_count = vkd3d_desc.layer_count;
> > - rtv_desc->view = view;
> > - rtv_desc->resource = resource;
> > - new_rtv_desc.magic = VKD3D_DESCRIPTOR_MAGIC_RTV;
> > - new_rtv_desc.sample_count =
> >
> > vk_samples_from_dxgi_sample_desc(&resource->desc.SampleDesc);
> > - new_rtv_desc.format = vkd3d_desc.format;
> > - new_rtv_desc.width = d3d12_resource_desc_get_width(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - new_rtv_desc.height = d3d12_resource_desc_get_height(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - new_rtv_desc.layer_count = vkd3d_desc.layer_count;
> > - new_rtv_desc.view = view;
> > - new_rtv_desc.resource = resource;
> > -
> > - done:
> > - pthread_spin_lock(rtv_desc->lock);
> > - descriptor_lock = rtv_desc->lock;
> > -
> > - d3d12_rtv_desc_destroy(rtv_desc, device);
> > - *rtv_desc = new_rtv_desc;
> > -
> > - rtv_desc->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > /* DSVs */
> > @@ -3040,28 +3098,28 @@ void d3d12_dsv_desc_create_dsv(struct d3d12_dsv_desc
> > *dsv_desc, struct d3d12_dev
> > {
> >      struct vkd3d_texture_view_desc vkd3d_desc;
> >      struct vkd3d_view *view;
> > -
> > - d3d12_dsv_desc_destroy(dsv_desc, device);
> > -
> >
> > struct d3d12_dsv_desc new_dsv_desc = {};
> > -
> >
> > pthread_spinlock_t *descriptor_lock;
> >
> > if (!resource)
> > {
> >     FIXME("NULL resource DSV not implemented.\n");
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (resource->desc.Dimension == D3D12_RESOURCE_DIMENSION_TEXTURE3D)
> >    {
> >        WARN("Cannot create DSV for 3D texture.\n");
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (!init_default_texture_view_desc(&vkd3d_desc, resource, desc ? desc->Format :
> >    0))
> > -
> >    return;
> > -
> >    goto done;
> >
> > if (!(vkd3d_desc.format->vk_aspect_mask & (VK_IMAGE_ASPECT_DEPTH_BIT |
> >    VK_IMAGE_ASPECT_STENCIL_BIT)))
> >    {
> >        WARN("Trying to create DSV for format %#x.\n", vkd3d_desc.format->dxgi_format);
> > -
> >    return;
> > -
> >    goto done;
> >
> > }
> >
> > if (desc)
> >
> > @@ -3096,16 +3154,26 @@ void d3d12_dsv_desc_create_dsv(struct d3d12_dsv_desc
> > *dsv_desc, struct d3d12_dev
> >      assert(d3d12_resource_is_texture(resource));
> >  if (!vkd3d_create_texture_view(device, resource->u.vk_image, &vkd3d_desc,
> >  &view))
> > -
> >    return;
> > -
> >    goto done;
> > -
> > - new_dsv_desc.magic = VKD3D_DESCRIPTOR_MAGIC_DSV;
> > - new_dsv_desc.sample_count =
> >
> > vk_samples_from_dxgi_sample_desc(&resource->desc.SampleDesc);
> > - new_dsv_desc.format = vkd3d_desc.format;
> > - new_dsv_desc.width = d3d12_resource_desc_get_width(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - new_dsv_desc.height = d3d12_resource_desc_get_height(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - new_dsv_desc.layer_count = vkd3d_desc.layer_count;
> > - new_dsv_desc.view = view;
> > - new_dsv_desc.resource = resource;
> > -
> > - done:
> > - pthread_spin_lock(dsv_desc->lock);
> > - descriptor_lock = dsv_desc->lock;
> > -
> > - d3d12_dsv_desc_destroy(dsv_desc, device);
> > - *dsv_desc = new_dsv_desc;
> > - dsv_desc->magic = VKD3D_DESCRIPTOR_MAGIC_DSV;
> > - dsv_desc->sample_count =
> >
> > vk_samples_from_dxgi_sample_desc(&resource->desc.SampleDesc);
> > - dsv_desc->format = vkd3d_desc.format;
> > - dsv_desc->width = d3d12_resource_desc_get_width(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - dsv_desc->height = d3d12_resource_desc_get_height(&resource->desc,
> >
> > vkd3d_desc.miplevel_idx);
> > - dsv_desc->layer_count = vkd3d_desc.layer_count;
> > - dsv_desc->view = view;
> > - dsv_desc->resource = resource;
> > - dsv_desc->lock = descriptor_lock;
> > - pthread_spin_unlock(descriptor_lock);
> >
> > }
> >
> > /* ID3D12DescriptorHeap */
> > @@ -3200,6 +3268,13 @@ static ULONG STDMETHODCALLTYPE
> > d3d12_descriptor_heap_Release(ID3D12DescriptorHea
> >                  break;
> >          }
> > -
> >    for (i = 0; i < heap->desc.NumDescriptors; i++)
> > -
> >    {
> > -
> >        pthread_spin_destroy(&heap->locks[i]);
> > -
> >    }
> > -
> > -
> >    vkd3d_free((void*)heap->locks);
> > -
> >    vkd3d_free(heap);
> >
> >    d3d12_device_release(device);
> >
> > @@ -3323,6 +3398,9 @@ static HRESULT d3d12_descriptor_heap_init(struct
> > d3d12_descriptor_heap *descript
> >      if (FAILED(hr = vkd3d_private_store_init(&descriptor_heap->private_store)))
> >          return hr;
> > - if (!(descriptor_heap->locks = vkd3d_malloc(sizeof(pthread_spinlock_t) *
> >
> > desc->NumDescriptors)))
> > -
> >    return E_OUTOFMEMORY;
> > -
> >
> > d3d12_device_add_ref(descriptor_heap->device = device);
> >
> > return S_OK;
> >
> > @@ -3367,6 +3445,15 @@ HRESULT d3d12_descriptor_heap_create(struct d3d12_device
> > *device,
> >  memset(object->descriptors, 0, descriptor_size * desc->NumDescriptors);
> >
> > - for (unsigned int i = 0; i < desc->NumDescriptors; i++)
> > - {
> > -
> >    struct d3d12_desc *cur_desc = (struct d3d12_desc *)
> >
> > (object->descriptors + (i * descriptor_size));
> > -
> >    pthread_spinlock_t *lock = &object->locks[i];
> > -
> > -
> >    pthread_spin_init(lock, PTHREAD_PROCESS_PRIVATE);
> > -
> >    cur_desc->lock = lock;
> > -
> >
> > }
> > -
> >
> > TRACE("Created descriptor heap %p.\n", object);
> >
> > *descriptor_heap = object;
> >
> > diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h
> > index bd9670e..732ded5 100644
> > --- a/libs/vkd3d/vkd3d_private.h
> > +++ b/libs/vkd3d/vkd3d_private.h
> > @@ -466,6 +466,7 @@ void vkd3d_view_incref(struct vkd3d_view *view)
> > DECLSPEC_HIDDEN;
> > struct d3d12_desc
> > {
> >      uint32_t magic;
> > - pthread_spinlock_t *lock;
> > VkDescriptorType vk_descriptor_type;
> > union
> > {
> >
> > @@ -521,6 +522,7 @@ HRESULT vkd3d_create_static_sampler(struct d3d12_device
> > *device,
> > struct d3d12_rtv_desc
> > {
> >      uint32_t magic;
> > - pthread_spinlock_t *lock;
> > VkSampleCountFlagBits sample_count;
> > const struct vkd3d_format *format;
> > uint64_t width;
> >
> > @@ -541,6 +543,7 @@ void d3d12_rtv_desc_create_rtv(struct d3d12_rtv_desc
> > *rtv_desc, struct d3d12_dev
> > struct d3d12_dsv_desc
> > {
> >      uint32_t magic;
> > - pthread_spinlock_t *lock;
> > VkSampleCountFlagBits sample_count;
> > const struct vkd3d_format *format;
> > uint64_t width;
> >
> > @@ -570,6 +573,8 @@ struct d3d12_descriptor_heap
> >  struct vkd3d_private_store private_store;
> >
> > - pthread_spinlock_t *locks;
> > - BYTE descriptors[];
> >
> > };
> >
> > --
> > 2.23.0

No, this patch is completely unrelated to that bug.

Just so you're aware though, on discord this 
patch(https://github.com/GloriousEggroll/proton-ge-custom/blob/proton-ge-4.15/game...) 
was mentioned, which supposedly fixes that bug, if that's what you're 
looking for.

-Derek