[1/3] vulkan: Define new VK_MESA_query_timestamp extension [v2]
diff mbox

Message ID 20180623151521.8306-2-keithp@keithp.com
State New
Headers show

Commit Message

Keith Packard June 23, 2018, 3:15 p.m. UTC
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, &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 <jason.ekstrand@intel.com>

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 include/vulkan/vk_mesa_query_timestamp.h | 41 ++++++++++++++++++++++++
 src/vulkan/registry/vk.xml               | 15 +++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 include/vulkan/vk_mesa_query_timestamp.h

Comments

Jason Ekstrand June 23, 2018, 5:13 p.m. UTC | #1
I haven't thought through this comment all that hard but would it make 
sense to have three timestamps, CPU, GPU, CPU so that you have error bars 
on the GPU timestamp?  At the very least, two timestamps would be better 
than one so that, when we pull it into the kernel, it can provide something 
more accurate than userspace trying to grab a snapshot.

--Jason


On June 23, 2018 10:15:34 Keith Packard <keithp@keithp.com> wrote:

> This extension adds a single function to query the current GPU
> timestamp, just like glGetInteger64v(GL_TIMESTAMP, &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 <jason.ekstrand@intel.com>
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> include/vulkan/vk_mesa_query_timestamp.h | 41 ++++++++++++++++++++++++
> src/vulkan/registry/vk.xml               | 15 +++++++++
> 2 files changed, 56 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 00000000000..4e0b50cb9cc
> --- /dev/null
> +++ b/include/vulkan/vk_mesa_query_timestamp.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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 VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
> +    VkDevice device, uint64_t *timestamp);
> +
> +VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
> +    VkDevice device, uint64_t *timestamp);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __VK_MESA_QUERY_TIMESTAMP_H__ */
> +
> diff --git a/src/vulkan/registry/vk.xml b/src/vulkan/registry/vk.xml
> index 7018bbe8421..3a5cecdc8e3 100644
> --- a/src/vulkan/registry/vk.xml
> +++ b/src/vulkan/registry/vk.xml
> @@ -6102,6 +6102,11 @@ server.
>             <param><type>uint32_t</type> <name>maxDrawCount</name></param>
>             <param><type>uint32_t</type> <name>stride</name></param>
>         </command>
> +        <command>
> +            <proto><type>VkResult</type> 
> <name>vkQueryCurrentTimestampMESA</name></proto>
> +            <param><type>VkDevice</type> <name>device</name></param>
> +            <param><type>uint64_t</type>* <name>pTimestamp</name></param>
> +        </command>
>     </commands>
>
>     <feature api="vulkan" name="VK_VERSION_1_0" number="1.0" comment="Vulkan 
>     core API interface definitions">
> @@ -8826,5 +8831,15 @@ server.
>                 <enum value="&quot;VK_KHR_extension_211&quot;"          
>                 name="VK_KHR_EXTENSION_211_EXTENSION_NAME"/>
>             </require>
>         </extension>
> +        <extension name="VK_MESA_query_timestamp" number="212"
> +		   type="device" author="MESA"
> +		   contact="Keith Packard @keithp"
> +		   supported="vulkan">
> +            <require>
> +                <enum value="1"                                         
> name="VK_MESA_QUERY_TIMESTAMP_SPEC_VERSION"/>
> +                <enum value="&quot;VK_MESA_query_timestamp&quot;"       
> name="VK_MESA_QUERY_TIMESTAMP_NAME"/>
> +                <command name="vkQueryCurrentTimestampMESA"/>
> +            </require>
> +        </extension>
>     </extensions>
> </registry>
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Pekka Paalanen July 10, 2018, 7:16 a.m. UTC | #2
On Sat, 23 Jun 2018 12:13:53 -0500
Jason Ekstrand <jason@jlekstrand.net> wrote:

> I haven't thought through this comment all that hard but would it make 
> sense to have three timestamps, CPU, GPU, CPU so that you have error bars 
> on the GPU timestamp?  At the very least, two timestamps would be better 
> than one so that, when we pull it into the kernel, it can provide something 
> more accurate than userspace trying to grab a snapshot.

Hi,

three timestamps sounds like a good idea to me, but you might want to
reach out to media developers (e.g. gstreamer) who have experience in
synchronizing different clocks and what that will actually take.

When reading the CPU timestamp, I believe it would be necessary for
userspace to tell which clock it should be reading. I'm not sure all
userspace is always using CLOCK_MONOTONIC. But if you have a better
rationale, that would be interesting to record and document.


Thanks,
pq

> On June 23, 2018 10:15:34 Keith Packard <keithp@keithp.com> wrote:
> 
> > This extension adds a single function to query the current GPU
> > timestamp, just like glGetInteger64v(GL_TIMESTAMP, &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 <jason.ekstrand@intel.com>
> >
> > Signed-off-by: Keith Packard <keithp@keithp.com>
> > ---
> > include/vulkan/vk_mesa_query_timestamp.h | 41 ++++++++++++++++++++++++
> > src/vulkan/registry/vk.xml               | 15 +++++++++
> > 2 files changed, 56 insertions(+)
> > create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
Keith Packard July 10, 2018, 6:02 p.m. UTC | #3
Pekka Paalanen <ppaalanen@gmail.com> writes:

> On Sat, 23 Jun 2018 12:13:53 -0500
> Jason Ekstrand <jason@jlekstrand.net> wrote:
>
>> I haven't thought through this comment all that hard but would it make 
>> sense to have three timestamps, CPU, GPU, CPU so that you have error bars 
>> on the GPU timestamp?  At the very least, two timestamps would be better 
>> than one so that, when we pull it into the kernel, it can provide something 
>> more accurate than userspace trying to grab a snapshot.
>
> Hi,
>
> three timestamps sounds like a good idea to me, but you might want to
> reach out to media developers (e.g. gstreamer) who have experience in
> synchronizing different clocks and what that will actually take.

Oh, I know that's really hard, and I don't want to solve that problem
here. I explicitly *don't* solve it though -- I simply expose the
ability to get correlated values in the two application-visible time
domains that the Vulkan API already exposes (surface time and GPU
time). How to synchronize the application as those two clocks drift
around is outside the domain of this extension.

> When reading the CPU timestamp, I believe it would be necessary for
> userspace to tell which clock it should be reading. I'm not sure all
> userspace is always using CLOCK_MONOTONIC. But if you have a better
> rationale, that would be interesting to record and document.

The extension doesn't state which clock is returned, you provide a
device and a surface and get timestamps for both of them. This allows
applications to translate between the two Vulkan time domains.

As far as the implementation goes, it asks the device driver to get
correlated GPU and CLOCK_MONOTONIC values and then asks the WSI layer to
translate between CLOCK_MONOTONIC and surface time. Right now, all three
surface types already operate in CLOCK_MONOTONIC natively, so there
isn't any translation needed to surface time.

The anv and radv driver implementations are simplistic and could easily
be improved; they both simply fetch the GPU clock and then
CLOCK_MONOTONIC sequentially without attempting to bound the error
between them.

The entire reason for this extension is to allow me to implement the
GOOGLE_display_timing extension using only public interfaces into the
drivers. That patch is blocked on getting this functionality landed.
Pekka Paalanen July 11, 2018, 7:31 a.m. UTC | #4
On Tue, 10 Jul 2018 11:02:23 -0700
"Keith Packard" <keithp@keithp.com> wrote:

> Pekka Paalanen <ppaalanen@gmail.com> writes:
> 
> > On Sat, 23 Jun 2018 12:13:53 -0500
> > Jason Ekstrand <jason@jlekstrand.net> wrote:
> >  
> >> I haven't thought through this comment all that hard but would it make 
> >> sense to have three timestamps, CPU, GPU, CPU so that you have error bars 
> >> on the GPU timestamp?  At the very least, two timestamps would be better 
> >> than one so that, when we pull it into the kernel, it can provide something 
> >> more accurate than userspace trying to grab a snapshot.  
> >
> > Hi,
> >
> > three timestamps sounds like a good idea to me, but you might want to
> > reach out to media developers (e.g. gstreamer) who have experience in
> > synchronizing different clocks and what that will actually take.  
> 
> Oh, I know that's really hard, and I don't want to solve that problem
> here. I explicitly *don't* solve it though -- I simply expose the
> ability to get correlated values in the two application-visible time
> domains that the Vulkan API already exposes (surface time and GPU
> time). How to synchronize the application as those two clocks drift
> around is outside the domain of this extension.

Hi Keith,

I did not mean you would be solving that problem. I meant that it would
be good to figure out what people actually want from the API to be able
to solve the problem themselves.


Thanks,
pq
Keith Packard July 11, 2018, 3:56 p.m. UTC | #5
Pekka Paalanen <ppaalanen@gmail.com> writes:

> I did not mean you would be solving that problem. I meant that it would
> be good to figure out what people actually want from the API to be able
> to solve the problem themselves.

Thanks for the clarification. I'd suggest that we not try and solve that
problem until we have someone who needs it?

What I'm using this extension for is to correlate GPU times with WSI
times as required by the GOOGLE_display_timing extension. That extension
reports the time gap between the end of rendering and when a frame is
displayed as reported by the WSI backend, so I needed some way to
translate between those two time domains. To do this, I call this new
function once per presentation, which means I'm getting recent values
for both clocks which should track any minor clock skew.

Patch
diff mbox

diff --git a/include/vulkan/vk_mesa_query_timestamp.h b/include/vulkan/vk_mesa_query_timestamp.h
new file mode 100644
index 00000000000..4e0b50cb9cc
--- /dev/null
+++ b/include/vulkan/vk_mesa_query_timestamp.h
@@ -0,0 +1,41 @@ 
+/*
+ * 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 VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
+    VkDevice device, uint64_t *timestamp);
+
+VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
+    VkDevice device, uint64_t *timestamp);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __VK_MESA_QUERY_TIMESTAMP_H__ */
+
diff --git a/src/vulkan/registry/vk.xml b/src/vulkan/registry/vk.xml
index 7018bbe8421..3a5cecdc8e3 100644
--- a/src/vulkan/registry/vk.xml
+++ b/src/vulkan/registry/vk.xml
@@ -6102,6 +6102,11 @@  server.
             <param><type>uint32_t</type> <name>maxDrawCount</name></param>
             <param><type>uint32_t</type> <name>stride</name></param>
         </command>
+        <command>
+            <proto><type>VkResult</type> <name>vkQueryCurrentTimestampMESA</name></proto>
+            <param><type>VkDevice</type> <name>device</name></param>
+            <param><type>uint64_t</type>* <name>pTimestamp</name></param>
+        </command>
     </commands>
 
     <feature api="vulkan" name="VK_VERSION_1_0" number="1.0" comment="Vulkan core API interface definitions">
@@ -8826,5 +8831,15 @@  server.
                 <enum value="&quot;VK_KHR_extension_211&quot;"          name="VK_KHR_EXTENSION_211_EXTENSION_NAME"/>
             </require>
         </extension>
+        <extension name="VK_MESA_query_timestamp" number="212"
+		   type="device" author="MESA"
+		   contact="Keith Packard @keithp"
+		   supported="vulkan">
+            <require>
+                <enum value="1"                                         name="VK_MESA_QUERY_TIMESTAMP_SPEC_VERSION"/>
+                <enum value="&quot;VK_MESA_query_timestamp&quot;"       name="VK_MESA_QUERY_TIMESTAMP_NAME"/>
+                <command name="vkQueryCurrentTimestampMESA"/>
+            </require>
+        </extension>
     </extensions>
 </registry>