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

Fix bug where AttachmentImages can't be used in multiple descriptor sets at the same time #1562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

faulesocke
Copy link

@faulesocke faulesocke commented Apr 21, 2021

This fixes #1557 It seems to work just fine with all the tests I did.

  • Ran cargo fmt on the changes

@Rua
Copy link
Contributor

Rua commented Apr 21, 2021

A layout transition is a write operation, which means that concurrent access to the same image should not be allowed. Pipeline barriers protect against access on the same queue, but not another queue. The change you propose would introduce a data race if the image is concurrently accessed by another queue.

The lock_submit and check_image_access functions further down rely on the exclusive value to provide the correct locking for the image. If exclusive is false, then this locks the image only for reading, which allows a command buffer on another queue to be executed and access this image concurrently, without the insertion of a semaphore. If the first queue is actually writing to the image at that time, that's a synchronisation error.

@faulesocke
Copy link
Author

Oh. I wasn't aware that the exclusive flag is being used for cross-command buffer dependencies. I will think of a different fix.

@Eliah-Lakhin Eliah-Lakhin added PR: Review next Sunday This PR is planned for FIRST review on the nearest Sunday. and removed PR: Review next Sunday This PR is planned for FIRST review on the nearest Sunday. labels Apr 21, 2021
@Eliah-Lakhin
Copy link
Contributor

@faulesocke Please ping me when it's ready for the final review. Thanks!

@Eliah-Lakhin Eliah-Lakhin added the PR: Work in progress PR is not ready for final review yet label Apr 27, 2021
@Eliah-Lakhin
Copy link
Contributor

@faulesocke Any updates regarding this PR?

@faulesocke
Copy link
Author

@Eliah-Lakhin sorry I haven't had time for this and probably won't have time for another month or two.

@AustinJ235 AustinJ235 added the PR: Long Standing There were no updates from the Author for a long time. label Sep 4, 2021
@vE5li
Copy link
Contributor

vE5li commented Jul 12, 2022

Is there any update on this?

fayalalebrun added a commit to VideowindoW/vulkano that referenced this pull request Oct 20, 2022
This allows us to render the same storage image more than
once. Related: vulkano-rs#1562
@vE5li
Copy link
Contributor

vE5li commented Oct 28, 2022

@fayalalebrun Does the commit a03cfa0 mean this can be closed?

@fayalalebrun
Copy link
Contributor

@vE5li No, that's for a fork of Vulkano and doesn't address the underlying issue.

@vE5li
Copy link
Contributor

vE5li commented Oct 30, 2022

Ah, my bad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Long Standing There were no updates from the Author for a long time. PR: Work in progress PR is not ready for final review yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lots of conflicts with reused uniform buffers in 0.22
6 participants