Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MVKDevice: Correct some required limits. #2076

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

cdavis5e
Copy link
Collaborator

If the wideLines feature isn't supported, then lineWidthGranularity
must be zero.

If the fullDrawIndexUint32 feature is supported, then
maxDrawIndexedIndexValue must be UINT32_MAX. I had originally done
this when I turned the feature on, but for a while now, we've been
setting it to one less, because primitive restart can't be disabled and
the value is defined to exclude primitive restart.

The wording in the spec is ambiguous. The description of
maxDrawIndexedIndexValue says:

  • maxDrawIndexedIndexValue is the maximum index value that can
    be used for indexed draw calls when using 32-bit indices. This
    excludes the primitive restart index value of 0xFFFFFFFF.
    [emphasis
    added]

But the description of fullDrawIndexUint32 says:

  • fullDrawIndexUint32 specifies the full 32-bit range of indices is
    supported for indexed draw calls when using a VkIndexType of
    VK_INDEX_TYPE_UINT32. maxDrawIndexedIndexValue is the maximum
    index value that may [sic] be used (aside from the primitive
    restart index, which is always 232-1 when the VkIndexType
    is VK_INDEX_TYPE_UINT32)
    . If this feature is supported,
    maxDrawIndexedIndexValue must be 232-1; otherwise
    it must be no smaller than 224-1. [emphasis added]

It's unclear whether it means that the primitive restart index is to be
ignored, or the maximum draw index must account for it.

The alternative is to disable fullDrawIndexUint32 because we cannot
set maxDrawIndexedIndexValue to UINT32_MAX; but that might mislead
programs into thinking that we only support 24-bit vertex indices.

Fixes the dEQP-VK.info.device_properties test.

If the `wideLines` feature isn't supported, then `lineWidthGranularity`
must be zero.

Fixes one part of the `dEQP-VK.info.device_properties` CTS test.
If the `fullDrawIndexUint32` feature is supported, then
`maxDrawIndexedIndexValue` must be `UINT32_MAX`. I had originally done
this when I turned the feature on, but for a while now, we've been
setting it to one less, because primitive restart can't be disabled and
the value is defined to exclude primitive restart.

The wording in the spec is ambiguous. The description of
`maxDrawIndexedIndexValue` says:

> * `maxDrawIndexedIndexValue` is the maximum index value that **can**
>   be used for indexed draw calls when using 32-bit indices. *This
>   excludes the primitive restart index value of 0xFFFFFFFF.* [emphasis
>   added]

But the description of `fullDrawIndexUint32` says:

> * `fullDrawIndexUint32` specifies the full 32-bit range of indices is
>   supported for indexed draw calls when using a VkIndexType of
>   `VK_INDEX_TYPE_UINT32`. `maxDrawIndexedIndexValue` is the maximum
>   index value that **may** [sic] be used *(aside from the primitive
>   restart index, which is always 2<sup>32</sup>-1 when the VkIndexType
>   is `VK_INDEX_TYPE_UINT32`)*. If this feature is supported,
>   `maxDrawIndexedIndexValue` **must** be 2<sup>32</sup>-1; otherwise
>   it **must** be no smaller than 2<sup>24</sup>-1. [emphasis added]

It's unclear whether it means that the primitive restart index is to be
ignored, or the maximum draw index must account for it.

The alternative is to disable `fullDrawIndexUint32` because we cannot
set `maxDrawIndexedIndexValue` to `UINT32_MAX`; but that might mislead
programs into thinking that we only support 24-bit vertex indices.

Fixes the rest of the `dEQP-VK.info.device_properties` test.
@billhollings
Copy link
Contributor

I'm confused.

As you mention, for fullDrawIndexUint32, the spec says:

If this feature is supported, maxDrawIndexedIndexValue must be 232-1

This should mean we are doing it correctly already. Where does the conflicting requirement that maxDrawIndexedIndexValue must be UINT32_MAX, come from? If it's coming from the CTS test, could the test be in error?

@cdavis5e
Copy link
Collaborator Author

cdavis5e commented Nov 23, 2023

I'm confused.

As you mention, for fullDrawIndexUint32, the spec says:

If this feature is supported, maxDrawIndexedIndexValue must be 232-1

This should mean we are doing it correctly already. Where does the conflicting requirement that maxDrawIndexedIndexValue must be UINT32_MAX, come from? If it's coming from the CTS test, could the test be in error?

UINT32_MAX is 232-1. 232 is 0x100000000 (one followed by eight zeroes in hex), which cannot fit in a 32-bit integer. We were setting it to 232-2, i.e. UINT32_MAX-1..

@billhollings billhollings merged commit 88c9176 into KhronosGroup:main Nov 23, 2023
5 checks passed
@billhollings
Copy link
Contributor

UINT32_MAX is 232-1. 232 is 0x100000000 (one followed by eight zeroes in hex), which cannot fit in a 32-bit integer. We were setting it to 232-2, i.e. UINT32_MAX-1.

Well! I guess that was my official math-brain-fart for this week! Of course you are correct. 🤦🏻‍♂️😂

@cdavis5e cdavis5e deleted the required-limits branch November 23, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants