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

vkDestroyFence validation fails after vkDeviceWaitIdle call if no vkWaitForFences is called #85

Closed
karl-lunarg opened this issue May 14, 2018 · 5 comments
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@karl-lunarg
Copy link
Contributor

Issue by PVRDevTech (MIGRATED)
Thursday May 03, 2018 at 10:16 GMT
Originally opened as KhronosGroup/Vulkan-LoaderAndValidationLayers#2632


Short description- reproduction

Application appears to have no other errors. There are two sets of fences synchronising command buffers submissions and command buffer presentation. When cleaning up exiting the application:

  • Call vkDeviceWaitIdle. Do not explicitly wait for any fences
  • Destroy the objects (including the vkDestroyFence)

Result (Actual)

Standard validation fails with: VK_DEBUG_REPORT_OBJECT_TYPE_FENCE_EXT. VULKAN_LAYER_VALIDATION: Object: 0x1c (Type = 7) | Fence 0x1c is in use. The spec valid usage text states 'All queue submission commands that refer to fence must have completed execution' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-vkDestroyFence-fence-01120)

Additional info (+Workaround)

If I call vkWaitForFences and vkResetFences after (or before) vkDeviceWaitIdle, and before vkDestroyFances, the validation error goes away.Additional info

I also tried just querying the fence status before destroying:

  • vkDeviceWaitIdle
  • vkGetFenceStatus
  • vkDestroyFence

This also fixes the issue (i.e. it does not throw the validation error), which makes me think that it is just that the layer code has the wrong idea about the fence status reasoning (i.e. not updated by vkDeviceWaitIdle that the fence must be signalled).

Expected (Correct) behaviour

Since vkDeviceWaitIdle has been called, it is expected that all commands have finished executing, hence all the fences are signalled, hence the validation error is incorrect as the fences must be signalled.

Rationale

I assume that vkDeviceWaitIdle should not have returned if the fences were not signalled, as then it would be implied that the corresponding commands are not executed.

Debugging Details

For SDK 1.1.73.0 The failure can become apparent seen around core_validation.cpp:3364: if ((*fence_node)->scope == kSyncScopeInternal && (*fence_node)->state == FENCE_INFLIGHT).

Since If I insert a vkWaitForFences the error goes away, it follows that the layer probably misses the fact that the fence must have been signalled as the device has completed all commands. I think that the vkDeviceWaitIdle command should mark the fences as complete itself, or something to that effect.

Thanks,
Gerry

@karl-lunarg karl-lunarg added the Bug Something isn't working label May 14, 2018
@jzulauf-lunarg
Copy link
Contributor

jzulauf-lunarg commented May 24, 2018

@PVRDevTech Attempted to repro this using VulkanSamples API-Samples/multithreaded_command_buffers modified to vkDeviceWaitIdle instead of vkWaitForFences without seeing the ERROR. I did see it clear the status of the pending fences in core_validation_layer RetireWorkOnQueue during validation of the vkDeviceWaitIdle as expected.

Is there a repro case that you can share? Were there any other validation messages prior to the incorrect one?

@PVRDevTech
Copy link

@jzulauf-lunarg - Sorry for the delay, the notification was completely missed.

In our SDK, any of the Vulkan examples. https://github.com/powervr-graphics/Native_SDK/tree/5.1

It's not that big to download and it's CMake, but the repro case does require a tiny modification:

In (let's say) examples/Vulkan/02_IntroducingPVRShell/VulkanIntroducingPVRShell.cpp

The example can be built from its own folder (has a standalone CMakeLists.txt) to avoid having to build all of them.

Please comment out lines 1005, 1006, 1013, 1014 (the WaitForFences and ResetFences before DestroyFence) which are my workaround for the validation error.

If you have any issues with building it I can try to somehow provide a cut-down repro version, but it really should be straightforward with this one.

Thanks,
Gerry

@jzulauf-lunarg
Copy link
Contributor

jzulauf-lunarg commented Jun 15, 2018

Thanks for the great repro case. We think the validation layer is doing the right thing here subject to clarification with the working group.

The fence that is raising the issue is used for signalling read completion from vkAcquireNextImageKHR -- which is not a queue operation.

By our reading vkDeviceWaitIdle should not affect the state of those fences.

vkDeviceWaitIdle is equivalent to calling vkQueueWaitIdle for all queues owned by device.

It does not state that waiting for read completion of swap chain images is included in it's synchronization scope. The only way to know if a fence used by vkAcquireNextImageKHR is to wait for that fence.

Where we believe the spec is incomplete is w.r.t. vkDestroyFence. The valid usage there only requires:

All queue submission commands that refer to fence must have completed execution

However, given that in the process of signalling a fence, implementations will likely want to write to a fence memory location, it seems unsafe and invalid to allow destruction before the application knows (via vkWaitFence) that the implementation has signaled the fence.

I'll raise the issue re: VkDestroyFence with the Working Group to get clarification and will reopen the issue if validation needs to be changed.

@PVRDevTech
Copy link

All right, thank you, very interesting point - with current vkDeviceWaitIdle definition, there is nothing guaranteeing that the acquire fences are done.

Thanks,
Gerry.

@jzulauf-lunarg
Copy link
Contributor

Issue opened with WG.

@shannon-lunarg shannon-lunarg added this to the sdk-1.1.82.0 milestone Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants