search

KhronosGroup / Vulkan-Portability

Apache License 2.0
40 stars 4 forks source link

Proposed VK_KHR_portability_subset header file changes for final #36

Open billhollings opened 2 years ago

billhollings commented 2 years ago

This issue is to track the header file changes as we move VK_KHR_portability_subset extension from provisional to final.

billhollings commented 2 years ago

2020 Provisional:

typedef struct VkPhysicalDevicePortabilitySubsetFeaturesKHR {
    VkStructureType    sType;
    void*              pNext;
    VkBool32           constantAlphaColorBlendFactors;
    VkBool32           events;
    VkBool32           imageViewFormatReinterpretation;
    VkBool32           imageViewFormatSwizzle;
    VkBool32           imageView2DOn3DImage;
    VkBool32           multisampleArrayImage;
    VkBool32           mutableComparisonSamplers;
    VkBool32           pointPolygons;
    VkBool32           samplerMipLodBias;
    VkBool32           separateStencilMaskRef;
    VkBool32           shaderSampleRateInterpolationFunctions;
    VkBool32           tessellationIsolines;
    VkBool32           tessellationPointMode;
    VkBool32           triangleFans;
    VkBool32           vertexAttributeAccessBeyondStride;
} VkPhysicalDevicePortabilitySubsetFeaturesKHR;

typedef struct VkPhysicalDevicePortabilitySubsetPropertiesKHR {
    VkStructureType    sType;
    void*              pNext;
    uint32_t           minVertexInputBindingStrideAlignment;
} VkPhysicalDevicePortabilitySubsetPropertiesKHR;
billhollings commented 2 years ago

Proposed Final (based on minimum macOS 11.0 and iOS 14.0, and active portability issues and CTS conformance level):

typedef struct VkPhysicalDevicePortabilitySubsetFeaturesKHR {
    VkStructureType    sType;
    void*              pNext;
    VkBool32           imageView2DOn3DImage;
    VkBool32           nonUniformMemoryBarriers;
    VkBool32           pointPolygons;
    VkBool32           samplerMipLodBias;
    VkBool32           shaderImageGatherExtended;
    VkBool32           shaderSampleRateInterpolationFunctions;
    VkBool32           tessellationIsolines;
    VkBool32           tessellationPointMode;
    VkBool32           triangleFans;
    VkBool32           workgroupMatrices;
} VkPhysicalDevicePortabilitySubsetFeaturesKHR;

VkPhysicalDevicePortabilitySubsetPropertiesKHR:

VkPhysicalDevicePortabilitySubsetPropertiesKHR:

kvark commented 2 years ago

Removed imageViewFormatSwizzle is covered by native swizzle on all but older IM GPU's where it is covered by shader swizzling.

