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

Change detection of seperate stencil usage #2193

Merged
merged 1 commit into from Apr 25, 2023

Conversation

Fighter19
Copy link
Contributor

@Fighter19 Fighter19 commented Apr 24, 2023

Fix crash on Vivante GPU (#2192)

  1. Update documentation to reflect any user-facing changes - in this repository.
    N/A

  2. Make sure that the changes are covered by unit-tests.

  3. Run cargo fmt on the changes.

  4. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  5. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

See issue #2192.
Commit b968451 changed the way has_separate_stencil_usage is being determined, leading to it being evaluated to true, even when it was formerly false.
This change reverts that aspect of the commit.

has_seperate_stencil usage will be included when stencil_usage is different from usage.
This likely is a bug that leads to a ImageStencilUsageCreateInfo being passed with a COLOR_ATTACHMENT bit being set, that is rejected by the Vivante GPU.

Changelog:

### Bugs fixed
- Crash of application during start on i.MX board due to vkGetPhysicalDeviceImageFormatProperties2KHR returning VK_ERROR_FORMAT_NOT_SUPPORTED.

@marc0246 marc0246 linked an issue Apr 24, 2023 that may be closed by this pull request
@Rua
Copy link
Contributor

Rua commented Apr 24, 2023

I don't know if this is the correct solution. The purpose of the original code is to treat these different cases as equivalent. Now they no longer are equivalent for the purposes of caching. Can you elaborate more on why this fix works?

@Fighter19
Copy link
Contributor Author

I can imagine that the solution is flawed, as I am not entirely aware of how the caching nor Vulkan works.
I am not aware for example, why specifying the COLOR_ATTACHMENT bit is supposedly allowed for the stencil,
and why copying all of the bits is even valid. (I personally would have assumed, that COLOR_ATTACHMENT needs to be cleared and DEPTH_STENCIL_ATTACHMENT set).
I suppose my change does revert the benefit of treating both cases as the same.

At the same time it fixes the issue with has_seperate_stencil_usage always evaluating to true (in case stencil_usage.is_empty() || !aspects.contains(ImageAspects::DEPTH | ImageAspects::STENCIL)) and therefore adding a ImageStencilUsageCreateInfo to p_next, which the GPU or driver rejects.

In order to provide a better fix (optimally one that preferably omits the extra struct),
I'd need to know, if the way ImageStencilUsageCreateInfo is initialized is actually valid (passing the color attachment bit).

@Rua
Copy link
Contributor

Rua commented Apr 24, 2023

Looking again, I think the bug is just line 1070 having the condition reversed? It looks like it should be !=.

@Fighter19
Copy link
Contributor Author

Fighter19 commented Apr 24, 2023

I think so too. I will try again using that.

I also found out that apparently the GPU rejects COLOR_ATTACHMENT being set, setting DEPTH_STENCIL_ATTACHMENT instead made the call succeed.

I temporarily tried the following:

*stencil_usage = (*usage).difference(ImageUsage::COLOR_ATTACHMENT).union(ImageUsage::DEPTH_STENCIL_ATTACHMENT);

Maybe instances in which COLOR_ATTACHMENT is passed to ImageStencilUsageCreateInfo could be caught, as not all GPUs see an issue with getting COLOR_ATTACHMENT passed, apparently?

EDIT: The call also succeeds when inverting the statement.

@Rua
Copy link
Contributor

Rua commented Apr 24, 2023

If the GPU rejects certain combinations, it must have its reasons for that, surely? Not everything has to be supported. In the case of attachments, it probably depends on the format you're using: creating a color attachment with a depth/stencil format makes no sense, and vice versa.

Fix crash on Vivante GPU (vulkano-rs#2192) by properly
determining when to include seperate stencil usage.
@Fighter19 Fighter19 changed the title Revert determination of seperate stencil usage Change detection of seperate stencil usage Apr 25, 2023
@Rua Rua merged commit bb7990f into vulkano-rs:master Apr 25, 2023
3 checks passed
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
Fix crash on Vivante GPU (vulkano-rs#2192) by properly
determining when to include seperate stencil usage.
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.

i.MX8: image_format_properties_unchecked returns None
2 participants