Skip to content
Snippets Groups Projects

vkd3d: Implement ID3D12Device2::CreatePipelineState().

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:createpipelinestate into master
2 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • -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.

    • Author Developer

      Do we need "dummy_align"? Could we use DECLSPEC_ALIGN?

      DECLSPEC_ALIGN broke the tests because the size was unaffected, e.g. sizeof(union d3d12_blend_subobject) was 396 instead of its aligned size of 400.

    • And where does this cause a problem? I'd expect the struct layout to remain the same, due to the alignment of the following member begin a multiple of 8.

    • Author Developer

      It works if applied to the first struct member instead of the whole struct declaration.

    • Please register or sign in to reply
  • Conor McCarthy added 6 commits

    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.

    Compare with previous version

  • Conor McCarthy added 49 commits

    added 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.

    Compare with previous version

  • Giovanni Mascellani
  • Giovanni Mascellani
  • Conor McCarthy added 2 commits

    added 2 commits

    • b623dd91 - tests: Test CreatePipelineState().
    • a90f4c38 - vkd3d: Support depth bounds test.

    Compare with previous version

  • Conor McCarthy added 2 commits

    added 2 commits

    • 4c6730c8 - tests: Test CreatePipelineState().
    • b54b7f90 - vkd3d: Support depth bounds test.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • 76930795 - vkd3d: Implement ID3D12Device2::CreatePipelineState().
    • 055b7720 - tests: Test CreatePipelineState().
    • 4f4e1604 - vkd3d: Support depth bounds test.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

    • +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...

    • Author Developer

      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.

    • Yeah, it seems to do the right thing: https://godbolt.org/z/dfM4WT38T

    • Please register or sign in to reply
  • Conor McCarthy added 3 commits

    added 3 commits

    • 6909cedb - vkd3d: Implement ID3D12Device2::CreatePipelineState().
    • ce45b0a9 - tests: Test CreatePipelineState().
    • 3bb5f6ee - vkd3d: Support depth bounds test.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • e74384e7 - vkd3d: Implement ID3D12Device2::CreatePipelineState().
    • ced1e872 - tests: Test CreatePipelineState().
    • 8aa37243 - vkd3d: Support depth bounds test.

    Compare with previous version

  • 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.

  • Conor McCarthy added 5 commits

    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.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 15 commits

    added 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.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading