Hey Keith,
Most of the cool kids prefer gitlab MR, can you open one going forward?
That aside, here is some long overdue input on the patch itself.
My biggest concern that we expose the extension, even when it's not implemented.
The rest are rather minor - more documentation/comments, style fixes
and nitpicks.
On Thu, 30 Jan 2020 at 04:48, Keith Packard wrote:
> v5:
> Squash core bits and anv/radv wrappers into a single patch
>
We have more Vulkan drivers than anv/radv these days, with a few
others in the works.
For posterity sake, it might be worth keeping core + driver enablement
separate patches.
As an added bonus, this way the respective teams can test, review,
merge or even revert (please no) completely independently from each
other.
> --- a/src/amd/vulkan/radv_extensions.py
> +++ b/src/amd/vulkan/radv_extensions.py
> @@ -166,6 +166,7 @@ EXTENSIONS = [
> Extension('VK_AMD_shader_trinary_minmax', 1, True),
> Extension('VK_GOOGLE_decorate_string',1, True),
> Extension('VK_GOOGLE_hlsl_functionality1',1, True),
> +Extension('VK_GOOGLE_display_timing', 1, True),
Nit: sort?
> --- a/src/amd/vulkan/radv_wsi.c
> +++ b/src/amd/vulkan/radv_wsi.c
> @@ -316,3 +316,36 @@ VkResult radv_GetPhysicalDevicePresentRectanglesKHR(
> surface,
> pRectCount, pRects);
> }
> +
> +/* VK_GOOGLE_display_timing */
> +VkResult
> +radv_GetRefreshCycleDurationGOOGLE(
> + VkDevice _device,
> + VkSwapchainKHR swapchain,
> + VkRefreshCycleDurationGOOGLE *pDisplayTimingProperties)
Nit: inconsistent indentation (follow GetPastPresentationTiming example below).
> --- a/src/intel/vulkan/anv_extensions.py
> +++ b/src/intel/vulkan/anv_extensions.py
> @@ -170,6 +170,7 @@ EXTENSIONS = [
> Extension('VK_ANDROID_external_memory_android_hardware_buffer', 3,
> 'ANDROID'),
> Extension('VK_ANDROID_native_buffer', 7, 'ANDROID'),
> Extension('VK_GOOGLE_decorate_string',1, True),
> +Extension('VK_GOOGLE_display_timing', 1, True),
The dummy true will always advertise the extension. The extension
depends on VK_KHR_swapchain which uses DRV_HAS_SURFACE guard, which is
a good start.
Although there is no Wayland implementation, so how does the extension
work there? We really should not advertise it in said case.
Same applies for radv as well, obviously.
> --- a/src/vulkan/wsi/wsi_common.c
> +++ b/src/vulkan/wsi/wsi_common.c
> @@ -54,6 +55,7 @@ wsi_device_init(struct wsi_device *wsi,
> WSI_GET_CB(GetPhysicalDeviceProperties2);
> WSI_GET_CB(GetPhysicalDeviceMemoryProperties);
> WSI_GET_CB(GetPhysicalDeviceQueueFamilyProperties);
> + WSI_GET_CB(GetPhysicalDeviceProperties);
Nit: sort
> @@ -94,11 +104,15 @@ wsi_device_init(struct wsi_device *wsi,
> WSI_GET_CB(GetImageMemoryRequirements);
> WSI_GET_CB(GetImageSubresourceLayout);
> WSI_GET_CB(GetMemoryFdKHR);
> + WSI_GET_CB(GetPhysicalDeviceProperties);
> WSI_GET_CB(GetPhysicalDeviceFormatProperties);
> WSI_GET_CB(GetPhysicalDeviceFormatProperties2KHR);
Nit: sort - DeviceProperties should be here.
> @@ -210,6 +224,8 @@ wsi_swapchain_init(const struct wsi_device *wsi,
> chain->device = device;
> chain->alloc = *pAllocator;
> chain->use_prime_blit = false;
> + chain->timing_insert = 0;
> + chain->timing_count = 0;
>
Not needed we memset(0) at the top of the function.
> +static VkResult
> +wsi_image_init_timestamp(const struct wsi_swapchain *chain,
> + struct wsi_image *image)
> +{
> + const struct wsi_device *wsi = chain->wsi;
> + VkResult result;
> + /* Set up command buffer to get timestamp info */
> +
> + result = wsi->CreateQueryPool(
> + chain->device,
> + &(const VkQueryPoolCreateInfo) {
> + .sType = VK_STRUCTURE_TYPE_QUERY_POOL_CREATE_INFO,
> +.queryType = VK_QUERY_TYPE_TIMESTAMP,
> +.queryCount = 1,
> +},
> + NULL,
> + >query_pool);
> +
Current code prefers using local variables. Bonus - it will ease
fixing the indentation. Comments apply for the whole patch.
> +static struct wsi_timing *
> +wsi_next_timing(struct wsi_swapchain *chain, int image_index)
Unused image_index. Left over from earlier revision, or incomplete code?
> +{
> + uint32_t j = chain->timing_insert;
> + ++chain->timing_insert;
Please use post increments through the patch.
> + if (chain->timing_insert >= WSI_TIMING_HISTORY)
> + chain->timing_insert = 0;
> + if (chain->timing_count < WSI_TIMING_HISTORY)
> + ++chain->timing_count;
> + struct wsi_timing *timing = >timing[j];
> + memset(timing, '\0', sizeof (*timing));
The memset here is rather misleading. Since the caller already
(re)sets some of the fields one might as well just expand that.
> +void
> +wsi_present_complete(struct