Does that mean you are automatically opting into slow path on those older GPUs? They would run slow because they are old, and this change would make them run even slower, likely unnecessary (if the user doesn't need swizzling).

A bigger issue here is that swizzling semantics is different between Metal and Vulkan. Vulkan:

This remapping must be the identity swizzle for storage image descriptors, input attachment descriptors, framebuffer attachments

Metal:

The pattern you want the GPU to apply to pixels when you read or sample pixels from the texture.

The key difference is "framebuffer attachments". Vulkan swizzle affects them, but Metal swizzle doesn't. I don't think MoltenVK is handling this well currently, is it?

billhollings commented 2 years ago

Does that mean you are automatically opting into slow path on those older GPUs? They would run slow because they are old, and this change would make them run even slower, likely unnecessary (if the user doesn't need swizzling).

Well...in this case the spec would assume it is available. It would then be up to an implementation to activate it or not by default, or through config. This kind of conformance/performance default choice trade-off for any particular implementation might apply to other features too...like tessellation, etc. I guess we could talk about whether in general we want to include flags that talk about performance behavior (ie. "shaderSwizzle", "computedTessellation", "emulatedSemaphores"), but it might be hard to know where to draw the line on that.

This remapping must be the identity swizzle for storage image descriptors, input attachment descriptors, framebuffer attachments

Isn't this saying that framebuffers are not swizzled in Vulkan (ie. "identity swizzle")?

billhollings commented 2 years ago

Proposed Final after Triage (based on minimum macOS 11.0 and iOS 14.0, plus):

typedef struct VkPhysicalDevicePortabilitySubsetFeaturesKHR {
    VkStructureType    sType;
    void*              pNext;
    VkBool32           borderColorOpaqueBlackR4G4B4A4;
    VkBool32           borderColorOpaqueBlackR5G5B5A1;
    VkBool32           depthClampDeltaOneBiasClampNegative;
    VkBool32           depthClampMultipleViewport;
    VkBool32           imageView2DOn3DImage;
    VkBool32           nonUniformMemoryBarriers;
    VkBool32           pointPolygons;
    VkBool32           samplerMipLodBias;
    VkBool32           samplingExplicitLODRepeatingEdgeAccuracyConformance;
    VkBool32           samplingImplicitLODProjectedGeometryAccuracyConformance;
    VkBool32           shaderAtomicOperationLocal;
    VkBool32           shaderDuplicateSpecId;
    VkBool32           shaderEarlyFragmentTestDiscardNoTestsStencil;
    VkBool32           shaderEarlyFragmentTestMultisampleDepth;
    VkBool32           shaderFragmentDepth32FloatMultisample;
    VkBool32           shaderFragmentSamplerLod1DArrayShadow;
    VkBool32           shaderGraphicsFuzzConformance;
    VkBool32           shaderImageGatherExtended;
    VkBool32           shaderIntCubeSampler;
    VkBool32           shaderNoPositionC0E1;
    VkBool32           shaderSampleRateInterpolationFunctions;
    VkBool32           shaderSharedVariableReleaseAcquire;
    VkBool32           shaderSpecConstantOpCompositeInsert;
    VkBool32           shaderSPIRVAssemblyConformance;
    VkBool32           shaderTessellationInputOutputBarrier;
    VkBool32           shaderTessellationInputOutputNesting;
    VkBool32           subpassDependencyLateFragmentTests;
    VkBool32           tessellationIsolines;
    VkBool32           tessellationPointMode;
    VkBool32           triangleFans;
    VkBool32           workgroupMatrices;
} VkPhysicalDevicePortabilitySubsetFeaturesKHR;

VkPhysicalDevicePortabilitySubsetFeaturesKHR:

For borderColorOpaqueBlackR4G4B4A4 and borderColorOpaqueBlackR5G5B5A1, I looked for a single flag to cover both. The next abstraction level might be borderColorOpaqueBlackPacked16, but that would include R5G6B5, which passes all opaque black border tests, and using that flag, a user would exclude that validly-performing format.

billhollings commented 2 years ago

This spreadsheet summarizes the requirements for the capability flags, and categorizes them into the following categories:

billhollings commented 2 years ago

We need to address whether the flags should be positive feature flags indicating feature supported if true (eg. triangleFans), or negative feature flags indicating feature supported if false (eg. disableTriangleFans).

The question arises about what the feature flags should default to if an implementation does not support the VK_KHR_portability_subset extension, but the app submits the VkPhysicalDevicePortabilitySubsetFeaturesKHR structure anyway.

If the structure is submitted to an implementation that does not support it, it come back unchanged.

Most Vulkan extension add functionality, and therefore if a zeroed structure is submitted and the extension is not supported, the flags will all come back false, indicating the added functionality is not present.

However, a zeroed VkPhysicalDevicePortabilitySubsetFeaturesKHR structure that came back unchanged would similarly indicate that the features are not supported. However, the most common case of this happening would be on a fully-conformant implementation that does not need to support the extension, and the app would therefore falsely interpret the unchanged zeroed structure as indicating the implementation does not support any of the features, when in reality it does.

Changing the structure to a negative feature values would stop this from happening. For instance an unchanged zeroed disableTriangleFans flag would correctly indicate that the conformant implementation does in fact support triangle fans, as expected.

billhollings commented 2 years ago

@charles-lunarg Can you comment on the spec fidelity on the latest question about having the portability spec explicitly indicate what is being disabled, as opposed to passively disabling by not enabling a feature flag.

charles-lunarg commented 2 years ago

This essentially is asking whether we should work around applications that do not check if an extension is available and enabled before using extension structures. Applications should be easily able to tell if the VK_KHR_portability_subset extension is on a device with vkEnumerateDeviceExtensions.

If an implementation doesn't support VK_KHR_portability_subset and the application passes VkPhysicalDevicePortabilitySubsetFeaturesKHR into the implementation, then uses the (unchanged) struct somehow, that's on the application for expecting those fields to have any sort of meaning.

I don't think we should be worrying about this case.