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

Portable RT: Fixes for shader-binding-table assembly (sbt data) #1993

Merged
merged 9 commits into from
Jan 30, 2025

Conversation

fabian-lunarg
Copy link
Contributor

@fabian-lunarg fabian-lunarg commented Jan 27, 2025

  • adjust replacer compute-shaders to allow for pass-through of data
  • Drop GL_GOOGLE_include_directive from internal compute-shader, unused and caused issues on radv
  • rework num-addresses calculation, account for extra data following handles.
  • Use accelerationStructureCaptureReplay only when allocator supports opaque addresses
  • minor adjustments and fixes for edge-cases

this fixes portable replay (using -m rebind) of applications using shader-record buffers, sourcing data from shader-binding-tables.
in glsl e.g. layout(shaderRecordEXT, std430) buffer ShaderRecordBuffer{...}
in SPIRV: StorageClassShaderRecordBufferKHR

in presence of that feature, when re-assembling a new sbt, it's not sufficient to remap handles.
also the extra data needs to be copied/passed-through.
this is fixed in this PR.

the simplest example around is: https://github.com/SaschaWillems/Vulkan/tree/master/examples/raytracingsbtdata

also this PR fixes issues encountered with portable-Rt on radv

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 356754.

@fabian-lunarg fabian-lunarg force-pushed the fabian_work_on_sbt_data branch from 7074c8a to e136f23 Compare January 27, 2025 11:18
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 356756.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5935 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5935 passed.

{
auto* previous_pipeline = object_table_->GetVkPipelineInfo(
command_buffer_info->bound_pipelines.at(VK_PIPELINE_BIND_POINT_COMPUTE));
GFXRECON_ASSERT(previous_pipeline);
Copy link
Contributor

Choose a reason for hiding this comment

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

previous_pipeline MUST be valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. at this point we already checked the bound_pipelines map and know a compute-pipeline was previously bound (rare'ish case anyway, since the command-buffer is used to record a raytracing pipeline). so in fact, we only assert that it can also be mapped.

Copy link
Contributor Author

@fabian-lunarg fabian-lunarg Jan 27, 2025

Choose a reason for hiding this comment

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

double-checked the assumption, I think it's correct. the only caveat I could think of if a compute-pipeline would have been bound and then unbound again. but that's not allowed ("pipeline must be a valid VkPipeline handle")

Copy link
Contributor

Choose a reason for hiding this comment

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

@antonio-lunarg is your concern resolved?

{
auto* previous_pipeline = object_table_->GetVkPipelineInfo(
command_buffer_info->bound_pipelines.at(VK_PIPELINE_BIND_POINT_COMPUTE));
GFXRECON_ASSERT(previous_pipeline);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

@antonio-lunarg is your concern resolved?

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 358099.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5951 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 358164.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5952 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5952 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 359206.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5960 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5960 passed.

@fabian-lunarg fabian-lunarg force-pushed the fabian_work_on_sbt_data branch from f47de41 to a68aeea Compare January 29, 2025 11:57
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 359348.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5963 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5963 passed.

@fabian-lunarg fabian-lunarg merged commit 01e5bb8 into LunarG:dev Jan 30, 2025
9 checks passed
@fabian-lunarg fabian-lunarg deleted the fabian_work_on_sbt_data branch January 30, 2025 07:55
fabian-lunarg added a commit to fabian-lunarg/gfxreconstruct that referenced this pull request Jan 30, 2025
…rG#1993)

* Account for all bind-points when tracking bound pipelines
* Adjust internal compute-shader: pass-through data that could not be mapped
* Keep track of opaque/assigned acceleration-structure addresses
* Rebind previously bound compute-pipelines, if any
* Adjust internal compute-shader: pass-through data that could not be mapped (SBT)
* rework I/O address calculation, account for data-payloads and pass-through
* Drop GL_GOOGLE_include_directive from internal compute-shader, not used
* Use accelerationStructureCaptureReplay only when allocator supports opaque addresses
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.

5 participants