timestamp example two problems

[pc-john]

I see two issues with timestamp example:

1. Support for timestamps is detected by comparing VkPhysicalDeviceLimits::timestampPeriod to zero. If it is zero, the device is considered to not support timestamps. I was trying to find support for this idea in Vulkan specification, but found none. The only base for the timestamp support in the spec is based on timestampComputeAndGraphics and timestampValidBits in the spec. There is no word about VkPhysicalDeviceLimits::timestampPeriod value if timestamps are not supported.
2. In the part "Interpreting the results", following equation is used float delta_in_ms = float(time_stamps[1] - time_stamps[0]) * device_limits.timestampPeriod / 1000000.0f;. The equation does not handle timestamp overflows correctly, in my opinion. Timestamp precision is between 36..64 bits. And returned timestamps contain zeros on the place of bits outside of the valid bits range. So, if time_stamps[1] already wraps over to value zero and time_stamps[0] contains maximum value of 36 bits of precision, we have 0x0000'0000'0000'0000 - 0x0000'000f'ffff'ffff = 0xffff'fff0'0000'0001. In other words, the result of subtraction is now not 1 but extremely high number. I guess that correct computation shall use the mask uint64_t timestampValidBitMask = (timestampValidBits >= 64) ? ~uint64_t(0) : (uint64_t(1) << timestampValidBits) - 1; . And the fixed equation would be float delta_in_ms = float((time_stamps[1] - time_stamps[0]) & timestampValidBitMask) * device_limits.timestampPeriod / 1000000.0f;.
3. The text uses both "timestamp" and "time stamp". The spec uses only "timestamp". I am inclined to suggest to be consistent with the spec.

If I am mistaken, especially on point 1 and 2, please correct me.

Edit: Corrected computation of timestampValidBitMask.

[SaschaWillems]

Sorry for the late reply. And yes, you're correct. Point 1 and 2 need to be reworked. Point 1 was implemented that way because back then there were Vulkan implementations out there that did not properly report time stamp support.

[pc-john]

Oh. I see. Broken drivers are pain. Thanks for maintaining of the projects!

[SaschaWillems]

I have to correct myself here. The way I do check for timestamp support in that sample is correct:

"Timestamps are supported on any queue which reports a non-zero value for timestampValidBits via vkGetPhysicalDeviceQueueFamilyProperties. If the timestampComputeAndGraphics limit is VK_TRUE, timestamps are supported by every queue family that supports either graphics or compute operations (see VkQueueFamilyProperties)." (from the spec)

Will take a look at pt. 2 though.

[pc-john]

Still to point 1:

In my opinion, this sentence seems not correct to me: "Here we need to check if the timestampPeriod limit of the physical device is greater than zero. If that’s the case, timestamp queries are supported:". If I understand your previous post correctly, it is here just because of some old driver bug. Based on spec, there are no grounds to claim timestamp support based on timestampPeriod.

But you are right that the code is correct. The timestampPeriod test looked superfluous and misleading to new Vulkan programmers, and it is not clear from the text that it is just a hack around old driver bug. The rest of the code makes perfect sense and seems to perfectly match Vulkan spec.

So, after your explanation, I am inclined to suggest two solutions:

1. remove old code that workarounds problems with old and non-conformant drivers; this would simplify your example and make it more understandable for new Vulkan programmers
2. change the wording to make it more clear what is going on here. For example: "Here we check if the timestampPeriod of the physical device is zero. Some old buggy drivers did not report the lack of timestamp support correctly, so we workaround it by checking timestampPeriod to be zero:"

It is your example, so take my suggestions just as suggestions. I prefer people to learn on workaround-free code, because we might end up with complicated examples carrying workarounds for years of bugs in graphics drivers. Not sure if this is what the person learning Vulkan is expecting from Vulkan tutorials and examples. Maybe, it is ok, but I would somehow prefer to make it clear that something is workaround and superfluous when using conformant Vulkan driver.