Skip to content
Snippets Groups Projects

vkd3d-utils: D3DReflect() and reflection tests, part 1/2.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:mr1 into master

Unlike previous vkd3d-utils interfaces, ID3D12ShaderReflection is rather more large and complex, and will probably end up needing several new individual scan interfaces from vkd3d-shader, which are themselves not exactly trivial to design.

Therefore, instead of implementing everything in vkd3d-shader and then hooking up the vkd3d-utils interfaces on top of that, this patch series copies the existing implementation of reflection and then begins the process of moving its implementation to vkd3d-shader.

The primary motivation here is to add reflection crosstests (primarily for the benefit of the HLSL compiler) without being blocked on API design. Part 2 of this patch series does this.

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
  • Unlike previous vkd3d-utils interfaces, ID3D12ShaderReflection is rather more large and complex, and will probably end up needing several new individual scan interfaces from vkd3d-shader, which are themselves not exactly trivial to design.

    Therefore, instead of implementing everything in vkd3d-shader and then hooking up the vkd3d-utils interfaces on top of that, this patch series copies the existing implementation of reflection and then begins the process of moving its implementation to vkd3d-shader.

    The primary motivation here is to add reflection crosstests (primarily for the benefit of the HLSL compiler) without being blocked on API design. Part 2 of this patch series does this.

    Are we going to need much more than input and output signatures from D3DReflect()? If not, it seems a fair bit easier to just add some helpers which use either vkd3d_shader_scan() or D3DReflect() for that to d3d12_crosstest.h.

    Also, note that this currently fails on the CI.

  • Author Developer

    Are we going to need much more than input and output signatures from D3DReflect()? If not, it seems a fair bit easier to just add some helpers which use either vkd3d_shader_scan() or D3DReflect() for that to d3d12_crosstest.h.

    I would like to be able to test signatures, constant buffers, and resources. (That's most of ID3D12ShaderReflection; the other part is basically the STAT chunk, which we currently don't even output.) Signatures is vkd3d_shader_scan_signature_info; resources should be vkd3d_shader_scan_descriptor_info1 (plus I think a couple of extra patches I have). The hard part is constant buffers; I've basically tried to design the interface several times and always been put off by how complex it gets. Still, perhaps the best thing to do is figure it out now anyway...

  • Elizabeth Figura added 5 commits

    added 5 commits

    • a5649f45 - vkd3d-utils: Add D3DReflect().
    • 1a68c428 - vkd3d-utils: Strip d3dcompiler_ prefix from reflection definitions.
    • 611007e5 - vkd3d-utils: Use vkd3d_shader_scan() to retrieve signature information.
    • 137b5501 - tests: Test VKD3D_SHADER_COMPILE_OPTION_BACKCOMPAT_MAP_SEMANTIC_NAMES.
    • 051ee292 - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • In case it helps, I don't think we necessarily need a complete ID3D11ShaderReflection and ID3D12ShaderReflection implementation right from the start; that's not what we do for libvkd3d d3d12 interfaces either.

  • Elizabeth Figura added 50 commits

    added 50 commits

    • 051ee292...cd77b2a9 - 45 commits from branch wine:master
    • e2dbe579 - include: Add vkd3d_d3d12shader.idl.
    • 4d32fdb9 - vkd3d-utils: Add D3DReflect().
    • a792f6d9 - vkd3d-utils: Implement input and output signature reflection.
    • 5a8685cb - tests: Test VKD3D_SHADER_COMPILE_OPTION_BACKCOMPAT_MAP_SEMANTIC_NAMES.
    • f699335e - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • Author Developer

    Alright, I've redone the relevant parts of this series implementing reflection from scratch. I'll try to design a reasonable API for constant buffers.

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Giovanni Mascellani unapproved this merge request

    unapproved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Approved, as in "I don't have strong opinions on this, but the code you wrote doesn't look particularly evil". The CI pipeline fails in seemingly minor ways, though.

  • added 1 commit

    • 3e0ad2b9 - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • Elizabeth Figura added 4 commits

    added 4 commits

    • 3108cb29 - vkd3d-utils: Add D3DReflect().
    • 96314a6d - vkd3d-utils: Implement input and output signature reflection.
    • f5fd978b - tests: Test VKD3D_SHADER_COMPILE_OPTION_BACKCOMPAT_MAP_SEMANTIC_NAMES.
    • f1fa2278 - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • Author Developer

    Build warnings should be finally fixed now.

  • diff --git a/include/vkd3d_utils.h b/include/vkd3d_utils.h
    index 686ddf386..845894c22 100644
    --- a/include/vkd3d_utils.h
    +++ b/include/vkd3d_utils.h
    @@ -86,6 +86,7 @@ VKD3D_UTILS_API HRESULT WINAPI D3DCreateBlob(SIZE_T data_size, ID3DBlob **blob);
     VKD3D_UTILS_API HRESULT WINAPI D3DPreprocess(const void *data, SIZE_T size, const char *filename,
             const D3D_SHADER_MACRO *defines, ID3DInclude *include,
             ID3DBlob **shader, ID3DBlob **error_messages);
    +VKD3D_UTILS_API HRESULT WINAPI D3DReflect(const void *data, SIZE_T data_size, REFIID iid, void **reflection);
     
     /**
      * Set a callback to be called when vkd3d-utils outputs debug logging.

    We'd want a \since 1.11 on that, right? (And move it after D3DStripShader(), if we're going to stick with ordering things by the version they were introduced.)

    diff --git a/libs/vkd3d-common/blob.c b/libs/vkd3d-common/blob.c
    index 0f6d5a5ee..59e9834d4 100644
    --- a/libs/vkd3d-common/blob.c
    +++ b/libs/vkd3d-common/blob.c
    @@ -23,6 +23,7 @@
     #include "vkd3d_blob.h"
     #include "vkd3d_debug.h"
     #include "vkd3d_memory.h"
    +#include "vkd3d_d3d12shader.h"
     
     struct vkd3d_blob
     {

    Why do we need that?

    +static ULONG STDMETHODCALLTYPE d3d12_reflection_AddRef(ID3D12ShaderReflection *iface)
    +{
    +    struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface);
    +    ULONG refcount = InterlockedIncrement(&reflection->refcount);
    +
    +    TRACE("%p increasing refcount to %u.\n", reflection, refcount);
    +
    +    return refcount;
    +}
    +
    +static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection *iface)
    +{
    +    struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface);
    +    ULONG refcount = InterlockedDecrement(&reflection->refcount);
    +
    +    TRACE("%p decreasing refcount to %u.\n", reflection, refcount);
    +
    +    if (!refcount)
    +    {
    +        free(reflection);
    +    }
    +
    +    return refcount;
    +}

    We have vkd3d_atomic_increment_u32()/vkd3d_atomic_decrement_u32() now, see e.g. vkd3d_blob_AddRef()/vkd3d_blob_Release().

    +static void check_signature_element(const D3D12_SIGNATURE_PARAMETER_DESC *desc,
    +        const D3D12_SIGNATURE_PARAMETER_DESC *expect)
    +{
    +    ok(!strcmp(desc->SemanticName, expect->SemanticName), "Got name \"%s\".\n", desc->SemanticName);
    +    ok(desc->SemanticIndex == expect->SemanticIndex, "Got index %u.\n", desc->SemanticIndex);
    +    ok(desc->Register == expect->Register, "Got register %u.\n", desc->Register);
    +    ok(desc->SystemValueType == expect->SystemValueType, "Got sysval %u.\n", desc->SystemValueType);
    +    ok(desc->ComponentType == expect->ComponentType, "Got data type %u.\n", desc->ComponentType);
    +    ok(desc->Mask == expect->Mask, "Got mask %#x.\n", desc->Mask);
    +    todo_if(desc->ReadWriteMask != expect->ReadWriteMask)
    +        ok(desc->ReadWriteMask == expect->ReadWriteMask, "Got used mask %#x.\n", desc->ReadWriteMask);
    +    ok(desc->Stream == expect->Stream, "Got stream %u.\n", desc->Stream);
    +}

    I imagine this is borrowed from something like d3dcompiler's check_parameter_desc() in Wine, but it's unfortunate to lose the original line numbers for ok(). See e.g. check_heap_desc() for how we usually implement that kind of helper in vkd3d.

  • Author Developer
    diff --git a/include/vkd3d_utils.h b/include/vkd3d_utils.h
    index 686ddf386..845894c22 100644
    --- a/include/vkd3d_utils.h
    +++ b/include/vkd3d_utils.h
    @@ -86,6 +86,7 @@ VKD3D_UTILS_API HRESULT WINAPI D3DCreateBlob(SIZE_T data_size, ID3DBlob **blob);
     VKD3D_UTILS_API HRESULT WINAPI D3DPreprocess(const void *data, SIZE_T size, const char *filename,
             const D3D_SHADER_MACRO *defines, ID3DInclude *include,
             ID3DBlob **shader, ID3DBlob **error_messages);
    +VKD3D_UTILS_API HRESULT WINAPI D3DReflect(const void *data, SIZE_T data_size, REFIID iid, void **reflection);
     
     /**
      * Set a callback to be called when vkd3d-utils outputs debug logging.

    We'd want a \since 1.11 on that, right? (And move it after D3DStripShader(), if we're going to stick with ordering things by the version they were introduced.)

    Fixed in v6.

    diff --git a/libs/vkd3d-common/blob.c b/libs/vkd3d-common/blob.c
    index 0f6d5a5ee..59e9834d4 100644
    --- a/libs/vkd3d-common/blob.c
    +++ b/libs/vkd3d-common/blob.c
    @@ -23,6 +23,7 @@
     #include "vkd3d_blob.h"
     #include "vkd3d_debug.h"
     #include "vkd3d_memory.h"
    +#include "vkd3d_d3d12shader.h"
     
     struct vkd3d_blob
     {

    Why do we need that?

    Because of GUID definition :-/

    +static ULONG STDMETHODCALLTYPE d3d12_reflection_AddRef(ID3D12ShaderReflection *iface)
    +{
    +    struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface);
    +    ULONG refcount = InterlockedIncrement(&reflection->refcount);
    +
    +    TRACE("%p increasing refcount to %u.\n", reflection, refcount);
    +
    +    return refcount;
    +}
    +
    +static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection *iface)
    +{
    +    struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface);
    +    ULONG refcount = InterlockedDecrement(&reflection->refcount);
    +
    +    TRACE("%p decreasing refcount to %u.\n", reflection, refcount);
    +
    +    if (!refcount)
    +    {
    +        free(reflection);
    +    }
    +
    +    return refcount;
    +}

    We have vkd3d_atomic_increment_u32()/vkd3d_atomic_decrement_u32() now, see e.g. vkd3d_blob_AddRef()/vkd3d_blob_Release().

    Fixed in v6.

    +static void check_signature_element(const D3D12_SIGNATURE_PARAMETER_DESC *desc,
    +        const D3D12_SIGNATURE_PARAMETER_DESC *expect)
    +{
    +    ok(!strcmp(desc->SemanticName, expect->SemanticName), "Got name \"%s\".\n", desc->SemanticName);
    +    ok(desc->SemanticIndex == expect->SemanticIndex, "Got index %u.\n", desc->SemanticIndex);
    +    ok(desc->Register == expect->Register, "Got register %u.\n", desc->Register);
    +    ok(desc->SystemValueType == expect->SystemValueType, "Got sysval %u.\n", desc->SystemValueType);
    +    ok(desc->ComponentType == expect->ComponentType, "Got data type %u.\n", desc->ComponentType);
    +    ok(desc->Mask == expect->Mask, "Got mask %#x.\n", desc->Mask);
    +    todo_if(desc->ReadWriteMask != expect->ReadWriteMask)
    +        ok(desc->ReadWriteMask == expect->ReadWriteMask, "Got used mask %#x.\n", desc->ReadWriteMask);
    +    ok(desc->Stream == expect->Stream, "Got stream %u.\n", desc->Stream);
    +}

    I imagine this is borrowed from something like d3dcompiler's check_parameter_desc() in Wine, but it's unfortunate to lose the original line numbers for ok(). See e.g. check_heap_desc() for how we usually implement that kind of helper in vkd3d.

    I omitted them because the test contexts provide all the necessary information. I can restore them if preferred though.

  • Elizabeth Figura added 35 commits

    added 35 commits

    • f1fa2278...14da4df9 - 30 commits from branch wine:master
    • ed9b92f4 - include: Add vkd3d_d3d12shader.idl.
    • c86f46ef - vkd3d-utils: Add D3DReflect().
    • 2c675dd7 - vkd3d-utils: Implement input and output signature reflection.
    • aaa6b751 - tests: Test VKD3D_SHADER_COMPILE_OPTION_BACKCOMPAT_MAP_SEMANTIC_NAMES.
    • 7f64952f - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • I omitted them because the test contexts provide all the necessary information.

    That's only true because there are currently only two places calling this though; it doesn't seem like something that would scale well.

  • added 1 commit

    • 1dca120b - tests: Test signature reflection via D3DReflect().

    Compare with previous version

  • Author Developer

    I omitted them because the test contexts provide all the necessary information.

    That's only true because there are currently only two places calling this though; it doesn't seem like something that would scale well.

    I suppose I don't see a need for it to scale well. If we have more tests for signature elements they can be added to the same function.

    And, well, in general I think putting that information in the test context is just nicer than trying to manipulate the line number. I've made the change anyway here because I don't really want to hold up this bikeshedding (or argue with the maintainer), but I would like to express a preference to use test contexts in lieu of line number manipulation in cases like this.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading