From 04dde7a3f2d5633e1bad159e35e45c90238894ae Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Thu, 30 Jan 2025 22:03:21 +0100 Subject: [PATCH] Add warnings on incorrect `GPUBuffer` or `GPUTexture` usage when binding to `GPUContext` (in non-release builds) --- Source/Engine/Graphics/GPUContext.cpp | 47 +++++++++++++++++++ Source/Engine/Graphics/GPUContext.h | 8 +++- Source/Engine/Graphics/GPUResource.h | 9 ++++ Source/Engine/Graphics/Textures/GPUTexture.h | 9 ---- .../DirectX/DX11/GPUBufferDX11.cpp | 1 + .../DirectX/DX11/GPUBufferDX11.h | 5 ++ .../DirectX/DX11/GPUContextDX11.cpp | 8 ++++ .../DirectX/DX11/IShaderResourceDX11.h | 3 -- .../DirectX/DX12/DescriptorHeapDX12.cpp | 12 ----- .../DirectX/DX12/DescriptorHeapDX12.h | 11 ++++- .../DirectX/DX12/GPUBufferDX12.cpp | 2 +- .../DirectX/DX12/GPUBufferDX12.h | 4 +- .../DirectX/DX12/GPUContextDX12.cpp | 10 +++- .../GraphicsDevice/Vulkan/GPUBufferVulkan.cpp | 1 + .../GraphicsDevice/Vulkan/GPUBufferVulkan.h | 4 ++ .../Vulkan/GPUContextVulkan.cpp | 8 ++++ .../GraphicsDevice/Vulkan/GPUDeviceVulkan.h | 6 +++ .../GraphicsDevice/Vulkan/GPUTextureVulkan.h | 4 ++ 18 files changed, 121 insertions(+), 31 deletions(-) diff --git a/Source/Engine/Graphics/GPUContext.cpp b/Source/Engine/Graphics/GPUContext.cpp index e35c6cb1f..a3180f2cb 100644 --- a/Source/Engine/Graphics/GPUContext.cpp +++ b/Source/Engine/Graphics/GPUContext.cpp @@ -11,6 +11,53 @@ GPUContext::GPUContext(GPUDevice* device) { } +#if !BUILD_RELEASE + +#include "Engine/Core/Log.h" + +void GPUContext::LogInvalidResourceUsage(int32 slot, const GPUResourceView* view, InvalidBindPoint bindPoint) +{ + GPUResource* resource = view ? view->GetParent() : nullptr; + const Char* resourceType = TEXT("resource"); + const Char* flagType = TEXT("flags"); + if (resource) + { + switch (resource->GetResourceType()) + { + case GPUResourceType::RenderTarget: + case GPUResourceType::Texture: + case GPUResourceType::CubeTexture: + case GPUResourceType::VolumeTexture: + resourceType = TEXT("texture"); + flagType = TEXT("GPUTextureFlags"); + break; + case GPUResourceType::Buffer: + resourceType = TEXT("buffer"); + flagType = TEXT("GPUBufferFlags"); + break; + } + } + const Char* usage = TEXT("-"); + switch (bindPoint) + { + case InvalidBindPoint::SRV: + usage = TEXT("shader resource"); + break; + case InvalidBindPoint::UAV: + usage = TEXT("unordered access"); + break; + case InvalidBindPoint::DSV: + usage = TEXT("depth stencil"); + break; + case InvalidBindPoint::RTV: + usage = TEXT("render target"); + break; + } + LOG(Error, "Incorrect {} bind at slot {} as {} (ensure to setup correct {} when creating that resource)", resourceType, slot, usage, flagType); +} + +#endif + void GPUContext::FrameBegin() { _lastRenderTime = Platform::GetTimeSeconds(); diff --git a/Source/Engine/Graphics/GPUContext.h b/Source/Engine/Graphics/GPUContext.h index 52ff5ef5d..c38faee34 100644 --- a/Source/Engine/Graphics/GPUContext.h +++ b/Source/Engine/Graphics/GPUContext.h @@ -121,6 +121,12 @@ protected: double _lastRenderTime = -1; GPUContext(GPUDevice* device); +#if !BUILD_RELEASE + enum class InvalidBindPoint { SRV, UAV, DSV, RTV }; + + static void LogInvalidResourceUsage(int32 slot, const GPUResourceView* view, InvalidBindPoint bindPoint); +#endif + public: /// /// Gets the graphics device. @@ -143,7 +149,6 @@ public: public: #if GPU_ALLOW_PROFILE_EVENTS - /// /// Begins the profile event. /// @@ -158,7 +163,6 @@ public: virtual void EventEnd() { } - #endif public: diff --git a/Source/Engine/Graphics/GPUResource.h b/Source/Engine/Graphics/GPUResource.h index 2174f18bc..d98d6e1e6 100644 --- a/Source/Engine/Graphics/GPUResource.h +++ b/Source/Engine/Graphics/GPUResource.h @@ -190,6 +190,7 @@ API_CLASS(Abstract, NoSpawn, Attributes="HideInEditor") class FLAXENGINE_API GPU DECLARE_SCRIPTING_TYPE_NO_SPAWN(GPUResourceView); protected: static double DummyLastRenderTime; + GPUResource* _parent = nullptr; explicit GPUResourceView(const SpawnParams& params) : ScriptingObject(params) @@ -201,6 +202,14 @@ public: // Points to the cache used by the resource for the resource visibility/usage detection. Written during rendering when resource view is used. double* LastRenderTime; + /// + /// Gets parent GPU resource owning that view. + /// + API_PROPERTY() FORCE_INLINE GPUResource* GetParent() const + { + return _parent; + } + /// /// Gets the native pointer to the underlying view. It's a platform-specific handle. /// diff --git a/Source/Engine/Graphics/Textures/GPUTexture.h b/Source/Engine/Graphics/Textures/GPUTexture.h index 9aa6571e0..2e2072785 100644 --- a/Source/Engine/Graphics/Textures/GPUTexture.h +++ b/Source/Engine/Graphics/Textures/GPUTexture.h @@ -21,7 +21,6 @@ API_CLASS(Sealed, NoSpawn) class FLAXENGINE_API GPUTextureView : public GPUResou { DECLARE_SCRIPTING_TYPE_NO_SPAWN(GPUTextureView); protected: - GPUResource* _parent = nullptr; PixelFormat _format = PixelFormat::Unknown; MSAALevel _msaa = MSAALevel::None; @@ -40,14 +39,6 @@ protected: } public: - /// - /// Gets parent GPU resource owning that view. - /// - API_PROPERTY() FORCE_INLINE GPUResource* GetParent() const - { - return _parent; - } - /// /// Gets the view format. /// diff --git a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.cpp b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.cpp index 202407137..42e60de6d 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.cpp @@ -10,6 +10,7 @@ GPUBufferDX11::GPUBufferDX11(GPUDeviceDX11* device, const StringView& name) : GPUResourceDX11(device, name) { + _view.SetParnet(this); } GPUBufferView* GPUBufferDX11::View() const diff --git a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.h b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.h index c1b9133a9..3118a409b 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.h +++ b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUBufferDX11.h @@ -40,6 +40,11 @@ public: public: + void SetParnet(GPUBuffer* parent) + { + _parent = parent; + } + /// /// Release the view. /// diff --git a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUContextDX11.cpp b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUContextDX11.cpp index 0ff1392ec..e733b84fb 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUContextDX11.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUContextDX11.cpp @@ -355,7 +355,11 @@ void GPUContextDX11::BindCB(int32 slot, GPUConstantBuffer* cb) void GPUContextDX11::BindSR(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_SR_BINDED); + if (view && ((IShaderResourceDX11*)view->GetNativePtr())->SRV() == nullptr) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::SRV); +#endif auto handle = view ? ((IShaderResourceDX11*)view->GetNativePtr())->SRV() : nullptr; if (_srHandles[slot] != handle) { @@ -369,7 +373,11 @@ void GPUContextDX11::BindSR(int32 slot, GPUResourceView* view) void GPUContextDX11::BindUA(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_UA_BINDED); + if (view && ((IShaderResourceDX11*)view->GetNativePtr())->UAV() == nullptr) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::UAV); +#endif auto handle = view ? ((IShaderResourceDX11*)view->GetNativePtr())->UAV() : nullptr; if (_uaHandles[slot] != handle) { diff --git a/Source/Engine/GraphicsDevice/DirectX/DX11/IShaderResourceDX11.h b/Source/Engine/GraphicsDevice/DirectX/DX11/IShaderResourceDX11.h index c0a72a513..9b572707f 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX11/IShaderResourceDX11.h +++ b/Source/Engine/GraphicsDevice/DirectX/DX11/IShaderResourceDX11.h @@ -12,13 +12,11 @@ class IShaderResourceDX11 { public: - IShaderResourceDX11() { } public: - /// /// Gets handle to the shader resource view object. /// @@ -28,7 +26,6 @@ public: /// /// Gets CPU to the unordered access view object. /// - /// UAV virtual ID3D11UnorderedAccessView* UAV() const = 0; }; diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.cpp b/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.cpp index 931def028..1d14b88e4 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.cpp @@ -6,18 +6,6 @@ #include "GPUDeviceDX12.h" #include "Engine/GraphicsDevice/DirectX/RenderToolsDX.h" -D3D12_CPU_DESCRIPTOR_HANDLE DescriptorHeapWithSlotsDX12::Slot::CPU() const -{ - ASSERT_LOW_LAYER(Heap); - return Heap->CPU(Index); -} - -D3D12_GPU_DESCRIPTOR_HANDLE DescriptorHeapWithSlotsDX12::Slot::GPU() const -{ - ASSERT_LOW_LAYER(Heap); - return Heap->GPU(Index); -} - void DescriptorHeapWithSlotsDX12::Slot::CreateSRV(GPUDeviceDX12* device, ID3D12Resource* resource, D3D12_SHADER_RESOURCE_VIEW_DESC* desc) { if (Heap == nullptr) diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.h b/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.h index f80e38178..82bd87ce3 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.h +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/DescriptorHeapDX12.h @@ -35,8 +35,15 @@ public: } #endif - D3D12_CPU_DESCRIPTOR_HANDLE CPU() const; - D3D12_GPU_DESCRIPTOR_HANDLE GPU() const; + FORCE_INLINE D3D12_CPU_DESCRIPTOR_HANDLE CPU() const + { + return Heap ? Heap->CPU(Index) : D3D12_CPU_DESCRIPTOR_HANDLE {}; + } + + FORCE_INLINE D3D12_GPU_DESCRIPTOR_HANDLE GPU() const + { + return Heap ? Heap->GPU(Index) : D3D12_GPU_DESCRIPTOR_HANDLE {}; + } void CreateSRV(GPUDeviceDX12* device, ID3D12Resource* resource, D3D12_SHADER_RESOURCE_VIEW_DESC* desc = nullptr); void CreateRTV(GPUDeviceDX12* device, ID3D12Resource* resource, D3D12_RENDER_TARGET_VIEW_DESC* desc = nullptr); diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.cpp b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.cpp index 066f019ae..e0f9b5a64 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.cpp @@ -189,7 +189,7 @@ bool GPUBufferDX12::OnInit() } // Create views - _view.Init(_device, this); + _view.Init(_device, this, this); if (useSRV) { D3D12_SHADER_RESOURCE_VIEW_DESC srvDesc; diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.h b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.h index 87802747e..09a3891b4 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.h +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUBufferDX12.h @@ -46,10 +46,12 @@ public: /// /// The graphics device. /// The resource owner. - void Init(GPUDeviceDX12* device, ResourceOwnerDX12* owner) + /// The parent resource. + void Init(GPUDeviceDX12* device, ResourceOwnerDX12* owner, GPUResource* parent) { _device = device; _owner = owner; + _parent = parent; } /// diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUContextDX12.cpp b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUContextDX12.cpp index 8c31a358a..1d8b89659 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUContextDX12.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUContextDX12.cpp @@ -926,9 +926,13 @@ void GPUContextDX12::BindCB(int32 slot, GPUConstantBuffer* cb) void GPUContextDX12::BindSR(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_SR_BINDED); + if (view && ((IShaderResourceDX12*)view->GetNativePtr())->SRV().ptr == 0) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::SRV); +#endif auto handle = view ? (IShaderResourceDX12*)view->GetNativePtr() : nullptr; - if (_srHandles[slot] != handle || !handle) + if (_srHandles[slot] != handle) { _srMaskDirtyGraphics |= 1 << slot; _srMaskDirtyCompute |= 1 << slot; @@ -940,7 +944,11 @@ void GPUContextDX12::BindSR(int32 slot, GPUResourceView* view) void GPUContextDX12::BindUA(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_UA_BINDED); + if (view && ((IShaderResourceDX12*)view->GetNativePtr())->UAV().ptr == 0) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::UAV); +#endif _uaHandles[slot] = view ? (IShaderResourceDX12*)view->GetNativePtr() : nullptr; if (view) *view->LastRenderTime = _lastRenderTime; diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.cpp b/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.cpp index fbb3392ab..6b7151eca 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.cpp +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.cpp @@ -13,6 +13,7 @@ void GPUBufferViewVulkan::Init(GPUDeviceVulkan* device, GPUBufferVulkan* owner, { ASSERT(View == VK_NULL_HANDLE); + _parent = owner; Device = device; Owner = owner; Buffer = buffer; diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.h b/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.h index a75e2663e..fbbc3eb15 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.h +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUBufferVulkan.h @@ -46,6 +46,10 @@ public: void DescriptorAsUniformTexelBuffer(GPUContextVulkan* context, VkBufferView& bufferView) override; void DescriptorAsStorageBuffer(GPUContextVulkan* context, VkBuffer& buffer, VkDeviceSize& offset, VkDeviceSize& range) override; void DescriptorAsStorageTexelBuffer(GPUContextVulkan* context, VkBufferView& bufferView) override; +#if !BUILD_RELEASE + bool HasSRV() const override { return ((GPUBuffer*)_parent)->IsShaderResource(); } + bool HasUAV() const override { return ((GPUBuffer*)_parent)->IsUnorderedAccess(); } +#endif }; /// diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUContextVulkan.cpp b/Source/Engine/GraphicsDevice/Vulkan/GPUContextVulkan.cpp index a8b34f781..c363bed45 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUContextVulkan.cpp +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUContextVulkan.cpp @@ -1001,7 +1001,11 @@ void GPUContextVulkan::BindCB(int32 slot, GPUConstantBuffer* cb) void GPUContextVulkan::BindSR(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_SR_BINDED); + if (view && ((DescriptorOwnerResourceVulkan*)view->GetNativePtr())->HasSRV() == false) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::SRV); +#endif const auto handle = view ? (DescriptorOwnerResourceVulkan*)view->GetNativePtr() : nullptr; if (_srHandles[slot] != handle) { @@ -1013,7 +1017,11 @@ void GPUContextVulkan::BindSR(int32 slot, GPUResourceView* view) void GPUContextVulkan::BindUA(int32 slot, GPUResourceView* view) { +#if !BUILD_RELEASE ASSERT(slot >= 0 && slot < GPU_MAX_UA_BINDED); + if (view && ((DescriptorOwnerResourceVulkan*)view->GetNativePtr())->HasUAV() == false) + LogInvalidResourceUsage(slot, view, InvalidBindPoint::UAV); +#endif const auto handle = view ? (DescriptorOwnerResourceVulkan*)view->GetNativePtr() : nullptr; if (_uaHandles[slot] != handle) { diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.h b/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.h index d31149d20..e24bf449c 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.h +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.h @@ -722,6 +722,12 @@ public: { CRASH; } + +#if !BUILD_RELEASE + // Utilities for incorrect resource usage. + virtual bool HasSRV() const { return false; } + virtual bool HasUAV() const { return false; } +#endif }; extern GPUDevice* CreateGPUDeviceVulkan(); diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUTextureVulkan.h b/Source/Engine/GraphicsDevice/Vulkan/GPUTextureVulkan.h index 46da296d6..df53d35ac 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUTextureVulkan.h +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUTextureVulkan.h @@ -77,6 +77,10 @@ public: // [DescriptorOwnerResourceVulkan] void DescriptorAsImage(GPUContextVulkan* context, VkImageView& imageView, VkImageLayout& layout) override; void DescriptorAsStorageImage(GPUContextVulkan* context, VkImageView& imageView, VkImageLayout& layout) override; +#if !BUILD_RELEASE + bool HasSRV() const override { return ((GPUTexture*)_parent)->IsShaderResource(); } + bool HasUAV() const override { return ((GPUTexture*)_parent)->IsUnorderedAccess(); } +#endif }; ///