vkd3d: Implement ID3D12Device2::CreatePipelineState().
Merge request reports
Activity
-struct D3D12_RT_FORMAT_ARRAY +typedef struct D3D12_RT_FORMAT_ARRAY { DXGI_FORMAT RTFormats[D3D12_SIMULTANEOUS_RENDER_TARGET_COUNT]; UINT NumRenderTargets; -}; +} D3D12_RT_FORMAT_ARRAY;
That should be a separate change.
- if (FAILED(hr = d3d12_pipeline_state_create_graphics(device, desc, &object))) + if (FAILED(hr = pipeline_state_desc_from_d3d12_graphics_desc(&pipeline_desc, desc))) + return hr; + + if (FAILED(hr = d3d12_pipeline_state_create_graphics(device, &pipeline_desc, &object))) return hr;
It seems a fair bit nicer to keep d3d12_device_CreateGraphicsPipelineState() the way it is, and then do the conversion from D3D12_GRAPHICS_PIPELINE_STATE_DESC to d3d12_pipeline_state_desc inside d3d12_pipeline_state_create_graphics(). Likewise for d3d12_pipeline_state_create_compute() and d3d12_pipeline_state_create().
This commit can probably also be split a little; for example, the d3d12_pipeline_state_init_compute() changes don't depend on the d3d12_pipeline_state_init_graphics() changes, and the d3d12_pipeline_state_init_graphics() changes don't depend on pipeline_state_desc_from_d3d12_stream_desc().
+static void d3d12_promote_depth_stencil_desc(D3D12_DEPTH_STENCIL_DESC1 *dst, const D3D12_DEPTH_STENCIL_DESC *src) +{ + dst->DepthEnable = src->DepthEnable; + dst->DepthWriteMask = src->DepthWriteMask; + dst->DepthFunc = src->DepthFunc; + dst->StencilEnable = src->StencilEnable; + dst->StencilReadMask = src->StencilReadMask; + dst->StencilWriteMask = src->StencilWriteMask; + dst->FrontFace = src->FrontFace; + dst->BackFace = src->BackFace; + dst->DepthBoundsTestEnable = FALSE; +}
We'd typically implement that as something like this:
memcpy(dst, src, sizeof(*src)); dst->DepthBoundsTestEnable = FALSE;
and then likely just inline it in pipeline_state_desc_from_d3d12_graphics_desc() where we could drop the "DepthBoundsTestEnable" assignment because "desc" was zero-initialised.
+static VkShaderStageFlags pipeline_state_desc_get_shader_stages(const struct d3d12_pipeline_state_desc *desc) +{ + VkShaderStageFlags result = 0; + + if (desc->vs.BytecodeLength && desc->vs.pShaderBytecode) + result |= VK_SHADER_STAGE_VERTEX_BIT; + if (desc->hs.BytecodeLength && desc->hs.pShaderBytecode) + result |= VK_SHADER_STAGE_TESSELLATION_CONTROL_BIT; + if (desc->ds.BytecodeLength && desc->ds.pShaderBytecode) + result |= VK_SHADER_STAGE_TESSELLATION_EVALUATION_BIT; + if (desc->gs.BytecodeLength && desc->gs.pShaderBytecode) + result |= VK_SHADER_STAGE_GEOMETRY_BIT; + if (desc->ps.BytecodeLength && desc->ps.pShaderBytecode) + result |= VK_SHADER_STAGE_FRAGMENT_BIT; + if (desc->cs.BytecodeLength && desc->cs.pShaderBytecode) + result |= VK_SHADER_STAGE_COMPUTE_BIT; + + /* If we use rasterizer discard, force fragment shader to not exist. + * See VUID-VkGraphicsPipelineCreateInfo-pStages-06894. */ + if (desc->stream_output.NumEntries && + desc->stream_output.RasterizedStream == D3D12_SO_NO_RASTERIZED_STREAM) + { + result &= ~VK_SHADER_STAGE_FRAGMENT_BIT; + }
How would we trigger that? We use this to set "defined_stages" in pipeline_state_desc_from_d3d12_stream_desc(), but that never checks VK_SHADER_STAGE_FRAGMENT_BIT. In fact, it only really seems to care about VK_SHADER_STAGE_VERTEX_BIT and VK_SHADER_STAGE_COMPUTE_BIT.
+#define DCL_SUBOBJECT_INFO(type, field) {__alignof__(type), sizeof(type), offsetof(struct d3d12_pipeline_state_desc, field)}
I don't think we need this outside subject_info[]. I.e., we can do something like this:
static const struct { ... } subobject_info[] = { #define DCL_SUBOBJECT_INFO(type, field) ... ... #undef DCL_SUBOBJECT_INFO };
+ const char *stream_ptr, *stream_end;
uint8_t, probably.
+ /* Structs are packed, but padded so that their size + * is always a multiple of the size of a pointer. */ + stream_ptr = d3d12_desc->pPipelineStateSubobjectStream; + stream_end = stream_ptr + d3d12_desc->SizeInBytes; + desc_char = (char *)desc;
I.e., "Stream packets are aligned to the size of pointers.", right? In any case, this doesn't necessarily seem like the ideal place for this comment; it would seem more appropriate right before we align(i + size, ...) in the loop further below.
+ subobject_type = *(const D3D12_PIPELINE_STATE_SUBOBJECT_TYPE *)stream_ptr; + subobject_bit = 1ull << subobject_type;
This only works for subobject types smaller than 64, of course. In principle that's fine, but we only validate the subobject type a few lines later, which is a little ugly.
+ if (stream_ptr + i + size > stream_end) + { + WARN("Invalid pipeline state stream.\n"); + return E_INVALIDARG; + }
It's probably fine in practice, but this kind of thing is always a bit of a trigger when reviewing code. It seems nicer to use vkd3d_bound_range(), here and in the similar comparison at the start of the loop.
+union d3d12_root_signature_subobject +{ + struct + { + D3D12_PIPELINE_STATE_SUBOBJECT_TYPE type; + ID3D12RootSignature *root_signature; + }; + void *dummy_align; +};
Do we need "dummy_align"? Could we use DECLSPEC_ALIGN? Could we make these unions local to test_create_pipeline_state()? I don't think we need them at the top level, right?
The placement of "{" and "}" is a bit unusual in a few places in the test. For example in the tests[] declaration.
added 6 commits
- 47981ea4 - include: Add a D3D12_RT_FORMAT_ARRAY typedef.
- 288e298e - vkd3d: Introduce struct d3d12_pipeline_state_desc for graphics pipelines.
- 43f817ee - vkd3d: Use struct d3d12_pipeline_state_desc for compute pipelines.
- 2c2ddb65 - vkd3d: Implement ID3D12Device2::CreatePipelineState().
- f30cf5d2 - tests: Test CreatePipelineState().
- 5e579ed1 - vkd3d: Support depth bounds test.
Toggle commit listadded 49 commits
-
5e579ed1...9de793f1 - 43 commits from branch
wine:master
- d2278b1f - include: Add a D3D12_RT_FORMAT_ARRAY typedef.
- 9061e973 - vkd3d: Introduce struct d3d12_pipeline_state_desc for graphics pipelines.
- 2bbfecca - vkd3d: Use struct d3d12_pipeline_state_desc for compute pipelines.
- f4d28d22 - vkd3d: Implement ID3D12Device2::CreatePipelineState().
- ad9b2c56 - tests: Test CreatePipelineState().
- 968bf06b - vkd3d: Support depth bounds test.
Toggle commit list-
5e579ed1...9de793f1 - 43 commits from branch
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
added 2 commits
added 2 commits
+static HRESULT pipeline_state_desc_from_d3d12_graphics_desc(struct d3d12_pipeline_state_desc *desc, + const D3D12_GRAPHICS_PIPELINE_STATE_DESC *d3d12_desc) +{ + memset(desc, 0, sizeof(*desc)); + desc->root_signature = d3d12_desc->pRootSignature; + desc->vs = d3d12_desc->VS; + desc->ps = d3d12_desc->PS; + desc->ds = d3d12_desc->DS; + desc->hs = d3d12_desc->HS; + desc->gs = d3d12_desc->GS; + desc->stream_output = d3d12_desc->StreamOutput; + desc->blend_state = d3d12_desc->BlendState; + desc->sample_mask = d3d12_desc->SampleMask; + desc->rasterizer_state = d3d12_desc->RasterizerState; + memcpy(&desc->depth_stencil_state, &d3d12_desc->DepthStencilState, sizeof(d3d12_desc->DepthStencilState)); + desc->input_layout = d3d12_desc->InputLayout; + desc->strip_cut_value = d3d12_desc->IBStripCutValue; + desc->primitive_topology_type = d3d12_desc->PrimitiveTopologyType; + desc->rtv_formats.NumRenderTargets = d3d12_desc->NumRenderTargets; + memcpy(desc->rtv_formats.RTFormats, d3d12_desc->RTVFormats, sizeof(desc->rtv_formats.RTFormats)); + desc->dsv_format = d3d12_desc->DSVFormat; + desc->sample_desc = d3d12_desc->SampleDesc; + desc->node_mask = d3d12_desc->NodeMask; + desc->cached_pso = d3d12_desc->CachedPSO; + desc->flags = d3d12_desc->Flags; + return S_OK; +}
This can't fail, right?
+static HRESULT pipeline_state_desc_from_d3d12_compute_desc(struct d3d12_pipeline_state_desc *desc, + const D3D12_COMPUTE_PIPELINE_STATE_DESC *d3d12_desc) +{ + memset(desc, 0, sizeof(*desc)); + desc->root_signature = d3d12_desc->pRootSignature; + desc->cs = d3d12_desc->CS; + desc->node_mask = d3d12_desc->NodeMask; + desc->cached_pso = d3d12_desc->CachedPSO; + desc->flags = d3d12_desc->Flags; + return S_OK; +}
Likewise.
+static HRESULT pipeline_state_desc_from_d3d12_stream_desc(struct d3d12_pipeline_state_desc *desc, + const D3D12_PIPELINE_STATE_STREAM_DESC *d3d12_desc, VkPipelineBindPoint *vk_bind_point) +{ + D3D12_PIPELINE_STATE_SUBOBJECT_TYPE subobject_type; + const uint8_t *stream_ptr, *stream_end; + uint64_t defined_subobjects = 0; + uint64_t subobject_bit; + char *desc_char;
We might as well make "desc_char" an uint8_t pointer as well.
+ stream_ptr = d3d12_desc->pPipelineStateSubobjectStream; + stream_end = stream_ptr + d3d12_desc->SizeInBytes; + desc_char = (char *)desc; + + while (stream_ptr < stream_end) + { + if (!vkd3d_bound_range(0, sizeof(subobject_type), stream_end - stream_ptr)) + { + WARN("Invalid pipeline state stream.\n"); + return E_INVALIDARG; + } + + subobject_type = *(const D3D12_PIPELINE_STATE_SUBOBJECT_TYPE *)stream_ptr; + if (subobject_type >= ARRAY_SIZE(subobject_info)) + { + FIXME("Unhandled pipeline subobject type %#x.\n", subobject_type); + return E_INVALIDARG; + } + + subobject_bit = 1ull << subobject_type; + if (defined_subobjects & subobject_bit) + { + WARN("Duplicate pipeline subobject type %u.\n", subobject_type); + return E_INVALIDARG; + } + defined_subobjects |= subobject_bit; + + i = align(sizeof(subobject_type), subobject_info[subobject_type].alignment); + size = subobject_info[subobject_type].size; + + if (!vkd3d_bound_range(i, size, stream_end - stream_ptr)) + { + WARN("Invalid pipeline state stream.\n"); + return E_INVALIDARG; + } + + memcpy(&desc_char[subobject_info[subobject_type].dst_offset], &stream_ptr[i], size); + /* Stream packets are aligned to the size of pointers. */ + stream_ptr += align(i + size, sizeof(void *)); + }
It might be slightly nicer to get rid of "stream_end", and just index the stream from the start instead of adjusting the pointer. I'm not going to insist on it though.
+ struct d3d12_sample_mask_subobject + { + DECLSPEC_ALIGN(sizeof(void *)) D3D12_PIPELINE_STATE_SUBOBJECT_TYPE type; + UINT sample_mask; + };
Does that do the right thing? It seems like this would introduce padding between "type" and "sample_mask" that previously wasn't there. The tests seem to pass on the CI, but...
Yeah, it seems to do the right thing: https://godbolt.org/z/dfM4WT38T
Does that do the right thing? It seems like this would introduce padding between "type" and "sample_mask" that previously wasn't there.
I only aligns
type
, so if the second member has 4-byte alignment there is no padding.Right, my bad. Note that the latest version of the MR still has pipeline_state_desc_from_d3d12_graphics_desc() and pipeline_state_desc_from_d3d12_compute_desc() returning HRESULTs in the commits that introduce them though. Patch 4/6 gets rid of those, but ideally we wouldn't introduce them in the first place.
added 5 commits
- d5082075 - vkd3d: Introduce struct d3d12_pipeline_state_desc for graphics pipelines.
- d431bdf4 - vkd3d: Use struct d3d12_pipeline_state_desc for compute pipelines.
- fbb08643 - vkd3d: Implement ID3D12Device2::CreatePipelineState().
- 704b00d0 - tests: Test CreatePipelineState().
- 01971d16 - vkd3d: Support depth bounds test.
Toggle commit listadded 15 commits
-
01971d16...fb588b8d - 9 commits from branch
wine:master
- 529c0f46 - include: Add a D3D12_RT_FORMAT_ARRAY typedef.
- 39afbb8e - vkd3d: Introduce struct d3d12_pipeline_state_desc for graphics pipelines.
- 6196199a - vkd3d: Use struct d3d12_pipeline_state_desc for compute pipelines.
- d8ba0d2a - vkd3d: Implement ID3D12Device2::CreatePipelineState().
- 63b8972b - tests: Test CreatePipelineState().
- 76eb0adf - vkd3d: Support depth bounds test.
Toggle commit list-
01971d16...fb588b8d - 9 commits from branch