KhronosGroup / MoltenVK

MoltenVK is a Vulkan Portability implementation. It layers a subset of the high-performance, industry-standard Vulkan graphics and compute API over Apple's Metal graphics framework, enabling Vulkan applications to run on macOS, iOS and tvOS.
Apache License 2.0
4.76k stars 418 forks source link

Remove or disable timestampPeriod lowpass filter on non-Apple GPUs #2013

Closed SRSaunders closed 12 months ago

SRSaunders commented 1 year ago

Is there any way to disable the timestampPeriod low pass filter that was implemented right at the end of the 1.2.5 release cycle (commit 530bde199b0e8ca3b20e0f7c0b6fb08d27d8284b)?

See the following new code in MVKPhysicalDevice::updateTimestampsAndPeriod():

// Basic lowpass filter Y = (1 - a)Y + a*X.
// The lower a is, the slower Y will change over time.
static const float a = 0.05;
_properties.limits.timestampPeriod = ((1.0 - a) * _properties.limits.timestampPeriod) + (a * tsPeriod);

This negatively affects my embedded Optick profiler integration which depends on an accurate point-in-time measurement of timestampPeriod for GPU/CPU timeline calibration just before beginning a profiling trace. Averaging this value ruins the timeline calibration and GPU traces are now misaligned with CPU traces.

Could this filter be preferably removed or even disabled via a config setting (e.g. on/off or setting the a value to 1.0)?

billhollings commented 1 year ago

Huh. Interesting.

Since vkGetPhysicalDeviceProperties() is mostly about fixed platform characteristics, and extracts a ton of info, I didn't expect it to be something that an app would be calling frequently, so I figured volatile point-in-time values would be unhelpful, hence the averaging.

Perhaps we can add a config option for the a value, to allow an app to choose how volatile it wants the timestampPeriod to be?

SRSaunders commented 1 year ago

Perhaps we can add a config option for the a value, to allow an app to choose how volatile it wants the timestampPeriod to be?

That would work for me. Thx.

On second thought, is the filter even necessary in the first place? To make the filter work correctly and converge, one would have to call vkGetPhysicalDeviceProperties() many times (i.e. > 20 when a = 0.05). For instance, if the initial value was not a stable sample, then the filter will persist this negative effect until many iterations are complete.

I am thinking it might be simpler to let the end user handle this externally if required. In my case, I am not interested in any filtering, as I can let the HUD visually average any point in time changes, and for the profiler I absolutely do need the current point in time value for timeline calibration. For other applications, perhaps the end user would make a different choice and sample / average the value at a frequency appropriate to their application. Your thoughts?

It's too bad that one small change to improve async encoding has caused so much disruption in this area. I am wondering whether it would be better to return to the old implementation, but internally relocate the periodic timestamp correlation to a place where it would not interact negatively with command buffer submission.

billhollings commented 1 year ago

PR #2017 add a configurable alpha for the timestampPeriod.

To disable the filter, set env var MVK_CONFIG_TIMESTAMP_PERIOD_LOWPASS_ALPHA=1.0.

If this works for your needs, please close this issue.

SRSaunders commented 1 year ago

Thanks for implementing this. It looks like it will solve my issue but unfortunately I cannot test for 2-3 weeks since I am currently away on vacation until the end of Sept. I will test and hopefully close when I return.

SRSaunders commented 12 months ago

I have now tested this change using MoltenVK's config API with mvkConfig.timestampPeriodLowPassAlpha = 1.0 and it works well for my use case (game HUD and CPU/GPU timeline calibration for embedded Optick profiler).

I have not tested the environment variable approach but I assume it will work the same way.

Thanks for solving this issue and I will close now.