[PATCH vkd3d v2 1/2] vkd3d: Return success for all valid D3D12 alignments.
Windows checks for one of its valid default alignment sizes. The size test should not include DepthOrArraySize, should use the correct size for compressed textures, and the returned alignment should not be tested against the requested one. The current implementation returns ~0 in SizeInBytes for standard D3D12 alignments on some hardware and driver combinations, e.g. AMD RX 580 / RADV. Some games use this value unchecked; for example Hitman 2 will allocate heaps of size 0xffffffff and run out of vram. This patch also fixes RX 580 alignment test failures. Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- Supersedes 173581. --- libs/vkd3d/device.c | 50 ++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index a025e68..7ff567d 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -2938,7 +2938,7 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour if (FAILED(d3d12_resource_validate_desc(desc))) { WARN("Invalid resource desc.\n"); - goto invalid; + goto fail; } requested_alignment = desc->Alignment @@ -2946,6 +2946,8 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour if (desc->Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) { + if (requested_alignment != D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) + goto invalid; info->SizeInBytes = desc->Width; info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; } @@ -2954,32 +2956,43 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour if (FAILED(vkd3d_get_image_allocation_info(device, desc, info))) { WARN("Failed to get allocation info for texture.\n"); - goto invalid; + goto fail; } + /* Validate the alignment in the resource description. Also allow the Vulkan alignment in case the caller + * specifies it in future calls. We *could* enforce D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT, + * but it may prove better to allow 64KB (which D3D12 accepts on newer hardware anyway), and return + * whatever Vulkan requires. */ + if (requested_alignment != info->Alignment + && requested_alignment != D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT + && requested_alignment != D3D12_SMALL_RESOURCE_PLACEMENT_ALIGNMENT + && (desc->SampleDesc.Count == 1 || requested_alignment != D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT)) + goto invalid; + info->Alignment = max(info->Alignment, requested_alignment); - if (info->Alignment < D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) + if (info->Alignment < D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT + || requested_alignment < D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) { if (!(format = vkd3d_format_from_d3d12_resource_desc(device, desc, 0))) { WARN("Invalid format %#x.\n", desc->Format); - goto invalid; + goto fail; } - estimated_size = desc->Width * desc->Height * desc->DepthOrArraySize * format->byte_count; + /* Windows uses the slice size to determine small alignment eligibility. DepthOrArraySize is ignored. */ + estimated_size = vkd3d_format_is_compressed(format) + ? desc->Width * desc->Height * format->block_byte_count / (format->block_width * format->block_height) + : desc->Width * desc->Height * format->byte_count; if (estimated_size > D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) + { + if (requested_alignment == D3D12_SMALL_RESOURCE_PLACEMENT_ALIGNMENT) + goto invalid; info->Alignment = max(info->Alignment, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT); + } } } - if (desc->Alignment % info->Alignment) - { - WARN("Invalid resource alignment %#"PRIx64" (required %#"PRIx64").\n", - desc->Alignment, info->Alignment); - goto invalid; - } - info->SizeInBytes = align(info->SizeInBytes, info->Alignment); TRACE("Size %#"PRIx64", alignment %#"PRIx64".\n", info->SizeInBytes, info->Alignment); @@ -2987,13 +3000,16 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour return info; invalid: + WARN("Invalid resource alignment %#"PRIx64" (required %#x).\n", + requested_alignment, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT); + +fail: info->SizeInBytes = ~(uint64_t)0; - /* FIXME: Should we support D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT for small MSSA resources? */ - if (desc->SampleDesc.Count != 1) - info->Alignment = D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT; - else - info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; + /* Theoretically we should return D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT if + * (desc->SampleDesc.Count != 1 && !device->feature_options4.MSAA64KBAlignedTextureSupported) + * but Vulkan has no such requirement and it may prove unnecessary. */ + info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; TRACE("Alignment %#"PRIx64".\n", info->Alignment); -- 2.24.0
On Mon, 25 Nov 2019 at 10:30, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
+ if (requested_alignment != D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) + goto invalid; Should this be in d3d12_resource_validate_desc() instead? Likewise for images.
+ /* Validate the alignment in the resource description. Also allow the Vulkan alignment in case the caller + * specifies it in future calls. Did you observe that happening, or is this speculative?
+ estimated_size = vkd3d_format_is_compressed(format) + ? desc->Width * desc->Height * format->block_byte_count / (format->block_width * format->block_height) + : desc->Width * desc->Height * format->byte_count; That's not how size calculations for block-based formats work. See e.g. vkd3d_format_copy_data(). It also seems this is a separate change.
- if (desc->SampleDesc.Count != 1) - info->Alignment = D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT; - else - info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; + /* Theoretically we should return D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT if + * (desc->SampleDesc.Count != 1 && !device->feature_options4.MSAA64KBAlignedTextureSupported) + * but Vulkan has no such requirement and it may prove unnecessary. */ + info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT;
Separate change?
November 26, 2019 6:07 AM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
On Mon, 25 Nov 2019 at 10:30, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
+ if (requested_alignment != D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT) + goto invalid;
Should this be in d3d12_resource_validate_desc() instead? Likewise for images.
+ /* Validate the alignment in the resource description. Also allow the Vulkan alignment in case the caller + * specifies it in future calls.
Did you observe that happening, or is this speculative?
Speculative, so I can remove it, which would enable complete validation to occur in d3d12_resource_validate_desc() instead. Dimensions of block compressed textures are currently not validated either, nor are some combinations of flags with other options.
+ estimated_size = vkd3d_format_is_compressed(format) + ? desc->Width * desc->Height * format->block_byte_count / (format->block_width * format->block_height) + : desc->Width * desc->Height * format->byte_count;
That's not how size calculations for block-based formats work. See e.g. vkd3d_format_copy_data(). It also seems this is a separate change.
How about this: estimated_size = (desc->Height / format->block_height) * (desc->Width / format->block_width) * format->byte_count * format->block_byte_count; Conor
On Tue, 26 Nov 2019 at 15:04, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
November 26, 2019 6:07 AM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
On Mon, 25 Nov 2019 at 10:30, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
+ estimated_size = vkd3d_format_is_compressed(format) + ? desc->Width * desc->Height * format->block_byte_count / (format->block_width * format->block_height) + : desc->Width * desc->Height * format->byte_count;
That's not how size calculations for block-based formats work. See e.g. vkd3d_format_copy_data(). It also seems this is a separate change.
How about this: estimated_size = (desc->Height / format->block_height) * (desc->Width / format->block_width) * format->byte_count * format->block_byte_count;
That will result in a 0 estimated size for textures with any dimension smaller than the block size. That would be fine for e.g. a 1x1 texture since the total size would be smaller than D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT anyway, but note that that would also affect textures that are e.g. very wide but only 1 pixel tall.
November 26, 2019 10:24 PM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
On Tue, 26 Nov 2019 at 15:04, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
How about this: estimated_size = (desc->Height / format->block_height) * (desc->Width / format->block_width) * format->byte_count * format->block_byte_count;
That will result in a 0 estimated size for textures with any dimension smaller than the block size. That would be fine for e.g. a 1x1 texture since the total size would be smaller than D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT anyway, but note that that would also affect textures that are e.g. very wide but only 1 pixel tall.
When I was messing with this in Windows I think a resource was invalid if any dimension was not aligned with the block size in that dimension. I didn't check tiny dimensions, or if 1D textures are valid at all with a compressed format. I'll test all of these. But in any case the following should work. With dimension limits there's no chance of overflow. estimated_size = desc->Width * desc->Height * format->byte_count * format->block_byte_count / (format->block_width * format->block_height);
participants (2)
-
Conor McCarthy -
Henri Verbeet