[PATCH vkd3d 5/5] include: Document struct vkd3d_shader_interface_info.

[Zebediah Figura]

On 8/27/20 11:18 AM, Henri Verbeet wrote:
> On Thu, 27 Aug 2020 at 07:46, Zebediah Figura <z.figura12 at gmail.com> wrote:
>> +/** The type of a shader resource descriptor. */
>>  enum vkd3d_shader_descriptor_type
>>  {
>> -    VKD3D_SHADER_DESCRIPTOR_TYPE_SRV     = 0x0, /* t#  */
>> +    /** The descriptor is a shader resource view, bound to a t# register. */
>> +    VKD3D_SHADER_DESCRIPTOR_TYPE_SRV     = 0x0,
>> +    /** The descriptor is an unordered access view, bound to a u# register. */
>>      VKD3D_SHADER_DESCRIPTOR_TYPE_UAV     = 0x1, /* u#  */
>> +    /** The descriptor is a constant buffer view, bound to a cb# register. */
>>      VKD3D_SHADER_DESCRIPTOR_TYPE_CBV     = 0x2, /* cb# */
>> +    /** The descriptor is a sampler, bound to a s# register. */
>>      VKD3D_SHADER_DESCRIPTOR_TYPE_SAMPLER = 0x3, /* s#  */
>>
>>      VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_DESCRIPTOR_TYPE),
>>  };
>>
> I think I'd either leave the t#/u#/cb#/s# comments as they are, or
> write something slightly more elaborate. They make sense in the
> context of D3D assembly or byte code, but wouldn't necessarily when
> compiling e.g. HLSL to GLSL.
> 
>> +/**
>> + * A common structure describing the mapping a DXBC descriptor or descriptor
>> + * array to a SPIR-V descriptor or descriptor array.
>> + */
>>  struct vkd3d_shader_descriptor_binding
>>  {
>> +    /** The set of the Vulkan descriptor. */
>>      unsigned int set;
>> +    /** The binding index, within the set, of the Vulkan descriptor. */
>>      unsigned int binding;
>> -    unsigned int count; /* This must be 1 in this version of vkd3d-shader. */
>> +    /**
>> +     * The size of this descriptor array. Descriptor arrays are not supported in
>> +     * this version of vkd3d-shader, and therefore this value must be 1.
>> +     */
>> +    unsigned int count;
>>  };
> The vkd3d_shader_descriptor_binding structure doesn't describe the
> mapping as such; it describes the bind point in the target
> environment. It should also be noted that the target environment isn't
> necessarily Vulkan; SPIR-V for example can be used with OpenGL as
> well.
> 
>> +/**
>> + * Describes the mapping of a single DXBC resource or resource array to SPIR-V.
>> + * This structure is used in struct vkd3d_shader_interface_info.
>> + */
>>  struct vkd3d_shader_resource_binding
>>  {
>> +    /** The type of this descriptor. */
>>      enum vkd3d_shader_descriptor_type type;
>> +    /**
>> +     * Register space of the DXBC descriptor. If the source format does not
>> +     * support multiple register spaces, this parameter must be set to 0.
>> +     */
>>      unsigned int register_space;
>> +    /** Register index of the DXBC descriptor. */
>>      unsigned int register_index;
>>      enum vkd3d_shader_visibility shader_visibility;
>> -    unsigned int flags; /* vkd3d_shader_binding_flag */
>> +    /** A combination of zero or more elements of vkd3d_shader_binding_flag. */
>> +    unsigned int flags;
>>
>>      struct vkd3d_shader_descriptor_binding binding;
>>  };
> Somewhat similarly, I think it makes more sense to describe this in
> terms of mapping between the shader and its target environment. I.e.,
> the vkd3d_shader_interface_info structure performs a function similar
> to the root signature in d3d12.
> 
>> +/**
>> + * Describes the mapping of a DXBC resource-sampler pair to a SPIR-V combined
>> + * sampler (i.e. sampled image). This structure is used in struct
>> + * vkd3d_shader_interface_info.
>> + */
>>  struct vkd3d_shader_combined_resource_sampler
>>  {
>> +    /**
>> +     * Register space of the DXBC resource. Ifthe source format does not
>> +     * support multiple register spaces, this parameter must be set to 0.
>> +     */
>>      unsigned int resource_space;
> "If the"
> 
>> +/**
>> + * Describes the mapping of a single Direct3D push constant buffer to SPIR-V.
>> + * This structure is used in struct vkd3d_shader_interface_info.
>> + */
>>  struct vkd3d_shader_push_constant_buffer
>>  {
>> +    /**
>> +     * Register space of the Direct3D descriptor. If the source format does not
>> +     * support multiple register spaces, this parameter must be set to 0.
>> +     */
>>      unsigned int register_space;
>> +    /** Register index of the DXBC descriptor. */
>>      unsigned int register_index;
>>      enum vkd3d_shader_visibility shader_visibility;
>>
>> -    unsigned int offset; /* in bytes */
>> -    unsigned int size;   /* in bytes */
>> +    /** Offset, in bytes, in the SPIR-V push constant buffer. */
>> +    unsigned int offset;
>> +    /** Size, in bytes. */
>> +    unsigned int size;
>>  };
>>
> This maps a range of push constants to a constant buffer.
> 
>> +/**
>> + * A chained structure describing the correspondence between source and target
>> + * bindings of a compiled shader. This structure consists of several similar
>> + * arrays of structures, each element of which describes a single mapping from
>> + * a DXBC binding descriptor to a SPIR-V binding descriptor.
>> + *
> Like above, think of it as describing the interface between the shader
> (either source or target) and the target environment. (In some ways
> that's equivalent to describing a mapping between the source and
> target shaders of course, but the specifics depend on the target
> shader type.)

I might be overthinking, but describing it in such terms seems a little
too abstract, such that it's not clear what exactly should get put in
this structure. I'll try to come up with a more detailed compromise...

Incidentally, the terminology used in dependent structures seems pretty
closely tied to DXBC -> SPIR-V, and so I was inclined to reflect that in
the documentation. I'm not sure if there's an intent for this structure
to be more generic (aside from DXBC -> GLSL, it's not obvious to me what
it'd be used for, unless we're for some reason interested in compiling
to DXBC or between GLSL and SPIR-V), but perhaps using different field
names would help (e.g. not differentiating "space" and "set", or "index"
and "binding").

> 
>> + * This structure is optional. If omitted, vkd3d_shader_compile() will behave
>> + * as if no descriptors had been passed.
>> + *
> Actually, it will use a default mapping; see the "done:" label in
> vkd3d_dxbc_compiler_get_descriptor_binding(). It will also do that
> when the binding count is 0, which we should probably fix.
> 
>> + * This structure extends vkd3d_shader_compile_info.
>> + *
>> + * This structure contains only input parameters.
>> + */
>>  struct vkd3d_shader_interface_info
>>  {
>> +    /** Must be set to VKD3D_SHADER_STRUCTURE_TYPE_INTERFACE_INFO. */
>>      enum vkd3d_shader_structure_type type;
>> +    /** Optional pointer to a structure containing further parameters. */
>>      const void *next;
>>
>> +    /** An array of bindings for shader resource descriptors. */
>>      const struct vkd3d_shader_resource_binding *bindings;
>> +    /** Size, in elements, of \ref bindings. */
>>      unsigned int binding_count;
>>
>> +    /** An array of bindings for push constant buffers. */
>>      const struct vkd3d_shader_push_constant_buffer *push_constant_buffers;
>> +    /** Size, in elements, of \ref push_constant_buffers. */
>>      unsigned int push_constant_buffer_count;
>>
>> +    /** An array of bindings for combined samplers. */
>>      const struct vkd3d_shader_combined_resource_sampler *combined_samplers;
>> +    /** Size, in elements, of \ref combined_samplers. */
>>      unsigned int combined_sampler_count;
>>
>> +    /** An array of bindings for UAV counters. */
>>      const struct vkd3d_shader_uav_counter_binding *uav_counters;
>> +    /** Size, in elements, of \ref uav_counters. */
>>      unsigned int uav_counter_count;
>>  };
> "pointer to an array"
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200827/4ac4dab7/attachment.sig>