Re: [Mesa-dev] [PATCH mesa 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v3]
On Tue, Jul 10, 2018 at 9:47 AM Jason Ekstrand wrote:
> On Mon, Jul 9, 2018 at 5:35 PM Keith Packard wrote:
>
>> This extension adds a single function to query the current GPU
>> timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
>> function is needed to complete the implementation of
>> GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
>> timestamps.
>>
>> v2: Adopt Jason Ekstrand's coding conventions
>>
>> Declare variables at first use, eliminate extra whitespace between
>> types and names. Wrap lines to 80 columns.
>>
>> Add extension to list in alphabetical order
>>
>> Suggested-by: Jason Ekstrand
>>
>> v3: Report both device and surface timestamps
>>
>> This will allow us to get timestamps more closely aligned by
>> getting both in a single call from the kernel.
>>
>> To make this independent of the timebase used by the WSI
>> layer, provide a new wsi hook which converts CLOCK_MONOTONIC
>> into the matching WSI timebase. Right now, all of our backends
>> use CLOCK_MONOTONIC, so there's nothing actually doing
>> conversions, but it seemed best to put the infrastructure in
>> place so that I could validate the extension interface would
>> work if that became necessary.
>>
>> Signed-off-by: Keith Packard
>> ---
>> include/vulkan/vk_mesa_query_timestamp.h | 46
>> src/vulkan/registry/vk.xml | 20 +++
>> src/vulkan/wsi/wsi_common.c | 17 +
>> src/vulkan/wsi/wsi_common.h | 7
>> src/vulkan/wsi/wsi_common_private.h | 4 +++
>> 5 files changed, 94 insertions(+)
>> create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
>>
>> diff --git a/include/vulkan/vk_mesa_query_timestamp.h
>> b/include/vulkan/vk_mesa_query_timestamp.h
>> new file mode 100644
>> index 000..262f094db27
>> --- /dev/null
>> +++ b/include/vulkan/vk_mesa_query_timestamp.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + * Copyright © 2018 Keith Packard
>> + *
>> + * Permission to use, copy, modify, distribute, and sell this software
>> and its
>> + * documentation for any purpose is hereby granted without fee, provided
>> that
>> + * the above copyright notice appear in all copies and that both that
>> copyright
>> + * notice and this permission notice appear in supporting documentation,
>> and
>> + * that the name of the copyright holders not be used in advertising or
>> + * publicity pertaining to distribution of the software without specific,
>> + * written prior permission. The copyright holders make no
>> representations
>> + * about the suitability of this software for any purpose. It is
>> provided "as
>> + * is" without express or implied warranty.
>> + *
>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
>> SOFTWARE,
>> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
>> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT
>> OR
>> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
>> OF USE,
>> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
>> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
>> PERFORMANCE
>> + * OF THIS SOFTWARE.
>> + */
>> +
>> +#ifndef __VK_MESA_QUERY_TIMESTAMP_H__
>> +#define __VK_MESA_QUERY_TIMESTAMP_H__
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +typedef struct VkCurrentTimestampMESA {
>> +uint64_tdeviceTimestamp;
>> +uint64_tsurfaceTimestamp;
>> +} VkCurrentTimestampMESA;
>> +
>> +typedef VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
>> +VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA
>> *timestamp);
>> +
>> +VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
>> +VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA
>> *timestamp);
>>
>
> I hate to salami extensions too much but it might be better if this were
> two extensions. One would be
>
> typedef struct VkCurrentTimestampMESA {
> VkStructType sType;
> void *pNext;
> uint64_t timestamp;
> } VkCurrentTimestampMESA;
>
> VkResult vkGetCurrentTimestampMESA(VkDevice device, VkBaseInStructureKHR
> *pInfo, VkCurrentTimestampMESA *pTimestamp);
>
> And the second would just define two chain in structs
>
> typedef struct VkCurrentPresentTimestampGetInfoMESA {
> VkStructureType sType;
> void *pNext;
> VkSurfaceKHR surface;
> } VkCurrentPresentTimestampGetInfoMESA;
>
> typedef struct VkCurrentPresentTimestampMESA {
> VkStructureType sType;
> void *pNext;
> uint64_t presentTimestamp;
> } VkCurrentPresentTimestampMESA;
>
> You could probably also just make the surface part of
> VkCurrentPresentTimestampMESA and not have the extendible input struct.
> That might be a bit cleaner in some ways. If the vkGetCurrentTimestampMESA
> call is extended to get the surface timestamp, the
Re: [Mesa-dev] [PATCH mesa 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v3]
On Mon, Jul 9, 2018 at 5:35 PM Keith Packard wrote:
> This extension adds a single function to query the current GPU
> timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
> function is needed to complete the implementation of
> GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
> timestamps.
>
> v2: Adopt Jason Ekstrand's coding conventions
>
> Declare variables at first use, eliminate extra whitespace between
> types and names. Wrap lines to 80 columns.
>
> Add extension to list in alphabetical order
>
> Suggested-by: Jason Ekstrand
>
> v3: Report both device and surface timestamps
>
> This will allow us to get timestamps more closely aligned by
> getting both in a single call from the kernel.
>
> To make this independent of the timebase used by the WSI
> layer, provide a new wsi hook which converts CLOCK_MONOTONIC
> into the matching WSI timebase. Right now, all of our backends
> use CLOCK_MONOTONIC, so there's nothing actually doing
> conversions, but it seemed best to put the infrastructure in
> place so that I could validate the extension interface would
> work if that became necessary.
>
> Signed-off-by: Keith Packard
> ---
> include/vulkan/vk_mesa_query_timestamp.h | 46
> src/vulkan/registry/vk.xml | 20 +++
> src/vulkan/wsi/wsi_common.c | 17 +
> src/vulkan/wsi/wsi_common.h | 7
> src/vulkan/wsi/wsi_common_private.h | 4 +++
> 5 files changed, 94 insertions(+)
> create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
>
> diff --git a/include/vulkan/vk_mesa_query_timestamp.h
> b/include/vulkan/vk_mesa_query_timestamp.h
> new file mode 100644
> index 000..262f094db27
> --- /dev/null
> +++ b/include/vulkan/vk_mesa_query_timestamp.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright © 2018 Keith Packard
> + *
> + * Permission to use, copy, modify, distribute, and sell this software
> and its
> + * documentation for any purpose is hereby granted without fee, provided
> that
> + * the above copyright notice appear in all copies and that both that
> copyright
> + * notice and this permission notice appear in supporting documentation,
> and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no
> representations
> + * about the suitability of this software for any purpose. It is
> provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT
> OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
> USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
> PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#ifndef __VK_MESA_QUERY_TIMESTAMP_H__
> +#define __VK_MESA_QUERY_TIMESTAMP_H__
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +typedef struct VkCurrentTimestampMESA {
> +uint64_tdeviceTimestamp;
> +uint64_tsurfaceTimestamp;
> +} VkCurrentTimestampMESA;
> +
> +typedef VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
> +VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA
> *timestamp);
> +
> +VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
> +VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA
> *timestamp);
>
I hate to salami extensions too much but it might be better if this were
two extensions. One would be
typedef struct VkCurrentTimestampMESA {
VkStructType sType;
void *pNext;
uint64_t timestamp;
} VkCurrentTimestampMESA;
VkResult vkGetCurrentTimestampMESA(VkDevice device, VkBaseInStructureKHR
*pInfo, VkCurrentTimestampMESA *pTimestamp);
And the second would just define two chain in structs
typedef struct VkCurrentPresentTimestampGetInfoMESA {
VkStructureType sType;
void *pNext;
VkSurfaceKHR surface;
} VkCurrentPresentTimestampGetInfoMESA;
typedef struct VkCurrentPresentTimestampMESA {
VkStructureType sType;
void *pNext;
uint64_t presentTimestamp;
} VkCurrentPresentTimestampMESA;
You could probably also just make the surface part of
VkCurrentPresentTimestampMESA and not have the extendible input struct.
That might be a bit cleaner in some ways. If the vkGetCurrentTimestampMESA
call is extended to get the surface timestamp, the driver would have the
option to call the kernel for the more accurate thing or it could just
return the two separate things that your implementation already does.
The reason I suggest this is that Vulkan
