Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX12] Readonly StorageTexture binding does not create UAV binding #574

Closed
Wumpf opened this issue Apr 12, 2020 · 5 comments
Closed

[DX12] Readonly StorageTexture binding does not create UAV binding #574

Wumpf opened this issue Apr 12, 2020 · 5 comments
Labels
type: bug Something isn't working

Comments

@Wumpf
Copy link
Member

Wumpf commented Apr 12, 2020

Minimal repro here:
Wumpf/wgpu-rs@a00e08c
Note that this builds on top of the fix from #572/#573 - haven't tried again, but I'm quite sure the particular case would work before the fix.

Steps:

  • Use a uniform readonly image2D image. Note that you actually need to use the image, otherwise DX validation layer won't complain (does not influence hlsl conversion though).
    This is translated in hlsl to a RWTexture2D image, so a UAV, not a SRV as one might expect!
  • Use then then the correct binding:
wgpu::BindingType::StorageTexture {
                dimension: wgpu::TextureViewDimension::D3,
                component_type: wgpu::TextureComponentType::Uint,
                format: wgpu::TextureFormat::R32Uint,
                readonly: true,
             }
  • The resulting root signature apparently doesn't have a UAV: DX12 validation layer gets rather unhappy:
D3D12 ERROR: ID3D12Device::CreateGraphicsPipelineState: Root Signature doesn't match Vertex Shader: Shader UAV descriptor range (BaseShaderRegister=0, NumDescriptors=1, RegisterSpace=0) is not fully bound in root signature [ STATE_CREATION ERROR #688: CREATEGRAPHICSPIPELINESTATE_VS_ROOT_SIGNATURE_MISMATCH]

Without diving into wgpu/gfx-hal it looks like either a UAV binding is simply missing or the assumption was that the HLSL shader wouldn't use a UAV to begin with? Arguably it would have made more sense if it would convert everything to a SRV and changes the respective loads to texture.Load

Full converted hlsl shader code from the example repro for reference:

   RWTexture2D<float4> testimage : register(u0, space0);

    static float4 gl_Position;
    static int gl_VertexIndex;
    struct SPIRV_Cross_Input
    {
        uint gl_VertexIndex : SV_VertexID;
    };

    struct SPIRV_Cross_Output
    {
        float4 gl_Position : SV_Position;
    };

    void vert_main()
    {
        float2 position = float2(float(gl_VertexIndex), float((gl_VertexIndex & 1) * 2)) - 1.0f.xx;
        gl_Position = float4(position, 0.0f, 1.0f) + testimage[int2(0, 0)];
    }

    SPIRV_Cross_Output main(SPIRV_Cross_Input stage_input)
    {
        gl_VertexIndex = int(stage_input.gl_VertexIndex);
        vert_main();
        SPIRV_Cross_Output stage_output;
        stage_output.gl_Position = gl_Position;
        return stage_output;
    }
@grovesNL
Copy link
Collaborator

Probably related to KhronosGroup/SPIRV-Cross#1306, we might just need to update spirv_cross to use this flag

@kvark
Copy link
Member

kvark commented Apr 12, 2020

Oh snap, I thought this is already the way it works. I recall working on it in gfx-rs/gfx#3176 and testing on gfx-ocean, which uses a bunch of read-only descriptors now 🤔

@kvark kvark added the type: bug Something isn't working label Apr 12, 2020
@cwfitzgerald
Copy link
Member

Is this still an issue on master?

@cwfitzgerald
Copy link
Member

Looking at your repro closely it looks like you are creating a bind group that doesn't match the bind group layout (layout has one binding, bind group has none) so I'm going to call this closed for now, if you have an updated example that still has this problem, feel free to reopen!

@Wumpf
Copy link
Member Author

Wumpf commented Dec 21, 2020

apologies for not following up on this one. Thanks for digging in and pointing out that mistake!

kvark pushed a commit to kvark/wgpu that referenced this issue Jun 3, 2021
574: Implement AsRef for BufferView r=kvark a=de-vri-es

This PR implements `AsRef` and `AsMut` in addition to `Deref` and `DerefMut` for `BufferView` and `BufferViewMut`.

This allows the buffer views to be used directly by generic code that wants an `AsRef<[u8]>`.

It's also subjectively a small win when you want to pass the views to non-generic code. I find `buffer.as_ref()` clearer than `&*buffer`. That also goes for `buffer.deref()`, but `Deref` is not in the prelude.

Co-authored-by: Maarten de Vries <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants