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

First Timeline Semaphore use #158

Open
wants to merge 17 commits into
base: development
Choose a base branch
from
Open

Conversation

MoritzRoth
Copy link
Collaborator

The first step towards full timeline semaphore support. Related Issue: #45

Changes:

  • Adds and implements context_vulkan::record_and_submit_with_timeline_semaphore
  • Replaces fences in model_loader example with timeline semaphores.

@MoritzRoth MoritzRoth self-assigned this Mar 30, 2023
Copy link
Member

@johannesugb johannesugb left a comment

Choose a reason for hiding this comment

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

Cool stuff! Please mind the comments and update according to the requested changes in the Auto-Vk PR!

@@ -186,6 +186,7 @@ namespace avk
.setShaderUniformTexelBufferArrayDynamicIndexing(VK_TRUE)
.setShaderStorageTexelBufferArrayDynamicIndexing(VK_TRUE)
.setDescriptorIndexing(VK_TRUE)
.setTimelineSemaphore(VK_TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be enabled by default but instead, enabled through the std::function<void(vk::PhysicalDeviceVulkan12Features&)> parameter that can be passed to configure_and_compose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 170 to 190
/** \brief Records and submits commands to a queue together with a newly created timeline semaphore, which is signalled upon completion.
* \param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* \param aQueue The queue to submit the commands to.
* \param aSrcSignalStage Defines in which stage it is safe to signal the created timeline semaphore.
* \param aSignalValue The value to SIGNAL the timeline semaphore to.
* \param aInitialValue (optional) The value to INITIALIZE the newly created timeline semaphore with.
* \param aUsageFlags (optional) CommandBuffer usage flags.
* \return The newly created timeline semaphore.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** \brief Records and submits commands to a queue together with a newly created timeline semaphore, which is signalled upon completion.
* \param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* \param aQueue The queue to submit the commands to.
* \param aSrcSignalStage Defines in which stage it is safe to signal the created timeline semaphore.
* \param aSignalValue The value to SIGNAL the timeline semaphore to.
* \param aInitialValue (optional) The value to INITIALIZE the newly created timeline semaphore with.
* \param aUsageFlags (optional) CommandBuffer usage flags.
* \return The newly created timeline semaphore.
*/
/** @brief Records and submits commands to a queue together with a newly created timeline semaphore, which is signaled upon completion.
* @param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* @param aQueue The queue to submit the commands to.
* @param aSrcSignalStage Defines in which stage(s) after which is safe to signal the created timeline semaphore.
* @param aSignalValue The value to signal the timeline semaphore to.
* @param aInitialValue (optional) The value to initialize the newly created timeline semaphore with.
* @param aUsageFlags (optional) CommandBuffer usage flags.
* @return The newly created timeline semaphore.
*/

Please use @ instead of \, and please try to horizontally align all the @.
Also, please use spaces to align the comments!

Please mind the comment fixes!

And "signaled" is written in a wrong way multiple times, I'm afraid (also before this PR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.submit();

aSignalInfo.mSignalSemaphore.get().handle_lifetime_of(std::move(cmdBfr));
// TODO we probably need another way to handle the lifetime of resources since timeline semaphores aren't single use ==> live longer
Copy link
Member

Choose a reason for hiding this comment

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

That should actually be okay, because the (timeline) semaphore takes care of the ownership of the command buffer.
Or what was your argument in this case exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I was talking about here was that since timeline semaphores are no longer single-shot synchronization primitives (1 signal, 1 wait) they might have a lifetime close to the whole application runtime. If a command buffer now gets enqueued and executed early on in the semaphore's lifespan, won't it be kept alive until the semaphore is destroyed, way longer than it needs to exist?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah. Now I get the point. You're completely right.

Hmm, the right way to do this would probably be to turn std::vector<any_owning_resource_t> into std::vector<std::tuple<any_owning_resource_t, uint64_t>>, where the uint64_t item represents the timeline semaphore's value after which it is safe to delete the resource.

A similar approach is implemented for command buffers, see the following code:

	// prepare command buffer for re-recording
	void command_buffer_t::prepare_for_reuse()
	{
		if (mPostExecutionHandler.has_value()) {
			// Clear post-execution handler
			mPostExecutionHandler.reset();
		}
		if (mCustomDeleter.has_value() && *mCustomDeleter) {
			// If there is a custom deleter => call it now
			(*mCustomDeleter)();
			mCustomDeleter.reset();
		}
		mLifetimeHandledResources.clear();
	}

Just for timeline semaphores, it would not be a mLifetimeHandledResources.clear();, but a clear depending on the uint64_t value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the most recent changes, I added an optional value parameter to semaphore_t::handle_lifetime_of. Here I just pass the same value the semaphore will be signaled to, once the work has finished.

I suspect this is wrong in cases where the passed aSignalInfo uses a stage other than all_commands. Do you think we should add another param for a cmdBuffer deletion value, or just let the caller setup the lifetime handling themselves?

std::move(posFillCmd),
std::move(tcoFillCmd),
std::move(nrmFillCmd),
std::move(idxFillCmd)
// ^ No need for any synchronization in-between, because the commands do not depend on each other.
}, *mQueue);
}, * mQueue, { vk::PipelineStageFlagBits2::eAllTransfer }, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, * mQueue, { vk::PipelineStageFlagBits2::eAllTransfer }, 1);
}, *mQueue, avk::stage::all_transfer, 1);

Better in terms of conciseness and formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

std::move(materialCommands),
mMaterialBuffer->fill(gpuMaterials.data(), 0)
}, *mQueue);
matFence->wait_until_signalled();
}, *mQueue, {vk::PipelineStageFlagBits2::eAllCommands}, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, *mQueue, {vk::PipelineStageFlagBits2::eAllCommands}, 1);
}, *mQueue, avk::stage::all_commands, 1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MoritzRoth MoritzRoth changed the base branch from master to development July 5, 2023 18:57
Copy link
Member

@johannesugb johannesugb left a comment

Choose a reason for hiding this comment

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

Let's review again after the requested changes in Auto-Vk!

Comment on lines +183 to +190
* @param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* @param aQueue The queue to submit the commands to.
* @param aSrcSignalStage Defines in which stage it is safe to signal the created timeline semaphore.
* @param aSignalValue The value to SIGNAL the timeline semaphore to.
* @param aInitialValue (optional) The value to INITIALIZE the newly created timeline semaphore with.
* @param aUsageFlags (optional) CommandBuffer usage flags.
* @return The newly created timeline semaphore.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* @param aQueue The queue to submit the commands to.
* @param aSrcSignalStage Defines in which stage it is safe to signal the created timeline semaphore.
* @param aSignalValue The value to SIGNAL the timeline semaphore to.
* @param aInitialValue (optional) The value to INITIALIZE the newly created timeline semaphore with.
* @param aUsageFlags (optional) CommandBuffer usage flags.
* @return The newly created timeline semaphore.
*/
* @param aRecordedCommandsAndSyncInstructions List of commands to record and submit.
* @param aQueue The queue to submit the commands to.
* @param aSrcSignalStage Defines in which stage it is safe to signal the created timeline semaphore.
* @param aSignalValue The value to SIGNAL the timeline semaphore to.
* @param aInitialValue (optional) The value to INITIALIZE the newly created timeline semaphore with.
* @param aUsageFlags (optional) CommandBuffer usage flags.
* @return The newly created timeline semaphore.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Please add spaces before the *, so that all * are vertically aligned!

.submit();

aSignalInfo.mSignalSemaphore.get().handle_lifetime_of(std::move(cmdBfr));
// TODO we probably need another way to handle the lifetime of resources since timeline semaphores aren't single use ==> live longer
Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah. Now I get the point. You're completely right.

Hmm, the right way to do this would probably be to turn std::vector<any_owning_resource_t> into std::vector<std::tuple<any_owning_resource_t, uint64_t>>, where the uint64_t item represents the timeline semaphore's value after which it is safe to delete the resource.

A similar approach is implemented for command buffers, see the following code:

	// prepare command buffer for re-recording
	void command_buffer_t::prepare_for_reuse()
	{
		if (mPostExecutionHandler.has_value()) {
			// Clear post-execution handler
			mPostExecutionHandler.reset();
		}
		if (mCustomDeleter.has_value() && *mCustomDeleter) {
			// If there is a custom deleter => call it now
			(*mCustomDeleter)();
			mCustomDeleter.reset();
		}
		mLifetimeHandledResources.clear();
	}

Just for timeline semaphores, it would not be a mLifetimeHandledResources.clear();, but a clear depending on the uint64_t value.

MoritzRoth and others added 10 commits February 27, 2024 18:06
…ng directory, s.t. assets and shaders can be found when run from within VS.
add particle + metadata structs
add compute shader skeleton
add compute shader queue and setup pipeline (still unused atm)
create particle buffer, intermediate buffer and vertex buffers in init()
…ctly with the particle buffer i intend to use

add a temporary uniform mHostParticleBuffer and use it to draw prticles with mRenderPipeline
adjust a_triangle.vert to use the particleBuffer
compute shader still runs on tha same queue directly before each frame
…ll handle_lifetime_of, with the value the semaphore will be signaled to at the end of execution
…ate thread

add slider modifying physics refresh rate
keep workgroup dimensions at 1 for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants