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

clarification on the meaning of VK_PIPELINE_STAGE_2_ALL_GRAPHICS_BIT #2187

Open
brakhane opened this issue Aug 6, 2023 · 9 comments · May be fixed by #2219
Open

clarification on the meaning of VK_PIPELINE_STAGE_2_ALL_GRAPHICS_BIT #2187

brakhane opened this issue Aug 6, 2023 · 9 comments · May be fixed by #2219

Comments

@brakhane
Copy link

brakhane commented Aug 6, 2023

§ 7.1.2 currently defines VK_PIPELINE_STAGE_2_ALL_GRAPHICS_BIT as

[...] specifies the execution of all graphics pipeline stages, and is equivalent to the logical OR of [list of bits]

At the same time, VK_PIPELINE_STAGE_2_ALL_COMMANDS_BIT is defined as (emphasis mine)

[...] specifies all operations performed by all commands supported on the queue it is used with.

While ALL_COMMANDS explicitely mentions that only supported features are included, ALL_GRAPHICS doesn't; furthermore, it is specified as being equivalent to a logical OR of all the specific bits. Since a stage mask must not contain unsupported features, this could be interpreted as that ALL_GRAPHICS cannot be used unless all specific features are supported by the device.

The validation layers do interpret ALL_GRAPHICS to only include supported features, which is most likely what is intended.

If that is the case, the wording should be changed to make clear that it's the logical OR of all supported bits.

@krOoze
Copy link
Contributor

krOoze commented Aug 7, 2023

Somewhat part of the class of problems covered with #533.

Annoyingly "supported" has a special meaning, and does not necessarily mean enabled.

Not sure the "supported" does any good for anything. Personally I would just drop it and assume all stages are included. If they are not supported™, then it is a no-op anyway.

Since a stage mask must not contain unsupported features, this could be interpreted as that ALL_GRAPHICS cannot be used unless all specific features are supported by the device.

IMO should not be interpreted this way. It is semantic equivalence, not value equivalence. If VU does not specifically say VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT is forbidden, then it is allowed. (And it should be "supported" in graphics queue)

@brakhane
Copy link
Author

brakhane commented Aug 8, 2023

The biggest issue for me is the inconsistency between the two wordings. ALL_GRAPHICS and ALL_COMMANDS behave similar in that they don't include disabled features, but yet they are described differently.

I had a bug where the validation layer complained about a bit being set despite not being supported when ALL_GRAPHICS was used, but not in another instance where ALL_COMMANDS were used; it turned out to be something completely unrelated, but the different wording seemed consistent with the seemingly different behavior of the two flags. If both descriptions had been similar, I would have realized much earlier that I'm on a wild goose chase.

@Tobski
Copy link
Contributor

Tobski commented Aug 30, 2023

The Vulkan WG are happy to make these consistent, given "supported" was supposed to be read as interpreted by @brakhane here; but @krOoze if you have some more precise wording suggestions we'd be happy to consider alternatives.

@krOoze
Copy link
Contributor

krOoze commented Sep 3, 2023

The reason in the first place that "supported" wording was omitted is more than likely that the list already only includes Graphics queue related bits. IF even one flag bit there was not supported (because of queue), then the whole thing is not supported. So I am not sure we are helping anything here...

@Tobski
A) I don't see how supporteness makes any difference conceptually, and therefore for simplicity sake would prefer to drop it in both cases. (Furthemore, for version compatibility sake, I would prefer to somehow explicitly state the list of bits these meta-bits represent can grow, which so far is only informally understood)

otherwise

B) I would suggest to be strictly explicit with it, but it gets messy, since several moving parts may be deciding what flags are actually "supported" (and what does that mean). I mean do we speak only of queue support? In that case stating the bits have to be supported for GRAPHICS flag is redundant as stated above in the first paragraph...

This path feels to me like a minefield. What flags are "supported" in Barrier context may differ conceptually from SubpassDependency context which further may differ from QFOT barrier context.

C) I guess we could solve it with extra level of indirection, and say something like "is equivalent to OR of those flag bits from the following list that are valid in the context in which parameter of this type is used"

@Tobski
Copy link
Contributor

Tobski commented Sep 5, 2023

Supported here is primarily intended to indicate both valid for the queue family, and valid stages for the implementation - e.g. you wouldn't get a mesh shader bit implied if the implementation didn't have support for mesh shading. That's why we're shortcutting it because it's a bit messy. If it weren't for that we'd just point at the table of "Supported pipeline stage flags" in the spec and call it good.

That's why we don't want to just drop it, because we need some way of saying "you're not getting invalid bits". We can probably do that in another way. Perhaps something as simple as, roughly, "all bits that would be valid to supply to this command", would work?

@krOoze
Copy link
Contributor

krOoze commented Sep 6, 2023

@Tobski Quite explicitly says "supported on queue", so the wording does not clearly imply any other kind of supportations you might hope it does.

Yes, playing around the word of "valid" should be better, per option C above. There's no one "this command" though, so need to be worded somewhat more generic.

BTW, adding this "valid" clause also additionally applies to VK_PIPELINE_STAGE_2_PRE_RASTERIZATION_SHADERS_BIT (contains bits for stages that may be disabled) and VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT (contains extension bit). Not sure about VK_PIPELINE_STAGE_2_VERTEX_INPUT_BIT. Possibly also to access flags like VK_ACCESS_MEMORY_*_BIT, VK_ACCESS_2_SHADER_READ_BIT, and VK_ACCESS_2_SHADER_WRITE_BIT (pre-emptively in case other kind of writes are added).

@Tobski
Copy link
Contributor

Tobski commented Sep 7, 2023

We'll shift things around to say something about validity instead then.

@Tobski
Copy link
Contributor

Tobski commented Sep 11, 2023

Please take a look at #2219

@brakhane
Copy link
Author

Looks good to me!

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 a pull request may close this issue.

3 participants