Skip to content
Snippets Groups Projects

vkd3d: Implement ID3D12Device1.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:device_ifaces into master
1 unresolved thread

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
    • I'm not quite sure how I feel about the d3d12_device_iface typedef, so I'd like to hear from others on that one. On the one hand, this is probably one of the few legitimate uses of "typedef". On the other hand, it does add a layer of obfuscation, and I'm not sure it's solving a problem that's in dire need of solving.

           if (IsEqualGUID(riid, &IID_ID3D12Device)
      +            || IsEqualGUID(riid, &IID_ID3D12Device1)
                   || IsEqualGUID(riid, &IID_ID3D12Object)
                   || IsEqualGUID(riid, &IID_IUnknown))

      By convention, we check the newest IID first.

      +typedef enum D3D12_PIPELINE_STATE_SUBOBJECT_TYPE
      +{
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_ROOT_SIGNATURE = 0,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_VS = 1,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_PS = 2,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_DS = 3,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_HS = 4,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_GS = 5,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_CS = 6,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_STREAM_OUTPUT = 7,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_BLEND = 8,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_SAMPLE_MASK = 9,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_RASTERIZER = 10,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_DEPTH_STENCIL = 11,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_INPUT_LAYOUT = 12,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_IB_STRIP_CUT_VALUE = 13,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_PRIMITIVE_TOPOLOGY = 14,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_RENDER_TARGET_FORMATS = 15,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_DEPTH_STENCIL_FORMAT = 16,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_SAMPLE_DESC = 17,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_NODE_MASK = 18,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_CACHED_PSO = 19,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_FLAGS = 20,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_DEPTH_STENCIL1 = 21,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_VIEW_INSTANCING = 22,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_AS = 24,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_MS = 25,
      +    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_MAX_VALID = 26,
      +} D3D12_PIPELINE_STATE_SUBOBJECT_TYPE;

      We haven't been entirely consistent about it, but in general we prefer defining enum values in hexadecimal, matching the way we typically print them in debug traces.

      +#define VKD3D_HANDLE_SUBOBJECT_EXPLICIT(type_enum, type_name, assignment) \
      +    case D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_ ## type_enum: \
      +    {\
      +        const struct\
      +        {\
      +            D3D12_PIPELINE_STATE_SUBOBJECT_TYPE type; \
      +            type_name data; \
      +        } *subobject = (void *)stream_ptr; \
      +        if (stream_ptr + sizeof(*subobject) > stream_end) \
      +        { \
      +            WARN("Invalid pipeline state stream.\n"); \
      +            return E_INVALIDARG; \
      +        } \
      +        stream_ptr += align(sizeof(*subobject), sizeof(void*)); \
      +        assignment; \
      +        break;\
      +    }
      +
      +#define VKD3D_HANDLE_SUBOBJECT(type_enum, type, left_side) \
      +    VKD3D_HANDLE_SUBOBJECT_EXPLICIT(type_enum, type, left_side = subobject->data)

      Do we need the macros?

      The ID3D12Device2 and depth bounds test bits probably each deserve their own series.

    • Author Developer

      Do we need the macros?

      Instead of using a fixed header size, it depends upon packing of each struct, so the only truly reliable way to get the data is declare a struct. Without the macro the first case becomes:

      case D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_ROOT_SIGNATURE:
      {
      struct
      {
      D3D12_PIPELINE_STATE_SUBOBJECT_TYPE type;
      ID3D12RootSignature *root_signature;
      } *subobject = (void *)stream_ptr;
      desc->root_signature = subobject->root_signature;
      break;
      }

      It can be done that way if you prefer.

    • I'm not quite sure how I feel about the d3d12_device_iface typedef, so I'd like to hear from others on that one. On the one hand, this is probably one of the few legitimate uses of "typedef". On the other hand, it does add a layer of obfuscation, and I'm not sure it's solving a problem that's in dire need of solving.

      Yeah, same here. It breaks a convention that is used throughout vkd3d and Wine, for what I see as little advantage. Updating the interface version happens pretty rarely, and even if the diff's got a lot of lines, it still is a patch that is rather quick to write and to review.

    • Please register or sign in to reply
  • Conor McCarthy added 1 commit

    added 1 commit

    • da4feb3f - vkd3d: Implement ID3D12Device1 with stubs.

    Compare with previous version

  • Do we need the macros?

    Instead of using a fixed header size, it depends upon packing of each struct, so the only truly reliable way to get the data is declare a struct. Without the macro the first case becomes:

    case D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_ROOT_SIGNATURE:
    {
    struct
    {
    D3D12_PIPELINE_STATE_SUBOBJECT_TYPE type;
    ID3D12RootSignature *root_signature;
    } *subobject = (void *)stream_ptr;
    desc->root_signature = subobject->root_signature;
    break;
    }

    It can be done that way if you prefer.

    I think that's a fair bit more readable, yes, thanks.

    We may be able to do slightly better though, if we build a table like this:

        static const struct
        {
            size_t alignment;
            size_t size;
            size_t dst_offset;
        }
        subobject_info[] =
        {
            [D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_ROOT_SIGNATURE] = {__alignof__(ID3D12RootSignature *), sizeof(ID3D12RootSignature *), offsetof(struct d3d12_pipeline_state_desc, root_signature)},
            [D3D12_PIPELINE_STATE_SUBOBJECT_TYPE_VS]             = {__alignof__(ID3D12RootSignature *), sizeof(D3D12_SHADER_BYTECODE), offsetof(struct d3d12_pipeline_state_desc, vs)},
            ...
        };

    and then use that information to copy the data to the right location. We could even use macros to build that table if we're so inclined; I don't think it would be terribly necessary, but the macro in question would be simple enough and save some typing/reading.

  • Conor McCarthy changed title from vkd3d: Support higher ID3D12Device versions. to vkd3d: Implement ID3D12Device1.

    changed title from vkd3d: Support higher ID3D12Device versions. to vkd3d: Implement ID3D12Device1.

  • Conor McCarthy added 1 commit

    added 1 commit

    • fcc5ac7d - vkd3d: Implement ID3D12Device1 with stubs.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • -static inline struct d3d12_device *impl_from_ID3D12Device(ID3D12Device *iface)
    +static inline struct d3d12_device *impl_from_ID3D12Device(ID3D12Device1 *iface)

    This introduces a mismatch between the function name and what it actually does. I think it's particularly jarring in places like vkd3d_get_vk_device().

  • Conor McCarthy added 1 commit

    added 1 commit

    • 91321bc0 - vkd3d: Implement ID3D12Device1 with stubs.

    Compare with previous version

  •  static inline HRESULT d3d12_device_query_interface(struct d3d12_device *device, REFIID iid, void **object)
     {
    -    return ID3D12Device_QueryInterface(&device->ID3D12Device_iface, iid, object);
    +    return ID3D12Device_QueryInterface(&device->ID3D12Device1_iface, iid, object);
     }
     
     static inline ULONG d3d12_device_add_ref(struct d3d12_device *device)
     {
    -    return ID3D12Device_AddRef(&device->ID3D12Device_iface);
    +    return ID3D12Device_AddRef(&device->ID3D12Device1_iface);
     }
     
     static inline ULONG d3d12_device_release(struct d3d12_device *device)
     {
    -    return ID3D12Device_Release(&device->ID3D12Device_iface);
    +    return ID3D12Device_Release(&device->ID3D12Device1_iface);
     }
     
     static inline unsigned int d3d12_device_get_descriptor_handle_increment_size(struct d3d12_device *device,
             D3D12_DESCRIPTOR_HEAP_TYPE descriptor_type)
     {
    -    return ID3D12Device_GetDescriptorHandleIncrementSize(&device->ID3D12Device_iface, descriptor_type);
    +    return ID3D12Device_GetDescriptorHandleIncrementSize(&device->ID3D12Device1_iface, descriptor_type);
     }

    We should use the ID3D12Device1 variants here.

    I was slightly surprised the compiler didn't catch that; looks like we don't enable WIDL_C_INLINE_WRAPPERS. IIRC that was because it was slightly broken with older versions of MinGW-64, but perhaps we can revisit that.

  • Conor McCarthy added 1 commit

    added 1 commit

    • 67d7427f - vkd3d: Implement ID3D12Device1 with stubs.

    Compare with previous version

  • Note that this doesn't apply cleanly to current git.

  • Conor McCarthy added 93 commits

    added 93 commits

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 3 commits

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading