tests: Add UAV format-specific require directives and cap checks.
Supersedes !966 (closed), now requiring and checking support for individual UAV formats as suggested.
Marked as a Draft because I am not totally sure that I am checking for the support correctly on 5/6 for all of the pertinent backends, but it works on my machine:tm: (Intel UHD Graphics 770) as expected, the supported/unsupported UAV formats are consistent across them.
Merge request reports
Activity
mentioned in merge request !966 (closed)
372 374 runner->compile_options |= options[i].option; 373 375 } 374 376 } 377 else if (match_string(line, "uav-formats:", &line)) I think Henri was suggesting a syntax that would allow for many different capabilities to be queried (buffer, 1D texture, 2D texture, UAV load, UAV store, etc). I don't think we have to support all capabilities immediately, but at least have a syntax that makes it easy to add more in the future. I was proposing something like:
format r32-uint uav-load uav-store
This means that it's probably unfeasible to check that each test declares all the format capabilities it needs, but at the same time it's not that important either. As long as tests are working we don't care too much, and as soon as a test is failing a developer will add the missing capabilities as needed.
Also, being generic on both the format and the capability means that using
TypedUAVLoadAdditionalFormats
probably becomes more complicated than just explicitly querying each format individually.changed this line in version 2 of the diff
I think Henri was suggesting a syntax that would allow for many different capabilities to be queried (buffer, 1D texture, 2D texture, UAV load, UAV store, etc). I don't think we have to support all capabilities immediately, but at least have a syntax that makes it easy to add more in the future. I was proposing something like:
format r32-uint uav-load uav-store
I see, I implemented a syntax and added a table within the runner caps and requirements structs that allows for this (albeit, for now the only capability introduced is
uav-load
).This means that it's probably unfeasible to check that each test declares all the format capabilities it needs, but at the same time it's not that important either. As long as tests are working we don't care too much, and as soon as a test is failing a developer will add the missing capabilities as needed.
I understand. My previous version mistakenly assumed that if a UAV was declared with a specific format, then at least UAV loads for that format would be required. This was a mistake as many tests only required stores and not loads. I removed that patch and many uav-load requirements that weren't really needed by the tests.
Also, being generic on both the format and the capability means that using
TypedUAVLoadAdditionalFormats
probably becomes more complicated than just explicitly querying each format individually.To my knowledge, in d3d11 and d3d12 these formats are queried in a group and I am not aware of a method to query them specifically, unlike GL or Vulkan. But I might be missing it.
I think Henri was suggesting a syntax that would allow for many different capabilities to be queried (buffer, 1D texture, 2D texture, UAV load, UAV store, etc). I don't think we have to support all capabilities immediately, but at least have a syntax that makes it easy to add more in the future. I was proposing something like:
format r32-uint uav-load uav-store
This means that it's probably unfeasible to check that each test declares all the format capabilities it needs, but at the same time it's not that important either. As long as tests are working we don't care too much, and as soon as a test is failing a developer will add the missing capabilities as needed.
Also, being generic on both the format and the capability means that using
TypedUAVLoadAdditionalFormats
probably becomes more complicated than just explicitly querying each format individually.Pretty much.
-echo "${col}${res}${color_reset}: $test_name $details" +echo "${col}${res}${color_reset}: $test_name""$details"
Doesn't that cause us to then miss a space when the test fails? I suppose we could either explicitly add it in that path, or remove it in the success path using something like
details=${details# }
, and keep this line the way it currently is.Doesn't that cause us to then miss a space when the test fails? I suppose we could either explicitly add it in that path, or remove it in the success path using something like
details=${details# }
, and keep this line the way it currently is.hmm, I don't think it does. This is how a failing test is shown:
When
$details
is not overwritten by the short, inline version, it always stars with a\n
.Before this patch a trailing white-space is printed before the
\n
, so it is also good to get rid of it.
I implemented the suggested format. Got rid of many unnecessary uav-load requirements from the previous version.
Also, I realized that it is probably better not to skip the compilation part of the tests when there are missing capabilities and only the execution (the
[test]
block), so I introduced 4/6.When
$details
is not overwritten by the short, inline version, it always stars with a\n
.Before this patch a trailing white-space is printed before the
\n
, so it is also good to get rid of it.Ah right, indeed.
Also, being generic on both the format and the capability means that using
TypedUAVLoadAdditionalFormats
probably becomes more complicated than just explicitly querying each format individually.To my knowledge, in d3d11 and d3d12 these formats are queried in a group and I am not aware of a method to query them specifically, unlike GL or Vulkan. But I might be missing it.
I think ID3D12Device_CheckFeatureSupport() with D3D12_FEATURE_FORMAT_SUPPORT is still supposed to report accurate information for these formats. I see you found D3D12_FEATURE_FORMAT_SUPPORT, but you're not using it quite right. You're supposed to set the .Format field of the "format_support" structure before calling CheckFeatureSupport(). I.e.:
static bool format_support_uav_load(ID3D12Device *device, DXGI_FORMAT format) { D3D12_FEATURE_DATA_FORMAT_SUPPORT format_support = {.Format = format}; HRESULT hr; hr = ID3D12Device_CheckFeatureSupport(device, D3D12_FEATURE_FORMAT_SUPPORT, &format_support, sizeof(format_support)); ok(hr == S_OK, "Failed to query format support, hr %#x.\n", hr); return format_support.Support2 & D3D12_FORMAT_SUPPORT2_UAV_TYPED_LOAD; }
The d3d11 version works roughly the same way, but with slightly different field names and so on.
I think ID3D12Device_CheckFeatureSupport() with D3D12_FEATURE_FORMAT_SUPPORT is still supposed to report accurate information for these formats. I see you found D3D12_FEATURE_FORMAT_SUPPORT, but you're not using it quite right. You're supposed to set the .Format field of the "format_support" structure before calling CheckFeatureSupport().
I understand, thanks. In the new version the formats are checked individually for shader_runner_d3d11.c and shader_runner_d3d12.c.
I had to remove the capability checks for DXGI_FORMAT_UNKNOWN since this makes both CheckFeatureSupport() functions to return
E_FAIL
on Windows with native d3dcompiler47.dll, which is the reason why the Windows pipeline crashed when I didn't initialize the.Format
field.+ else if (match_string(line, "format", &line)) + { + DXGI_FORMAT format = parse_format(line, NULL, NULL, NULL, &line); + + while (line[0] != '\0') + { + if (match_string(line, "uav-load", &line)) + runner->require_format_caps[FORMAT_CAP_UAV_LOAD][format] = true; + else + fatal_error("Unknown format cap '%s'.\n", line); + } + }
We should reset these again when we encounter a new [require] block, right?
+ for (k = 0; k <= FORMAT_CAP_LAST; ++k) + { + for (i = 0; i <= DXGI_FORMAT_LAST; ++i) + { + if (runner->require_format_caps[k][i] && !caps->format_caps[k][i]) + return false; + } + }
We'd generally use _COUNT constants instead of _LAST constants for this kind of thing, but we may also be able to use "ARRAY_SIZE(runner->require_format_caps)" and "ARRAY_SIZE(*runner->require_format_caps)" instead here.
+ const char *format_cap_names[] = + { + [FORMAT_CAP_UAV_LOAD] = "uav-load", + };
"const char * const format_cap_names[] ="
@@ -175,6 +185,8 @@ struct shader_runner bool require_rov; bool require_wave_ops; + bool require_format_caps[FORMAT_CAP_LAST + 1][DXGI_FORMAT_LAST + 1]; + bool last_render_failed; uint32_t *uniforms;
I don't mind too much, but it seems slightly nicer to use a bitmap/flags instead of a bool array.
+ /* D3D12_FORMAT_SUPPORT2_UAV_TYPED_LOAD is equivalent to D3D11_FORMAT_SUPPORT2_UAV_TYPED_LOAD */ + return format_support2.OutFormatSupport2 & D3D12_FORMAT_SUPPORT2_UAV_TYPED_LOAD;
Sure, but I don't think that's by itself a reason to avoid D3D11_FORMAT_SUPPORT2_UAV_TYPED_LOAD.
+ runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32_FLOAT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32_FLOAT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32_SINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32G32B32A32_FLOAT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32G32B32A32_FLOAT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32G32B32A32_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32G32B32A32_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R32G32B32A32_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R32G32B32A32_SINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16G16B16A16_FLOAT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16G16B16A16_FLOAT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16G16B16A16_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16G16B16A16_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16G16B16A16_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16G16B16A16_SINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8G8B8A8_UNORM] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8G8B8A8_UNORM); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8G8B8A8_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8G8B8A8_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8G8B8A8_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8G8B8A8_SINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16_FLOAT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16_FLOAT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R16_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R16_SINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8_UNORM] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8_UNORM); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8_UINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8_UINT); + runner->caps.format_caps[FORMAT_CAP_UAV_LOAD][DXGI_FORMAT_R8_SINT] + = check_uav_load_format_support(runner->device, DXGI_FORMAT_R8_SINT);
Just iterate over an array of formats to test, IMO.
- "GL_ARB_internalformat_query", + "GL_ARB_internalformat_query2",
I suppose it doesn't make much of a difference in practice, but I don't think we need to require this extension. If it's unavailable, we can just report the corresponding capabilities as unavailable.
We should reset these again when we encounter a new [require] block, right?
True, I missed that.
We'd generally use _COUNT constants instead of _LAST constants for this kind of thing, but we may also be able to use "ARRAY_SIZE(runner->require_format_caps)" and "ARRAY_SIZE(*runner->require_format_caps)" instead here.
Okay, the new version just requires
#define DXGI_FORMAT_COUNT (DXGI_FORMAT_B4G4R4A4_UNORM + 1)
for array sizes.
"const char * const format_cap_names[] ="
I see, the array is no longer used in the new version though.
I don't mind too much, but it seems slightly nicer to use a bitmap/flags instead of a bool array.
I tried this and I think I agree, the new implementation looks nicer.
Sure, but I don't think that's by itself a reason to avoid D3D11_FORMAT_SUPPORT2_UAV_TYPED_LOAD.
I see, changed.
Just iterate over an array of formats to test, IMO.
Given that the new implementation uses bitmasks instead of bools, these lines are briefer now, so it might not be as necessary.
Also, the Vulkan runner requires to manually map one VK_FORMAT to a DXGI_FORMAT, and so does the OpenGL runner from GL_ formats, so a list is not so useful for these, and I think that the consistency between the backends is valuable.
But if you want I can change it to a list, in that case it perhaps would make sense to expose
shader_runner.c:parse_format()
and add the equivalent VK_FORMAT and GL formats to the entries, I am not so sure.I suppose it doesn't make much of a difference in practice, but I don't think we need to require this extension. If it's unavailable, we can just report the corresponding capabilities as unavailable.
Done.
added 40 commits
-
5775e4b7...c8cc1b1a - 34 commits from branch
wine:master
- 60117aaf - tests/shader-runner: Always expect format for UAV types.
- 85d96f13 - tests/shader-runner: Move parse_format() up.
- a494dc50 - tests: Replace spaces with dash on format names.
- 3b592617 - tests/shader-runner: Don't skip compilation on missing caps.
- c7b0de19 - tests/shader-runner: Explicitly require UAV load types.
- 624a6140 - tests/test-driver: Avoid double space when printing details.
Toggle commit list-
5775e4b7...c8cc1b1a - 34 commits from branch
added 16 commits
-
ac845d5e...738ecc9e - 10 commits from branch
wine:master
- 164076d1 - tests/shader_runner: Require explicit formats for UAV resources.
- cec62c54 - tests/shader_runner: Move parse_format() up.
- 19c23ca6 - tests/shader_runner: Replace spaces with dashes in format names.
- fb153bf5 - tests/shader_runner: Don't skip shader compilation on missing caps.
- b701f8d3 - tests/shader_runner: Explicitly require UAV load support.
- f5ed0d9e - tests/test-driver: Avoid double space when printing details.
Toggle commit list-
ac845d5e...738ecc9e - 10 commits from branch