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

Respect maxGeometryTotalOutputComponents limit #1121

Closed
liam-middlebrook opened this issue Jul 11, 2019 · 8 comments
Closed

Respect maxGeometryTotalOutputComponents limit #1121

liam-middlebrook opened this issue Jul 11, 2019 · 8 comments
Labels

Comments

@liam-middlebrook
Copy link
Contributor

Currently DXVK runs into issues with geometry shaders on some titles on NVIDIA hardware where it creates SPIR-V modules which exceed the hardware defined limit of 1024 (on all current vulkan-supported NVIDIA hardware) for maxGeometryTotalOutputComponents. This issue is can arise from two potential factors:

  1. DXVK adds an additional Output variable gs_vertex_out which it associates with SV_POSITION and that becomes a pointer to the output variable that gets created for the DXBC dcl_output_siv for SV_POSITION.

  2. If an application passed a DXBC blob to DXVK which violated this limit, DXVK should be able to provide a more accessible error message to the developer.

Let's dive a bit deeper on the first factor. This additional Output variable is currently generated by DXVK to cover cases where applications don't have dcl_input_siv directives in a fragment shader which match with the dcl_output_siv directives in a geometry shader [1]. After a discussion that @jeffbolznv and I had with a member of our D3D team we learned that applications may additionally pass in this system value information through an ISGN entry.

One of the geometry shaders in Justice (the Chinese MMO) over-extends the limit when translated through DXVK. This pipeline this shader is used in raises the same concern as above, where the geometry shader's dcl_output_siv for SV_POSITION not having a matching dcl_input_sivin the fragment shader for the pipeline. I've written out some patches (which are available in PR #1120 ) which allow for debug-logging of the ISGN, OSGN, PSGN entries for a shader.

Here's the OSGN and ISGN for the geometry and fragment shaders I was looking at in Justice. Note the systemValue, for registerId=3.


debug: Output Signature for - GS_f65d0f8030f8da306b336b69230ad1999c5442f1

debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 0
        registerId: 0
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 1
        registerId: 1
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 2
        registerId: 2
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: SV_Position
        semanticIndex: 0
        registerId: 3
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::Position
        streamId: 0

debug: Input Signature for - FS_a9f8f8cfa79a54a3f28b6889af2e0098b7050b83

debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 0
        registerId: 0
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 1
        registerId: 1
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: TEXCOORD
        semanticIndex: 2
        registerId: 2
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::None
        streamId: 0


debug: SGN Entry:
        semanticName: SV_Position
        semanticIndex: 0
        registerId: 3
        componentMask: xyzw
        componentType: DxbcScalarType::Float32
        systemValue: DxbcSystemValue::Position
        streamId: 0


debug: SGN Entry:
        semanticName: SV_IsFrontFace
        semanticIndex: 0
        registerId: 4
        componentMask: x
        componentType: DxbcScalarType::Uint32
        systemValue: DxbcSystemValue::IsFrontFace
        streamId: 0

So I think the proper fix for factor one will be to check against the ISGN and OSGN for system values (in case a geometry shader doesn't dcl_output_siv even though the corresponding fragment shader uses dcl_input_siv), and then apply the spv::BuiltInPosition decoration (or other built-ins as appropriate).

Factor two will probably be much less common and practically can be covered by @jeffbolznv's change adding the maxGeometryTotalOutputComponents check to the validation layers.

@doitsujin
Copy link
Owner

The problem with this issue is that we don't know which fragment shader a geometry shader will be used with at the time we do the DXBC->SPIR-V translation. I'm concerned that performing this translation at draw time is going to make the stuttering issues that DXVK has due to pipeline compilation even worse than they already are, not to mention that doing so would require a major rework.

Another option might be to patch the generated SPIR-V at the time the pipeline gets compiled and change unused output locations to private variables, but that's also rather tricky.

@liam-middlebrook
Copy link
Contributor Author

We don't need to know which fragment shader a geometry shader will be used with at translation-time.

We know that a geometry shader should have at least one dcl_output or dcl_output_siv. When DXVK parses these declarations in DxbcCompiler::emitDclOutput() and encounters the SV_POSITION output declaration (be it through dcl_output_siv or matching the registerId from the OSGN) it should emit a variable that is decorated with spv::BuiltInPosition.

I spent some time prototyping this out today. I'll try and get my patches for a fix cleaned up to send over to you in the next few days / week.

@doitsujin
Copy link
Owner

doitsujin commented Jul 12, 2019

The question is, what is supposed to happen if a game uses the geometry shader with a fragment shader which doesn't have the declaration? As far as I know, this is technically undefined behaviour in D3D11, but in practice, many games do rely on the exact ways things like this are handled in Windows drivers.

@doitsujin
Copy link
Owner

838372d (currently in a side branch) should be sufficient to fix this issue, but I'll have to test if this may cause any regressions.

@liam-middlebrook
Copy link
Contributor Author

I don't think that covers the case where a the SHDR section doesn't specify any _siv variants, but the paired shader expects a system value to be associated with an output. I have a patch #1123 that should resolve that.

@doitsujin
Copy link
Owner

doitsujin commented Jul 12, 2019

Is that even possible? It would be extremely weird if the HLSL compiler wrote that info into the signatures but not the instruction stream, and DXVK doesn't take that into account when mapping the o# registers to SPIR-V built-ins right now anyway so something should be broken if there are shaders like this in the wild.

@doitsujin
Copy link
Owner

For some reason, this breaks terrain rendering in Assassin's Creed Odyssey, even with your additional patch. Not sure what's going on there just yet.

@doitsujin doitsujin added the bug label Jul 13, 2019
@doitsujin
Copy link
Owner

doitsujin commented Jul 13, 2019

Turns out that this is actually the problem you described, but DXVK does not load system values in TCS/GS stages.

I don't think this can be fixed for those stages. Vulkan wants us to put all built-ins that are passed between stages in a block where the VS output block must be identical to the TCS input block, but we don't necessarily know which ones we need since the TCS ISGN may only be a subset of the VS OSGN. Limiting this to geometry shaders only should work though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants