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

Jump to bottom

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

wants to merge 10 commits into from

Conversation

stonesthrow
Copy link
Contributor

Closes #1586

@stonesthrow
Update wsi.txt
0329fb3 
Change "should be interpreted as follows" to " indicates the following"
@stonesthrow
Update swapchain_counters.txt
5c24ec7 
"may" change to "must"
@stonesthrow
Update wsi.txt
41150d5 
"may" change to "must"
@stonesthrow stonesthrow added this to the Needs Approval milestone Nov 8, 2021
@llandwerlin-intel
Copy link

Looks good.

@linyaa-kiwi
Copy link
Contributor

lgtm

cubanismo
cubanismo reviewed Jan 19, 2022
View reviewed changes
chapters/VK_EXT_display_control/swapchain_counters.txt
@@ -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"...

krOoze
krOoze suggested changes Jan 20, 2022
View reviewed changes
chapters/VK_KHR_shared_presentable_image/wsi.txt Outdated Show resolved Hide resolved
chapters/VK_KHR_swapchain/wsi.txt Outdated
@@ -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
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.

chapters/VK_KHR_swapchain/wsi.txt Outdated
@@ -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:".

krOoze
krOoze reviewed Jul 7, 2022
View reviewed changes
chapters/VK_EXT_display_control/swapchain_counters.txt
@@ -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
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"...

chapters/VK_KHR_swapchain/wsi.txt
image from the new swapchain is ready to be presented.
As usual, flink:vkQueuePresentKHR may: fail if pname:oldSwapchain has
entered a state that causes ename:VK_ERROR_OUT_OF_DATE_KHR to be returned.
pname:oldSwapchain. flink:vkQueuePresentKHR must: return ename:VK_ERROR_OUT_OF_DATE_KHR.
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.

This seems wrong, and contrary to the original text.

@linyaa-kiwi
Copy link
Contributor

@stonesthrow Is this MR still alive? It appears that everything is resolved except one comment by krooze.

@oddhack
Copy link
Contributor

oddhack commented Apr 13, 2023

@stonesthrow Is this MR still alive? It appears that everything is resolved except one comment by krooze.

@stonesthrow pinging on this PR again - would be nice to get it off the queue.

@stonesthrow
Copy link
Contributor Author

@oddhack Sorry, I just haven't had any bandwidth to look into this again. At this time I am swamped.
Maybe its close to complete. It was either this or another issue that the underlying text of the spec got a big change and so I was thinking this needed to re-investigated to be sure it changes were needed and correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oddhack oddhack oddhack left review comments

@krOoze krOoze krOoze requested changes

@cubanismo cubanismo cubanismo left review comments

Assignees
No one assigned
Labels
System Integration
Projects
None yet
Milestone
Needs Approval
Development

Successfully merging this pull request may close these issues.

Some more suspicious swapchain RFC keyword usage
6 participants
@stonesthrow @llandwerlin-intel @linyaa-kiwi @oddhack @cubanismo @krOoze