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.

Sign up for GitHub

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

Some more suspicious swapchain rfc keyword usage #1680

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Some more suspicious swapchain rfc keyword usage #1680

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0329fb3
Update wsi.txt
stonesthrow Nov 8, 2021
5c24ec7
Update swapchain_counters.txt
stonesthrow Nov 8, 2021
41150d5
Update wsi.txt
stonesthrow Nov 8, 2021
e36c231
vkGetSwapchainStatusKHR codes
stonesthrow Dec 6, 2021
f12c413
Removed as-usual text
stonesthrow Dec 6, 2021
32416d0
Merge branch 'KhronosGroup:main' into Some-more-suspicious-swapchain-…
stonesthrow Dec 6, 2021
6cdf707
Change Status code line
stonesthrow Feb 4, 2022
b64bd1f
Reword return from retired oldSwapchain on vkQueuePresent
stonesthrow Feb 4, 2022
af04900
Merge branch 'KhronosGroup:main' into Some-more-suspicious-swapchain-…
stonesthrow Jun 17, 2022
ceb4ba8
Merge branch 'KhronosGroup:main' into Some-more-suspicious-swapchain-…
stonesthrow Jul 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chapters/VK_EXT_display_control/swapchain_counters.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ include::{generated}/api/protos/vkGetSwapchainCounterEXT.txt[]
* pname:pCounterValue will return the current value of the counter.

If a counter is not available because the swapchain is out of date, the
implementation may: return ename:VK_ERROR_OUT_OF_DATE_KHR.
implementation must: return ename:VK_ERROR_OUT_OF_DATE_KHR.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we drafted the OUT_OF_DATE language, some IHVs were uncomfortable with requiring errors in states that are defined as OUT_OF_DATE, and carry on as if things are fine instead. This language is fine with me, but other driver vendors may still object to such requirements.

Copy link
Contributor Author

@stonesthrow stonesthrow Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Good history. The issue by is KrO0ze identifying non-exact language. Do we know who objected? see if they reaffirm stance or not.

Copy link
Contributor

@krOoze krOoze Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of code relies on mustiness of this. So if you preserve "may", it needs some serious acompanying note explaining what exactly that implies as well as what is the correct way to deal with swapchain state.

I can imagine though the failure could be allowed to be deferred. E.g. acquire pretends success, and then it fails at present. Still needs to be copiously noted in spec if that can happen and explicitly how far the driver can push that leeway.

Copy link
Contributor Author

@stonesthrow stonesthrow Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cubanismo - lets see if we can craft some text that will work here.

Copy link
Contributor

@krOoze krOoze Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if resolved per @cubanismo comment, but I certainly am happy with "must"...


.Valid Usage
****
Expand Down
3 changes: 1 addition & 2 deletions chapters/VK_KHR_shared_presentable_image/wsi.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ include::{generated}/api/protos/vkGetSwapchainStatusKHR.txt[]
include::{generated}/validity/protos/vkGetSwapchainStatusKHR.txt[]
--

The possible return values for fname:vkGetSwapchainStatusKHR should: be
interpreted as follows:
The meanings of status codes returned by flink:vkGetSwapchainStatusKHR are:
stonesthrow marked this conversation as resolved.
Show resolved Hide resolved

* ename:VK_SUCCESS specifies the presentation engine is presenting the
contents of the shared presentable image, as per the swapchain's
Expand Down
2 changes: 1 addition & 1 deletion chapters/VK_KHR_swapchain/wsi.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ flink:vkQueuePresentKHR any images it had already acquired from
pname:oldSwapchain.
E.g., an application may present an image from the old swapchain before an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is "just" a note, though I would still avoid unmarkupped RFC terminology. Should be "can:".

image from the new swapchain is ready to be presented.
As usual, flink:vkQueuePresentKHR may: fail if pname:oldSwapchain has
flink:vkQueuePresentKHR must: fail if pname:oldSwapchain has
entered a state that causes ename:VK_ERROR_OUT_OF_DATE_KHR to be returned.
Copy link
Contributor

@krOoze krOoze Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not fix the full extent of the complaint in the original Issue.

The problem is also the sentence is tautological. If VK_ERROR_OUT_OF_DATE_KHR was returned, then by definition it did fail (no may or must about it).

I think the intention of the sentence is the following (and so should the wording match that)

Retired oldSwapchain may: still become unusable as much as non-retired swapchain. When that happens, it is treated identically by vkQueuePresentKHR; the command must return VK_ERROR_OUT_OF_DATE_KHR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded.


ifdef::VK_KHR_shared_presentable_image[]
Expand Down