Re: [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v2]

From: Jason Ekstrand <jason@jlekstrand.net>
To: Keith Packard <keithp@keithp.com>
Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v2]
Date: Mon, 15 Oct 2018 16:36:39 -0500	[thread overview]
Message-ID: <CAOFGe95oOM8bghdZUT_D3EKdDM-wpv9n0ac+VASTdHOmeGyEwA@mail.gmail.com> (raw)
In-Reply-To: <20181015212248.436-1-keithp@keithp.com>

[-- Attachment #1.1: Type: text/plain, Size: 11262 bytes --]

On Mon, Oct 15, 2018 at 4:22 PM Keith Packard <keithp@keithp.com> wrote:

> Offers three clocks, device, clock monotonic and clock monotonic
> raw. Could use some kernel support to reduce the deviation between
> clock values.
>
> v2:
>         Ensure deviation is at least as big as the GPU time interval.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/amd/vulkan/radv_device.c       | 84 ++++++++++++++++++++++++++++
>  src/amd/vulkan/radv_extensions.py  |  1 +
>  src/intel/vulkan/anv_device.c      | 88 ++++++++++++++++++++++++++++++
>  src/intel/vulkan/anv_extensions.py |  1 +
>  src/intel/vulkan/anv_gem.c         | 13 +++++
>  src/intel/vulkan/anv_private.h     |  2 +
>  6 files changed, 189 insertions(+)
>
> diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
> index 174922780fc..29f0afbc69b 100644
> --- a/src/amd/vulkan/radv_device.c
> +++ b/src/amd/vulkan/radv_device.c
> @@ -4955,3 +4955,87 @@ radv_GetDeviceGroupPeerMemoryFeatures(
>                                VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
>                                VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
>  }
> +
> +static const VkTimeDomainEXT radv_time_domains[] = {
> +       VK_TIME_DOMAIN_DEVICE_EXT,
> +       VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
> +       VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
> +};
> +
> +VkResult radv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
> +       VkPhysicalDevice                             physicalDevice,
> +       uint32_t                                     *pTimeDomainCount,
> +       VkTimeDomainEXT                              *pTimeDomains)
> +{
> +       int d;
> +       VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
> +
> +       for (d = 0; d < ARRAY_SIZE(radv_time_domains); d++) {
> +               vk_outarray_append(&out, i) {
> +                       *i = radv_time_domains[d];
> +               }
> +       }
> +
> +       return vk_outarray_status(&out);
> +}
> +
> +static uint64_t
> +radv_clock_gettime(clockid_t clock_id)
> +{
> +       struct timespec current;
> +       int ret;
> +
> +       ret = clock_gettime(clock_id, &current);
> +       if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
> +               ret = clock_gettime(CLOCK_MONOTONIC, &current);
> +       if (ret < 0)
> +               return 0;
> +
> +       return (uint64_t) current.tv_sec * 1000000000ULL + current.tv_nsec;
> +}
> +
> +#define TIMESTAMP 0x2358
> +
> +VkResult radv_GetCalibratedTimestampsEXT(
> +       VkDevice                                     _device,
> +       uint32_t                                     timestampCount,
> +       const VkCalibratedTimestampInfoEXT           *pTimestampInfos,
> +       uint64_t                                     *pTimestamps,
> +       uint64_t                                     *pMaxDeviation)
> +{
> +       RADV_FROM_HANDLE(radv_device, device, _device);
> +       uint32_t clock_crystal_freq =
> device->physical_device->rad_info.clock_crystal_freq;
> +       int d;
> +       uint64_t begin, end;
> +
> +       begin = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
> +
> +       for (d = 0; d < timestampCount; d++) {
> +               switch (pTimestampInfos[d].timeDomain) {
> +               case VK_TIME_DOMAIN_DEVICE_EXT:
> +                       /* XXX older kernels don't support this interface.
> */
> +                       pTimestamps[d] =
> device->ws->query_value(device->ws,
> +
> RADEON_TIMESTAMP);
> +                       break;
> +               case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
> +                       pTimestamps[d] =
> radv_clock_gettime(CLOCK_MONOTONIC);
> +                       break;
> +
> +               case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
> +                       pTimestamps[d] = begin;
> +                       break;
> +               default:
> +                       pTimestamps[d] = 0;
> +                       break;
> +               }
> +       }
> +
> +       end = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
> +
> +       uint64_t clock_period = end - begin;
> +       uint64_t device_period = (1000000 + clock_crystal_freq - 1) /
> clock_crystal_freq;
> +
> +       *pMaxDeviation = clock_period > device_period ? clock_period :
> device_period;
> +
> +       return VK_SUCCESS;
> +}
> diff --git a/src/amd/vulkan/radv_extensions.py
> b/src/amd/vulkan/radv_extensions.py
> index 5dcedae1c63..4c81d3f0068 100644
> --- a/src/amd/vulkan/radv_extensions.py
> +++ b/src/amd/vulkan/radv_extensions.py
> @@ -92,6 +92,7 @@ EXTENSIONS = [
>      Extension('VK_KHR_display',                          23,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
>      Extension('VK_EXT_direct_mode_display',               1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
>      Extension('VK_EXT_acquire_xlib_display',              1,
> 'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
> +    Extension('VK_EXT_calibrated_timestamps',             1, True),
>      Extension('VK_EXT_conditional_rendering',             1, True),
>      Extension('VK_EXT_conservative_rasterization',        1,
> 'device->rad_info.chip_class >= GFX9'),
>      Extension('VK_EXT_display_surface_counter',           1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index a2551452eb1..6a6539c9685 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -3021,6 +3021,94 @@ void anv_DestroyFramebuffer(
>     vk_free2(&device->alloc, pAllocator, fb);
>  }
>
> +static const VkTimeDomainEXT anv_time_domains[] = {
> +   VK_TIME_DOMAIN_DEVICE_EXT,
> +   VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
> +   VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
> +};
> +
> +VkResult anv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
> +   VkPhysicalDevice                             physicalDevice,
> +   uint32_t                                     *pTimeDomainCount,
> +   VkTimeDomainEXT                              *pTimeDomains)
> +{
> +   int d;
> +   VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
> +
> +   for (d = 0; d < ARRAY_SIZE(anv_time_domains); d++) {
> +      vk_outarray_append(&out, i) {
> +         *i = anv_time_domains[d];
> +      }
> +   }
> +
> +   return vk_outarray_status(&out);
> +}
> +
> +static uint64_t
> +anv_clock_gettime(clockid_t clock_id)
> +{
> +   struct timespec current;
> +   int ret;
> +
> +   ret = clock_gettime(clock_id, &current);
> +   if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
> +      ret = clock_gettime(CLOCK_MONOTONIC, &current);
> +   if (ret < 0)
> +      return 0;
> +
> +   return (uint64_t) current.tv_sec * 1000000000ULL + current.tv_nsec;
> +}
>

One lf these days, we should unify these.  I know this is at least the
second copy in anv and we've got it hand-rolled a couple other places.  I
don't care too much today though so meh.

> +
> +#define TIMESTAMP 0x2358
> +
> +VkResult anv_GetCalibratedTimestampsEXT(
> +   VkDevice                                     _device,
> +   uint32_t                                     timestampCount,
> +   const VkCalibratedTimestampInfoEXT           *pTimestampInfos,
> +   uint64_t                                     *pTimestamps,
> +   uint64_t                                     *pMaxDeviation)
> +{
> +   ANV_FROM_HANDLE(anv_device, device, _device);
> +   uint64_t timestamp_frequency = device->info.timestamp_frequency;
> +   int  ret;
> +   int d;
> +   uint64_t begin, end;
> +
> +   begin = anv_clock_gettime(CLOCK_MONOTONIC_RAW);
> +
> +   for (d = 0; d < timestampCount; d++) {
> +      switch (pTimestampInfos[d].timeDomain) {
> +      case VK_TIME_DOMAIN_DEVICE_EXT:
> +         /* XXX older kernels don't support this interface. */
>

I don't think we support kernels that old, so you can drop the comment.

> +         ret = anv_gem_reg_read(device, TIMESTAMP | 1,
> +                                &pTimestamps[d]);
> +
> +         if (ret != 0)
> +            return VK_ERROR_DEVICE_LOST;
>

If you're going to return device_lost, you should set device->lost = true;

> +         break;
> +      case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
> +         pTimestamps[d] = anv_clock_gettime(CLOCK_MONOTONIC);
> +         break;
> +
> +      case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
> +         pTimestamps[d] = begin;
> +         break;
> +      default:
> +         pTimestamps[d] = 0;
> +         break;
> +      }
> +   }
> +
> +   end = anv_clock_gettime(CLOCK_MONOTONIC_RAW);
> +
> +   uint64_t clock_period = end - begin;
> +   uint64_t device_period = (1000000000 + timestamp_frequency - 1) /
> timestamp_frequency;
>

I think you just want DIV_ROUND_UP(a, b)

> +
> +   *pMaxDeviation = clock_period > device_period ? clock_period :
> device_period;
>

MAX2(a, b) is your friend.

> +
> +   return VK_SUCCESS;
> +}
> +
>  /* vk_icd.h does not declare this function, so we declare it here to
>   * suppress Wmissing-prototypes.
>   */
> diff --git a/src/intel/vulkan/anv_extensions.py
> b/src/intel/vulkan/anv_extensions.py
> index d4915c95013..a8535964da7 100644
> --- a/src/intel/vulkan/anv_extensions.py
> +++ b/src/intel/vulkan/anv_extensions.py
> @@ -126,6 +126,7 @@ EXTENSIONS = [
>      Extension('VK_EXT_vertex_attribute_divisor',          3, True),
>      Extension('VK_EXT_post_depth_coverage',               1,
> 'device->info.gen >= 9'),
>      Extension('VK_EXT_sampler_filter_minmax',             1,
> 'device->info.gen >= 9'),
> +    Extension('VK_EXT_calibrated_timestamps',             1, True),
>  ]
>
>  class VkVersion:
> diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
> index c43b5ef9e06..1bdf040c1a3 100644
> --- a/src/intel/vulkan/anv_gem.c
> +++ b/src/intel/vulkan/anv_gem.c
> @@ -423,6 +423,19 @@ anv_gem_fd_to_handle(struct anv_device *device, int
> fd)
>     return args.handle;
>  }
>
> +int
> +anv_gem_reg_read(struct anv_device *device, uint32_t offset, uint64_t
> *result)
> +{
> +   struct drm_i915_reg_read args = {
> +      .offset = offset
> +   };
> +
> +   int ret = anv_ioctl(device->fd, DRM_IOCTL_I915_REG_READ, &args);
> +
> +   *result = args.val;
> +   return ret;
> +}
> +
>  #ifndef SYNC_IOC_MAGIC
>  /* duplicated from linux/sync_file.h to avoid build-time dependency
>   * on new (v4.7) kernel headers.  Once distro's are mostly using
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 599b903f25c..08376b00c8e 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1103,6 +1103,8 @@ int anv_gem_get_aperture(int fd, uint64_t *size);
>  int anv_gem_gpu_get_reset_stats(struct anv_device *device,
>                                  uint32_t *active, uint32_t *pending);
>  int anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle);
> +int anv_gem_reg_read(struct anv_device *device,
> +                     uint32_t offset, uint64_t *result);
>  uint32_t anv_gem_fd_to_handle(struct anv_device *device, int fd);
>  int anv_gem_set_caching(struct anv_device *device, uint32_t gem_handle,
> uint32_t caching);
>  int anv_gem_set_domain(struct anv_device *device, uint32_t gem_handle,
> --
> 2.19.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 14713 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel