From 01c6d5491e9b5bada21a501c03b9358819feb8e8 Mon Sep 17 00:00:00 2001
From: sunshineinabox <aqemail@gmail.com>
Date: Mon, 9 Sep 2024 18:53:40 -0700
Subject: [PATCH] Code Review Suggestions
---
src/Ryujinx.Graphics.Vulkan/PipelineBase.cs | 136 ++++++++++--------
.../PipelineDynamicState.cs | 31 ++--
src/Ryujinx.Graphics.Vulkan/PipelineState.cs | 2 -
.../VulkanInitialization.cs | 6 +-
src/Ryujinx.Graphics.Vulkan/VulkanRenderer.cs | 1 +
5 files changed, 100 insertions(+), 76 deletions(-)
diff --git a/src/Ryujinx.Graphics.Vulkan/PipelineBase.cs b/src/Ryujinx.Graphics.Vulkan/PipelineBase.cs
index 563201b666..0343bac6e3 100644
--- a/src/Ryujinx.Graphics.Vulkan/PipelineBase.cs
+++ b/src/Ryujinx.Graphics.Vulkan/PipelineBase.cs
@@ -1,4 +1,5 @@
using Ryujinx.Common.Logging;
+using Ryujinx.Common.Memory;
using Ryujinx.Graphics.GAL;
using Ryujinx.Graphics.Shader;
using Silk.NET.Vulkan;
@@ -640,14 +641,32 @@ namespace Ryujinx.Graphics.Vulkan
{
if (texture is TextureView srcTexture)
{
- var oldCullMode = _supportExtDynamic ? DynamicState.CullMode : _newState.CullMode;
- var oldStencilTestEnable = _supportExtDynamic ? DynamicState.StencilTestEnable : _newState.StencilTestEnable;
- var oldDepthTestEnable = _supportExtDynamic ? DynamicState.DepthTestEnable : _newState.DepthTestEnable;
- var oldDepthWriteEnable = _supportExtDynamic ? DynamicState.DepthWriteEnable : _newState.DepthWriteEnable;
- var oldTopology = _supportExtDynamic ? DynamicState.Topology : _newState.Topology;
- var oldTopologyClass = _newState.Topology;
- var oldViewports = DynamicState.Viewports;
- var oldViewportsCount = _supportExtDynamic ? DynamicState.ViewportsCount : _newState.ViewportsCount;
+ CullModeFlags oldCullMode;
+ bool oldStencilTestEnable;
+ bool oldDepthTestEnable;
+ bool oldDepthWriteEnable;
+ Silk.NET.Vulkan.PrimitiveTopology oldTopology;
+ Array16<Silk.NET.Vulkan.Viewport> oldViewports = DynamicState.Viewports;
+ uint oldViewportsCount;
+
+ if (_supportExtDynamic)
+ {
+ oldCullMode = DynamicState.CullMode;
+ oldStencilTestEnable = DynamicState.StencilTestEnable;
+ oldDepthTestEnable = DynamicState.DepthTestEnable;
+ oldDepthWriteEnable = DynamicState.DepthWriteEnable;
+ oldTopology = DynamicState.Topology;
+ oldViewportsCount = DynamicState.ViewportsCount;
+ }
+ else
+ {
+ oldCullMode = _newState.CullMode;
+ oldStencilTestEnable = _newState.StencilTestEnable;
+ oldDepthTestEnable = _newState.DepthTestEnable;
+ oldDepthWriteEnable = _newState.DepthWriteEnable;
+ oldTopology = _newState.Topology;
+ oldViewportsCount = _newState.ViewportsCount;
+ }
if (_supportExtDynamic)
{
@@ -661,9 +680,9 @@ namespace Ryujinx.Graphics.Vulkan
_newState.StencilTestEnable = false;
_newState.DepthTestEnable = false;
_newState.DepthWriteEnable = false;
- }
- SignalStateChange();
+ SignalStateChange();
+ }
Gd.HelperShader.DrawTexture(
Gd,
@@ -673,13 +692,10 @@ namespace Ryujinx.Graphics.Vulkan
srcRegion,
dstRegion);
+ _newState.Topology = oldTopology;
+
if (_supportExtDynamic)
{
- if (oldTopologyClass != _newState.Topology)
- {
- _newState.Topology = oldTopology;
- }
-
DynamicState.SetCullMode(oldCullMode);
DynamicState.SetStencilTest(oldStencilTestEnable);
DynamicState.SetDepthTestBool(oldDepthTestEnable, oldDepthWriteEnable);
@@ -692,7 +708,6 @@ namespace Ryujinx.Graphics.Vulkan
_newState.DepthTestEnable = oldDepthTestEnable;
_newState.DepthWriteEnable = oldDepthWriteEnable;
_newState.ViewportsCount = oldViewportsCount;
- _newState.Topology = oldTopology;
}
DynamicState.SetViewports(ref oldViewports, oldViewportsCount);
@@ -832,7 +847,6 @@ namespace Ryujinx.Graphics.Vulkan
if (_supportExtDynamic2)
{
DynamicState.SetDepthBiasEnable(depthBiasEnable);
- changed = true;
}
else if (_newState.DepthBiasEnable != depthBiasEnable)
{
@@ -843,7 +857,6 @@ namespace Ryujinx.Graphics.Vulkan
if (depthBiasEnable)
{
DynamicState.SetDepthBias(factor, units, clamp);
- changed = true;
}
if (changed)
@@ -888,10 +901,11 @@ namespace Ryujinx.Graphics.Vulkan
_newState.DepthTestEnable = depthTest.TestEnable;
_newState.DepthWriteEnable = depthTest.WriteEnable;
_newState.DepthCompareOp = depthTest.Func.Convert();
+
+ SignalStateChange();
}
UpdatePassDepthStencil();
- SignalStateChange();
}
public void SetFaceCulling(bool enable, Face face)
@@ -903,9 +917,9 @@ namespace Ryujinx.Graphics.Vulkan
else
{
_newState.CullMode = enable ? face.Convert() : CullModeFlags.None;
- }
- SignalStateChange();
+ SignalStateChange();
+ }
}
public void SetFrontFace(FrontFace frontFace)
@@ -917,9 +931,9 @@ namespace Ryujinx.Graphics.Vulkan
else
{
_newState.FrontFace = frontFace.Convert();
- }
- SignalStateChange();
+ SignalStateChange();
+ }
}
public void SetImage(ShaderStage stage, int binding, ITexture image, Format imageFormat)
@@ -962,8 +976,6 @@ namespace Ryujinx.Graphics.Vulkan
{
DynamicState.SetLineWidth(Gd.Capabilities.SupportsWideLines ? width : 1.0f);
}
-
- SignalStateChange();
}
public void SetLogicOpState(bool enable, LogicalOp op)
@@ -1007,9 +1019,9 @@ namespace Ryujinx.Graphics.Vulkan
else
{
_newState.PatchControlPoints = (uint)vertices;
- }
- SignalStateChange();
+ SignalStateChange();
+ }
// TODO: Default levels (likely needs emulation on shaders?)
}
@@ -1033,10 +1045,11 @@ namespace Ryujinx.Graphics.Vulkan
else
{
_newState.PrimitiveRestartEnable = enable;
+
+ SignalStateChange();
}
// TODO: What to do about the index?
- SignalStateChange();
}
public void SetPrimitiveTopology(PrimitiveTopology topology)
@@ -1045,19 +1058,12 @@ namespace Ryujinx.Graphics.Vulkan
var vkTopology = Gd.TopologyRemap(topology).Convert();
+ _newState.Topology = vkTopology;
+
if (_supportExtDynamic)
{
- if ((_newState.Topology != vkTopology))
- {
- _newState.Topology = vkTopology;
- }
-
DynamicState.SetPrimitiveTopology(vkTopology);
}
- else
- {
- _newState.Topology = vkTopology;
- }
SignalStateChange();
}
@@ -1074,7 +1080,7 @@ namespace Ryujinx.Graphics.Vulkan
_newState.PipelineLayout = internalProgram.PipelineLayout;
_newState.HasTessellationControlShader = internalProgram.HasTessellationControlShader;
- _newState.StagesCount = internalProgram.IsCompute ? 1 : (uint)stages.Length;
+ _newState.StagesCount = (uint)stages.Length;
stages.CopyTo(_newState.Stages.AsSpan()[..stages.Length]);
@@ -1104,15 +1110,16 @@ namespace Ryujinx.Graphics.Vulkan
public void SetRasterizerDiscard(bool discard)
{
- if (!_supportExtDynamic2)
- {
- _newState.RasterizerDiscardEnable = discard;
- }
- else
+ if (_supportExtDynamic2)
{
DynamicState.SetRasterizerDiscard(discard);
}
- SignalStateChange();
+ else
+ {
+ _newState.RasterizerDiscardEnable = discard;
+
+ SignalStateChange();
+ }
if (!discard && Gd.IsQualcommProprietary)
{
@@ -1195,11 +1202,6 @@ namespace Ryujinx.Graphics.Vulkan
ClearScissor = regions[0];
}
- if (!_supportExtDynamic)
- {
- _newState.ScissorsCount = (uint)count;
- }
-
DynamicState.ScissorsCount = count;
for (int i = 0; i < count; i++)
@@ -1211,7 +1213,12 @@ namespace Ryujinx.Graphics.Vulkan
DynamicState.SetScissor(i, new Rect2D(offset, extent));
}
- SignalStateChange();
+ if (!_supportExtDynamic)
+ {
+ _newState.ScissorsCount = (uint)count;
+
+ SignalStateChange();
+ }
}
public void SetStencilTest(StencilTestDescriptor stencilTest)
@@ -1228,6 +1235,8 @@ namespace Ryujinx.Graphics.Vulkan
stencilTest.FrontDpFail.Convert(),
stencilTest.FrontFunc.Convert(),
stencilTest.TestEnable);
+
+ UpdatePassDepthStencil();
}
else
{
@@ -1240,6 +1249,9 @@ namespace Ryujinx.Graphics.Vulkan
_newState.StencilFrontDepthFailOp = stencilTest.FrontDpFail.Convert();
_newState.StencilFrontCompareOp = stencilTest.FrontFunc.Convert();
_newState.StencilTestEnable = stencilTest.TestEnable;
+
+ UpdatePassDepthStencil();
+ SignalStateChange();
}
DynamicState.SetStencilMask((uint)stencilTest.BackFuncMask,
@@ -1248,9 +1260,6 @@ namespace Ryujinx.Graphics.Vulkan
(uint)stencilTest.FrontFuncMask,
(uint)stencilTest.FrontMask,
(uint)stencilTest.FrontFuncRef);
-
- UpdatePassDepthStencil();
- SignalStateChange();
}
public void SetStorageBuffers(ReadOnlySpan<BufferAssignment> buffers)
@@ -1472,11 +1481,6 @@ namespace Ryujinx.Graphics.Vulkan
return Math.Clamp(value, 0f, 1f);
}
- if (!_supportExtDynamic)
- {
- _newState.ViewportsCount = (uint)count;
- }
-
DynamicState.ViewportsCount = (uint)count;
for (int i = 0; i < count; i++)
@@ -1492,7 +1496,12 @@ namespace Ryujinx.Graphics.Vulkan
Clamp(viewport.DepthFar)));
}
- SignalStateChange();
+ if (!_supportExtDynamic)
+ {
+ _newState.ViewportsCount = (uint)count;
+
+ SignalStateChange();
+ }
}
public void SwapBuffer(Auto<DisposableBuffer> from, Auto<DisposableBuffer> to)
@@ -1757,7 +1766,14 @@ namespace Ryujinx.Graphics.Vulkan
}
// Stencil test being enabled doesn't necessarily mean a write, but it's not critical to check.
- _passWritesDepthStencil |= _supportExtDynamic ? (DynamicState.DepthTestEnable && DynamicState.DepthWriteEnable) || DynamicState.StencilTestEnable : (_newState.DepthTestEnable && _newState.DepthWriteEnable) || _newState.StencilTestEnable;
+ if (_supportExtDynamic)
+ {
+ _passWritesDepthStencil |= (DynamicState.DepthTestEnable && DynamicState.DepthWriteEnable) || DynamicState.StencilTestEnable;
+ }
+ else
+ {
+ _passWritesDepthStencil |= (_newState.DepthTestEnable && _newState.DepthWriteEnable) || _newState.StencilTestEnable;
+ }
}
private bool RecreateGraphicsPipelineIfNeeded()
diff --git a/src/Ryujinx.Graphics.Vulkan/PipelineDynamicState.cs b/src/Ryujinx.Graphics.Vulkan/PipelineDynamicState.cs
index c119216990..66762232f0 100644
--- a/src/Ryujinx.Graphics.Vulkan/PipelineDynamicState.cs
+++ b/src/Ryujinx.Graphics.Vulkan/PipelineDynamicState.cs
@@ -82,7 +82,7 @@ namespace Ryujinx.Graphics.Vulkan
PrimitiveTopology = 1 << 16,
DepthBiasEnable = 1 << 17,
Standard = Blend | DepthBias | Scissor | Stencil | Viewport | FeedbackLoop,
- Extended = CullMode | FrontFace | DepthTestBool | DepthTestCompareOp | StencilTestEnableandStencilOp | PrimitiveTopology,
+ Extended = CullMode | FrontFace | DepthTestBool | DepthTestCompareOp | StencilTestEnableAndStencilOp | PrimitiveTopology,
Extended2 = RasterDiscard | PrimitiveRestart | DepthBiasEnable,
}
@@ -131,9 +131,16 @@ namespace Ryujinx.Graphics.Vulkan
_dirty |= DirtyFlags.DepthTestCompareOp;
}
- public void SetStencilTestandOp(StencilOp backFailOp, StencilOp backPassOp, StencilOp backDepthFailOp,
- CompareOp backCompareOp, StencilOp frontFailOp, StencilOp frontPassOp, StencilOp frontDepthFailOp,
- CompareOp frontCompareOp, bool stencilTestEnable)
+ public void SetStencilTestandOp(
+ StencilOp backFailOp,
+ StencilOp backPassOp,
+ StencilOp backDepthFailOp,
+ CompareOp backCompareOp,
+ StencilOp frontFailOp,
+ StencilOp frontPassOp,
+ StencilOp frontDepthFailOp,
+ CompareOp frontCompareOp,
+ bool stencilTestEnable)
{
_backFailOp = backFailOp;
_backPassOp = backPassOp;
@@ -146,18 +153,22 @@ namespace Ryujinx.Graphics.Vulkan
StencilTestEnable = stencilTestEnable;
- _dirty |= DirtyFlags.StencilTestEnableandStencilOp;
+ _dirty |= DirtyFlags.StencilTestEnableAndStencilOp;
}
public void SetStencilTest(bool stencilTestEnable)
{
StencilTestEnable = stencilTestEnable;
- _dirty |= DirtyFlags.StencilTestEnableandStencilOp;
+ _dirty |= DirtyFlags.StencilTestEnableAndStencilOp;
}
- public void SetStencilMask(uint backCompareMask, uint backWriteMask, uint backReference,
- uint frontCompareMask, uint frontWriteMask, uint frontReference)
+ public void SetStencilMask(uint backCompareMask,
+ uint backWriteMask,
+ uint backReference,
+ uint frontCompareMask,
+ uint frontWriteMask,
+ uint frontReference)
{
_backCompareMask = backCompareMask;
_backWriteMask = backWriteMask;
@@ -324,9 +335,9 @@ namespace Ryujinx.Graphics.Vulkan
RecordDepthTestCompareOp(gd.ExtendedDynamicStateApi, commandBuffer);
}
- if (_dirty.HasFlag(DirtyFlags.StencilTestEnableandStencilOp))
+ if (_dirty.HasFlag(DirtyFlags.StencilTestEnableAndStencilOp))
{
- RecordStencilTestandOp(gd.ExtendedDynamicStateApi, commandBuffer);
+ RecordStencilTestAndOp(gd.ExtendedDynamicStateApi, commandBuffer);
}
if (_dirty.HasFlag(DirtyFlags.LineWidth))
diff --git a/src/Ryujinx.Graphics.Vulkan/PipelineState.cs b/src/Ryujinx.Graphics.Vulkan/PipelineState.cs
index 793c2c9812..d1630faf34 100644
--- a/src/Ryujinx.Graphics.Vulkan/PipelineState.cs
+++ b/src/Ryujinx.Graphics.Vulkan/PipelineState.cs
@@ -258,8 +258,6 @@ namespace Ryujinx.Graphics.Vulkan
private bool _supportsFeedBackLoopDynamicState;
private uint _blendEnables;
-
-
public void Initialize(HardwareCapabilities capabilities)
{
HasTessellationControlShader = false;
diff --git a/src/Ryujinx.Graphics.Vulkan/VulkanInitialization.cs b/src/Ryujinx.Graphics.Vulkan/VulkanInitialization.cs
index a393ee9441..44c8a5dea2 100644
--- a/src/Ryujinx.Graphics.Vulkan/VulkanInitialization.cs
+++ b/src/Ryujinx.Graphics.Vulkan/VulkanInitialization.cs
@@ -492,10 +492,8 @@ namespace Ryujinx.Graphics.Vulkan
{
SType = StructureType.PhysicalDeviceExtendedDynamicState2FeaturesExt,
PNext = pExtendedFeatures,
- ExtendedDynamicState2 =
- supportedFeaturesExtExtendedDynamicState2.ExtendedDynamicState2,
- ExtendedDynamicState2LogicOp =
- supportedFeaturesExtExtendedDynamicState2.ExtendedDynamicState2LogicOp,
+ ExtendedDynamicState2 = supportedFeaturesExtExtendedDynamicState2.ExtendedDynamicState2,
+ ExtendedDynamicState2LogicOp = supportedFeaturesExtExtendedDynamicState2.ExtendedDynamicState2LogicOp,
ExtendedDynamicState2PatchControlPoints = supportedFeaturesExtExtendedDynamicState2.ExtendedDynamicState2PatchControlPoints,
};
diff --git a/src/Ryujinx.Graphics.Vulkan/VulkanRenderer.cs b/src/Ryujinx.Graphics.Vulkan/VulkanRenderer.cs
index 9c603fbe35..f40ddf8f8b 100644
--- a/src/Ryujinx.Graphics.Vulkan/VulkanRenderer.cs
+++ b/src/Ryujinx.Graphics.Vulkan/VulkanRenderer.cs
@@ -423,6 +423,7 @@ namespace Ryujinx.Graphics.Vulkan
properties.Limits.FramebufferStencilSampleCounts;
// Temporarily disable this, can be added back at a later date, make it easy to re-enable.
+ // Disabled because currently causing Device Lost error on nVidia.
featuresExtendedDynamicState2.ExtendedDynamicState2PatchControlPoints = false;
Capabilities = new HardwareCapabilities(