Skip to content
Snippets Groups Projects

tests: Add UAV format-specific require directives and cap checks.

Merged Francisco Casas requested to merge fcasas/vkd3d:waveops-intel-fix2 into master
4 unresolved threads

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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.

  • Francisco Casas changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Author Developer

    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.

  • Please register or sign in to reply
    • 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.

    • Author Developer

      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:

      image

      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.

    • Please register or sign in to reply
  • Francisco Casas added 3 commits

    added 3 commits

    • 815bbbb0 - tests/shader-runner: Don't skip compilation on missing caps.
    • ea113f5a - tests/shader-runner: Explicitly require UAV load types.
    • b832326a - tests/test-driver: Avoid double space when printing details.

    Compare with previous version

  • Author Developer

    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.

  • Francisco Casas added 2 commits

    added 2 commits

    • 2c76bdba - tests/shader-runner: Explicitly require UAV load types.
    • a6ad857b - tests/test-driver: Avoid double space when printing details.

    Compare with previous version

  • Author Developer

    Fixed a missing runner->device instead of just device. I am not sure how this was only an error on the build-crosstest job and not when compiling on Linux.

  • Francisco Casas marked this merge request as ready

    marked this merge request as ready

  • 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.

  • Francisco Casas added 2 commits

    added 2 commits

    • 630a4cd7 - tests/shader-runner: Explicitly require UAV load types.
    • 5775e4b7 - tests/test-driver: Avoid double space when printing details.

    Compare with previous version

  • Author Developer

    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.

    • Author Developer

      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.

    • Please register or sign in to reply
  • Francisco Casas added 40 commits

    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.

    Compare with previous version

  • Francisco Casas added 2 commits

    added 2 commits

    • 9d385f91 - tests/shader-runner: Explicitly require UAV load types.
    • bed4386f - tests/test-driver: Avoid double space when printing details.

    Compare with previous version

  • Author Developer

    I forgot to remove some obsolete paragraphs from the commit message.

  • Francisco Casas added 2 commits

    added 2 commits

    • f4112f6d - tests/shader-runner: Explicitly require UAV load types.
    • ac845d5e - tests/test-driver: Avoid double space when printing details.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 16 commits

    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.

    Compare with previous version

  • Please register or sign in to reply
    Loading