[5/7] vulkan: add VK_EXT_display_control [v5]
diff mbox

Message ID 20180615025256.10657-6-keithp@keithp.com
State New
Headers show

Commit Message

Keith Packard June 15, 2018, 2:52 a.m. UTC
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.

v2: Remove DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT flag. This has
    been removed from the proposed kernel API.

    Add NULL parameter to drmCrtcQueueSequence ioctl as we
    don't care what sequence the event was actually queued to.

v3: Adapt to pthread clock switch to MONOTONIC

v4: Fix scope for wsi_display_mode andwsi_display_connector allocs

    Suggested-by: Jason Ekstrand <jason@jlekstrand.net>

v5: Adopt Jason Ekstrand's coding conventions

    Declare variables at first use, eliminate extra whitespace between
    types and names. Wrap lines to 80 columns.

    Use wsi_rel_to_abs_time helper function to convert relative
    timeouts to absolute timeouts without causing overflow.

    Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 src/vulkan/wsi/wsi_common.h         |  10 +
 src/vulkan/wsi/wsi_common_display.c | 307 +++++++++++++++++++++++++++-
 src/vulkan/wsi/wsi_common_display.h |  29 +++
 3 files changed, 345 insertions(+), 1 deletion(-)

Comments

Jason Ekstrand June 20, 2018, 12:28 a.m. UTC | #1
Sorry for the flood of comments.  Most of it's pretty trivial.  The one
that has me the most concerned is the random appearence of 0.1s in some
strange places.  Other than that, I'm reasonably happy with it.

On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <keithp@keithp.com> wrote:

> This extension provides fences and frame count information to direct
> display contexts. It uses new kernel ioctls to provide 64-bits of
> vblank sequence and nanosecond resolution.
>
> v2: Remove DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT flag. This has
>     been removed from the proposed kernel API.
>
>     Add NULL parameter to drmCrtcQueueSequence ioctl as we
>     don't care what sequence the event was actually queued to.
>
> v3: Adapt to pthread clock switch to MONOTONIC
>
> v4: Fix scope for wsi_display_mode andwsi_display_connector allocs
>
>     Suggested-by: Jason Ekstrand <jason@jlekstrand.net>
>
> v5: Adopt Jason Ekstrand's coding conventions
>
>     Declare variables at first use, eliminate extra whitespace between
>     types and names. Wrap lines to 80 columns.
>
>     Use wsi_rel_to_abs_time helper function to convert relative
>     timeouts to absolute timeouts without causing overflow.
>
>     Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/vulkan/wsi/wsi_common.h         |  10 +
>  src/vulkan/wsi/wsi_common_display.c | 307 +++++++++++++++++++++++++++-
>  src/vulkan/wsi/wsi_common_display.h |  29 +++
>  3 files changed, 345 insertions(+), 1 deletion(-)
>
> diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
> index 054aad23c1c..081fe1dcf8d 100644
> --- a/src/vulkan/wsi/wsi_common.h
> +++ b/src/vulkan/wsi/wsi_common.h
> @@ -66,6 +66,16 @@ struct wsi_format_modifier_properties_list {
>     struct wsi_format_modifier_properties *modifier_properties;
>  };
>
> +struct wsi_fence {
> +   VkDevice                     device;
> +   const struct wsi_device      *wsi_device;
> +   VkDisplayKHR                 display;
> +   const VkAllocationCallbacks  *alloc;
> +   bool                         (*wait)(struct wsi_fence *fence,
> +                                        bool absolute, uint64_t timeout);
> +   void                         (*destroy)(struct wsi_fence *fence);
> +};
> +
>  struct wsi_interface;
>
>  #define VK_ICD_WSI_PLATFORM_MAX (VK_ICD_WSI_PLATFORM_DISPLAY + 1)
> diff --git a/src/vulkan/wsi/wsi_common_display.c
> b/src/vulkan/wsi/wsi_common_display.c
> index 504f7741d73..40eaa6a322e 100644
> --- a/src/vulkan/wsi/wsi_common_display.c
> +++ b/src/vulkan/wsi/wsi_common_display.c
> @@ -79,6 +79,7 @@ typedef struct wsi_display_connector {
>     struct list_head             display_modes;
>     wsi_display_mode             *current_mode;
>     drmModeModeInfo              current_drm_mode;
> +   uint32_t                     dpms_property;
>  #ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
>     xcb_randr_output_t           output;
>  #endif
> @@ -132,6 +133,15 @@ struct wsi_display_swapchain {
>     struct wsi_display_image     images[0];
>  };
>
> +struct wsi_display_fence {
> +   struct wsi_fence             base;
> +   bool                         event_received;
> +   bool                         destroyed;
> +   uint64_t                     sequence;
> +};
> +
> +static uint64_t fence_sequence;
> +
>  ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_mode, VkDisplayModeKHR)
>  ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_connector, VkDisplayKHR)
>
> @@ -307,6 +317,19 @@ wsi_display_get_connector(struct wsi_device
> *wsi_device,
>
>     connector->connected = drm_connector->connection !=
> DRM_MODE_DISCONNECTED;
>
> +   /* Look for a DPMS property */
> +   for (int p = 0; p < drm_connector->count_props; p++) {
> +      drmModePropertyPtr prop = drmModeGetProperty(wsi->fd,
> +
>  drm_connector->props[p]);
> +      if (!prop)
> +         continue;
> +      if (prop->flags & DRM_MODE_PROP_ENUM) {
> +         if (!strcmp(prop->name, "DPMS"))
> +            connector->dpms_property = drm_connector->props[p];
>

break?


> +      }
> +      drmModeFreeProperty(prop);
> +   }
> +
>     /* Mark all connector modes as invalid */
>     wsi_display_invalidate_connector_modes(wsi_device, connector);
>
> @@ -663,7 +686,7 @@ wsi_display_surface_get_capabilities2ext(VkIcdSurfaceBase
> *icd_surface,
>     caps->currentTransform = khr_caps.currentTransform;
>     caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
>     caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
> -   caps->supportedSurfaceCounters = 0;
> +   caps->supportedSurfaceCounters = VK_SURFACE_COUNTER_VBLANK_EXT;
>     return ret;
>  }
>
> @@ -896,12 +919,21 @@ static void wsi_display_page_flip_handler(int fd,
>     wsi_display_page_flip_handler2(fd, frame, sec, usec, 0, data);
>  }
>
> +static void wsi_display_vblank_handler(int fd, unsigned int frame,
> +                                       unsigned int sec, unsigned int
> usec,
> +                                       void *data);
> +
> +static void wsi_display_sequence_handler(int fd, uint64_t frame,
> +                                         uint64_t ns, uint64_t user_data);
> +
>  static drmEventContext event_context = {
>     .version = DRM_EVENT_CONTEXT_VERSION,
>     .page_flip_handler = wsi_display_page_flip_handler,
>  #if DRM_EVENT_CONTEXT_VERSION >= 3
>     .page_flip_handler2 = wsi_display_page_flip_handler2,
>  #endif
> +   .vblank_handler = wsi_display_vblank_handler,
> +   .sequence_handler = wsi_display_sequence_handler,
>  };
>
>  static void *
> @@ -1168,6 +1200,147 @@ bail:
>
>  }
>
> +static bool
> +wsi_display_fence_wait(struct wsi_fence *fence_wsi,
> +                       bool absolute,
> +                       uint64_t timeout)
>

Would it make more sense for this function to return a VkResult?  Then you
could tell the difference between success, timeout, and some other
failure.  I guess the only other thing to return would be
VK_ERROR_DEVICE_LOST which seems pretty harsh but, then again,
pthread_timed_wait just failed so that's also really bad.


> +{
> +   const struct wsi_device *wsi_device = fence_wsi->wsi_device;
> +   struct wsi_display *wsi =
> +      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_
> PLATFORM_DISPLAY];
> +   struct wsi_display_fence *fence = (struct wsi_display_fence *)
> fence_wsi;
> +
> +   if (!absolute)
> +      timeout = wsi_rel_to_abs_time(timeout);
>

Are relative times really useful?  I suspect it doesn't save you more than
a couple of lines and it makes the interface weird.


> +
> +   wsi_display_debug("%9lu wait fence %lu %ld\n",
> +                     pthread_self(), fence->sequence,
> +                     (int64_t) (timeout - wsi_get_current_monotonic()));
> +   wsi_display_debug_code(uint64_t start_ns =
> wsi_get_current_monotonic());
> +   pthread_mutex_lock(&wsi->wait_mutex);
> +
> +   bool value = false;
> +   int ret = 0;
> +   for (;;) {
> +      if (fence->event_received) {
> +         wsi_display_debug("%9lu fence %lu passed\n",
> +                           pthread_self(), fence->sequence);
> +         value = true;
> +         break;
> +      }
> +
> +      if (ret == ETIMEDOUT) {
> +         wsi_display_debug("%9lu fence %lu timeout\n",
> +                           pthread_self(), fence->sequence);
> +         value = false;
> +         break;
> +      }
> +
> +      ret = wsi_display_wait_for_event(wsi, timeout);
> +
> +      if (ret && ret != ETIMEDOUT) {
> +         wsi_display_debug("%9lu fence %lu error\n",
> +                           pthread_self(), fence->sequence);
> +         value = false;
> +         break;
> +      }
> +   }
> +   pthread_mutex_unlock(&wsi->wait_mutex);
> +   wsi_display_debug("%9lu fence wait %f ms\n",
> +                     pthread_self(),
> +                     ((int64_t) (wsi_get_current_monotonic() - start_ns))
> /
> +                     1.0e6);
> +   return value;
> +}
> +
> +static void
> +wsi_display_fence_check_free(struct wsi_display_fence *fence)
> +{
> +   if (fence->event_received && fence->destroyed)
> +      vk_free(fence->base.alloc, fence);
> +}
> +
> +static void
> +wsi_display_fence_destroy(struct wsi_fence *fence_wsi)
> +{
> +   struct wsi_display_fence *fence = (struct wsi_display_fence *)
> fence_wsi;
> +
>

An assert(!fence->destroyed) in here might be useful to guard against
double-frees.


> +   fence->destroyed = true;
> +   wsi_display_fence_check_free(fence);
> +}
> +
> +static struct wsi_display_fence *
> +wsi_display_fence_alloc(VkDevice device,
> +                        const struct wsi_device *wsi_device,
> +                        VkDisplayKHR display,
> +                        const VkAllocationCallbacks *allocator)
> +{
> +   struct wsi_display_fence *fence =
> +      vk_zalloc(allocator, sizeof (*fence),
> +                8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> +
> +   if (!fence)
> +      return NULL;
> +
> +   fence->base.device = device;
> +   fence->base.display = display;
> +   fence->base.wsi_device = wsi_device;
> +   fence->base.alloc = allocator;
> +   fence->base.wait = wsi_display_fence_wait;
> +   fence->base.destroy = wsi_display_fence_destroy;
> +   fence->event_received = false;
> +   fence->destroyed = false;
> +   fence->sequence = ++fence_sequence;
> +   return fence;
> +}
> +
> +static VkResult
> +wsi_register_vblank_event(struct wsi_display_fence *fence,
> +                          const struct wsi_device *wsi_device,
> +                          VkDisplayKHR display,
> +                          uint32_t flags,
> +                          uint64_t frame_requested,
> +                          uint64_t *frame_queued)
> +{
> +   struct wsi_display *wsi =
> +      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_
> PLATFORM_DISPLAY];
> +   struct wsi_display_connector *connector =
> +      wsi_display_connector_from_handle(display);
> +
> +   if (wsi->fd < 0)
> +      return VK_ERROR_INITIALIZATION_FAILED;
> +
> +   for (;;) {
> +      int ret = drmCrtcQueueSequence(wsi->fd, connector->crtc_id,
> +                                     flags,
> +                                     frame_requested,
> +                                     frame_queued,
> +                                     (uint64_t) fence);
> +
> +      if (!ret)
> +         return VK_SUCCESS;
> +
> +      if (errno != ENOMEM) {
> +         wsi_display_debug("queue vblank event %lu failed\n",
> fence->sequence);
> +         struct timespec delay = {
> +            .tv_sec = 0,
> +            .tv_nsec = 100000000ull,
> +         };
> +         nanosleep(&delay, NULL);
> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
>

Why are we sleeping for 0.1s before we return?  That seems fishy.


> +      }
> +
> +      pthread_mutex_lock(&wsi->wait_mutex);
> +      ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(
> 100000000ull));
>

What's with the magic number?


> +      pthread_mutex_unlock(&wsi->wait_mutex);
> +
> +      if (ret) {
> +         wsi_display_debug("vblank queue full, event wait failed\n");
> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
> +      }
> +   }
> +}
> +
>  /*
>   * Check to see if the kernel has no flip queued and if there's an image
>   * waiting to be displayed.
> @@ -1963,3 +2136,135 @@ wsi_get_randr_output_display(VkPhysicalDevice
> physical_device,
>  }
>
>  #endif
> +
> +/* VK_EXT_display_control */
> +VkResult
> +wsi_display_power_control(VkDevice device,
> +                          struct wsi_device *wsi_device,
> +                          VkDisplayKHR display,
> +                          const VkDisplayPowerInfoEXT *display_power_info)
> +{
> +   struct wsi_display *wsi =
> +      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_
> PLATFORM_DISPLAY];
> +   struct wsi_display_connector *connector =
> +      wsi_display_connector_from_handle(display);
> +   int mode;
> +
> +   if (wsi->fd < 0)
> +      return VK_ERROR_INITIALIZATION_FAILED;
> +
> +   switch (display_power_info->powerState) {
> +   case VK_DISPLAY_POWER_STATE_OFF_EXT:
> +      mode = DRM_MODE_DPMS_OFF;
> +      break;
> +   case VK_DISPLAY_POWER_STATE_SUSPEND_EXT:
> +      mode = DRM_MODE_DPMS_SUSPEND;
> +      break;
> +   default:
> +      mode = DRM_MODE_DPMS_ON;
> +      break;
> +   }
> +   drmModeConnectorSetProperty(wsi->fd,
> +                               connector->id,
> +                               connector->dpms_property,
> +                               mode);
> +   return VK_SUCCESS;
> +}
> +
> +VkResult
> +wsi_register_device_event(VkDevice device,
> +                          struct wsi_device *wsi_device,
> +                          const VkDeviceEventInfoEXT *device_event_info,
> +                          const VkAllocationCallbacks *allocator,
> +                          struct wsi_fence **fence_p)
> +{
> +   return VK_ERROR_FEATURE_NOT_PRESENT;
>

I don't think we're allowed to just not implemnet this.  At the very least,
we should accept the event and never trigger it.  Better would be to
actually wire up hotplug detection.  I have no idea how insane that would
be to do. :-P


> +}
> +
> +VkResult
> +wsi_register_display_event(VkDevice device,
> +                           struct wsi_device *wsi_device,
> +                           VkDisplayKHR display,
> +                           const VkDisplayEventInfoEXT
> *display_event_info,
> +                           const VkAllocationCallbacks *allocator,
> +                           struct wsi_fence **fence_p)
> +{
> +   struct wsi_display_fence *fence;
> +   VkResult ret = VK_ERROR_FEATURE_NOT_PRESENT;
> +
> +   switch (display_event_info->displayEvent) {
> +   case VK_DISPLAY_EVENT_TYPE_FIRST_PIXEL_OUT_EXT:
> +
> +      fence = wsi_display_fence_alloc(device, wsi_device, display,
> allocator);
> +
> +      if (!fence)
> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
>

Both RegisterDeviceEvent and RegisterDisplayEvent say they can only return
VK_SUCCESS.  We should submit a MR against the extensions to also allow
OUT_OF_HOST_MEMORY at the very least.


> +
> +      ret = wsi_register_vblank_event(fence, wsi_device, display,
> +                                      DRM_CRTC_SEQUENCE_RELATIVE, 1,
> NULL);
> +
> +      if (ret == VK_SUCCESS)
> +         *fence_p = &fence->base;
> +      else if (fence != NULL)
> +         vk_free(allocator, fence);
> +
> +      break;
> +   }
> +
> +   return ret;
> +}
> +
> +
> +VkResult
> +wsi_get_swapchain_counter(VkDevice device,
> +                          struct wsi_device *wsi_device,
> +                          VkSwapchainKHR _swapchain,
> +                          VkSurfaceCounterFlagBitsEXT flag_bits,
> +                          uint64_t *value)
> +{
> +   struct wsi_display *wsi =
> +      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_
> PLATFORM_DISPLAY];
> +   struct wsi_display_swapchain *swapchain =
> +      (struct wsi_display_swapchain *) wsi_swapchain_from_handle(_
> swapchain);
> +   struct wsi_display_connector *connector =
> +      wsi_display_mode_from_handle(swapchain->surface->
> displayMode)->connector;
> +
> +   if (wsi->fd < 0)
> +      return VK_ERROR_INITIALIZATION_FAILED;
> +
> +   if (!connector->active) {
> +      *value = 0;
> +      return VK_SUCCESS;
> +   }
> +
> +   int ret = drmCrtcGetSequence(wsi->fd, connector->crtc_id, value, NULL);
> +   if (ret)
> +      *value = 0;
> +
> +   return VK_SUCCESS;
> +}
> +
> +static void wsi_display_fence_event_handler(struct wsi_display_fence
> *fence)
> +{
> +   fence->event_received = true;
> +   wsi_display_fence_check_free(fence);
>
+}
> +
> +static void wsi_display_vblank_handler(int fd, unsigned int frame,
> +                                       unsigned int sec, unsigned int
> usec,
> +                                       void *data)
> +{
> +   struct wsi_display_fence *fence = data;
> +
> +   wsi_display_fence_event_handler(fence);
> +}
> +
> +static void wsi_display_sequence_handler(int fd, uint64_t frame,
> +                                         uint64_t nsec, uint64_t
> user_data)
> +{
> +   struct wsi_display_fence *fence =
> +      (struct wsi_display_fence *) (uintptr_t) user_data;
> +
> +   wsi_display_fence_event_handler(fence);
>

Any particular reason to put these all the way down here?  I think my
preference would be to move wsi_display_fence_event_handler to right after
wsi_display_fence_check_free and give it a predeclaration (instead of these
two) and then move the sequence and vblank handlers to right above
event_context since they're just little wrappers around
wsi_display_fence_check_free.  Sorry if that's a bit petty but it was hard
to find wsi_display_fence_check_free all the way down here and it's really
needed in order to understand the pseudo reference counting you're doing
with fences.


> +}
> +
> diff --git a/src/vulkan/wsi/wsi_common_display.h
> b/src/vulkan/wsi/wsi_common_display.h
> index 58447d2c6eb..2f760f825fb 100644
> --- a/src/vulkan/wsi/wsi_common_display.h
> +++ b/src/vulkan/wsi/wsi_common_display.h
> @@ -96,4 +96,33 @@ wsi_get_randr_output_display(VkPhysicalDevice
>  physical_device,
>
>  #endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
>
> +/* VK_EXT_display_control */
> +VkResult
> +wsi_display_power_control(VkDevice                      device,
> +                          struct wsi_device             *wsi_device,
> +                          VkDisplayKHR                  display,
> +                          const VkDisplayPowerInfoEXT
>  *display_power_info);
> +
> +VkResult
> +wsi_register_device_event(VkDevice                      device,
> +                          struct wsi_device             *wsi_device,
> +                          const VkDeviceEventInfoEXT
> *device_event_info,
> +                          const VkAllocationCallbacks   *allocator,
> +                          struct wsi_fence              **fence);
> +
> +VkResult
> +wsi_register_display_event(VkDevice                     device,
> +                           struct wsi_device            *wsi_device,
> +                           VkDisplayKHR                 display,
> +                           const VkDisplayEventInfoEXT
> *display_event_info,
> +                           const VkAllocationCallbacks  *allocator,
> +                           struct wsi_fence             **fence);
> +
> +VkResult
> +wsi_get_swapchain_counter(VkDevice                      device,
> +                          struct wsi_device             *wsi_device,
> +                          VkSwapchainKHR                swapchain,
> +                          VkSurfaceCounterFlagBitsEXT   flag_bits,
> +                          uint64_t                      *value);
> +
>  #endif
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Keith Packard June 20, 2018, 4:44 a.m. UTC | #2
Jason Ekstrand <jason@jlekstrand.net> writes:


>> +      if (!prop)
>> +         continue;
>> +      if (prop->flags & DRM_MODE_PROP_ENUM) {
>> +         if (!strcmp(prop->name, "DPMS"))
>> +            connector->dpms_property = drm_connector->props[p];
>>
>
> break?

Not break; I need to free the property. However, an early exit from the
loop seems reasonable. How about:

   for (int p = 0; connector->dpms_property == 0 && p < drm_connector->count_props; p++) {

This skips the whole sequence if the property has already been found, or
stops as soon as it has.

>> +static bool
>> +wsi_display_fence_wait(struct wsi_fence *fence_wsi,
>> +                       bool absolute,
>> +                       uint64_t timeout)
>>
>
> Would it make more sense for this function to return a VkResult?  Then you
> could tell the difference between success, timeout, and some other
> failure.  I guess the only other thing to return would be
> VK_ERROR_DEVICE_LOST which seems pretty harsh but, then again,
> pthread_timed_wait just failed so that's also really bad.

That's a good idea. The boolean return is pretty ambiguous. I copied
that from the radv internal fence API, which could also benefit from
this change. I've changed the API and adjusted the anv and radv code to
match. It reads a lot better now.

>> +   if (!absolute)
>> +      timeout = wsi_rel_to_abs_time(timeout);
>>
>
> Are relative times really useful?  I suspect it doesn't save you more than
> a couple of lines and it makes the interface weird.

No. Relative timeouts aren't actually used anywhere either. I've removed them.

I did catch a mistake in the anv driver looking at this -- the !waitAll
code wasn't bothering to check the fences if the time had already
passed, so an application polling would never catch the fences being
ready. I've changed the while (current_time < timeout) {} to a do {}
while (current_time < timeout) loop.

>> +static void
>> +wsi_display_fence_destroy(struct wsi_fence *fence_wsi)
>> +{
>> +   struct wsi_display_fence *fence = (struct wsi_display_fence *)
>> fence_wsi;
>> +
>>
>
> An assert(!fence->destroyed) in here might be useful to guard against
> double-frees.

Sure. I was under the impression that application bugs weren't supposed
to be rigorously checked in the implementation though? When should I be
checking API usage issues?

>> +      if (!ret)
>> +         return VK_SUCCESS;
>> +
>> +      if (errno != ENOMEM) {
>> +         wsi_display_debug("queue vblank event %lu failed\n",
>> fence->sequence);
>> +         struct timespec delay = {
>> +            .tv_sec = 0,
>> +            .tv_nsec = 100000000ull,
>> +         };
>> +         nanosleep(&delay, NULL);
>> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
>>
>
> Why are we sleeping for 0.1s before we return?  That seems fishy.

Yeah, the kernel API is not great. There's a finite queue which can be
consumed with both flip events and vblank wait events. If that fills,
we'll get an error back. The only way to empty it is to have some events
get delivered, and those will only get delivered after a vblank happens.

It's an application bug that triggers this -- requesting too many vblank
events. Throttling the application so it doesn't just spin makes it
possible to stop it.

>> +      pthread_mutex_lock(&wsi->wait_mutex);
>> +      ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(
>> 100000000ull));
>>
>
> What's with the magic number?

0.1s -- a value which is longer than any display time, but short enough
to catch things like DPMS off or VT switch without unduly delaying the
application.

>> +VkResult
>> +wsi_register_device_event(VkDevice device,
>> +                          struct wsi_device *wsi_device,
>> +                          const VkDeviceEventInfoEXT *device_event_info,
>> +                          const VkAllocationCallbacks *allocator,
>> +                          struct wsi_fence **fence_p)
>> +{
>> +   return VK_ERROR_FEATURE_NOT_PRESENT;
>>
>
> I don't think we're allowed to just not implemnet this.  At the very least,
> we should accept the event and never trigger it.  Better would be to
> actually wire up hotplug detection.  I have no idea how insane that would
> be to do. :-P

It's not a big deal to implement, I just didn't need it. I suppose the
test suite will be unhappy with this? Let me know if you want to insist
on having it implemented.

> Both RegisterDeviceEvent and RegisterDisplayEvent say they can only return
> VK_SUCCESS.  We should submit a MR against the extensions to also allow
> OUT_OF_HOST_MEMORY at the very least.

There's already weasel words in the section on memory allocation that
says the command must generate VK_ERROR_OUT_OF_HOST_MEMORY when
allocation fails. But, it would be nice for these APIs to be documented
as possibly returning that value.

> Any particular reason to put these all the way down here?  I think my
> preference would be to move wsi_display_fence_event_handler to right after
> wsi_display_fence_check_free and give it a predeclaration (instead of these
> two) and then move the sequence and vblank handlers to right above
> event_context since they're just little wrappers around
> wsi_display_fence_check_free.  Sorry if that's a bit petty but it was hard
> to find wsi_display_fence_check_free all the way down here and it's really
> needed in order to understand the pseudo reference counting you're doing
> with fences.

Sure; that's easy enough and reduces us to a single forward function
declaration instead of two.

I've implemented all of the indicated changes above; I'll send out a
replacement patch series shortly.
Jason Ekstrand June 20, 2018, 9:52 p.m. UTC | #3
On Tue, Jun 19, 2018 at 9:44 PM, Keith Packard <keithp@keithp.com> wrote:

> Jason Ekstrand <jason@jlekstrand.net> writes:
>
>
> >> +      if (!prop)
> >> +         continue;
> >> +      if (prop->flags & DRM_MODE_PROP_ENUM) {
> >> +         if (!strcmp(prop->name, "DPMS"))
> >> +            connector->dpms_property = drm_connector->props[p];
> >>
> >
> > break?
>
> Not break; I need to free the property. However, an early exit from the
> loop seems reasonable. How about:
>
>    for (int p = 0; connector->dpms_property == 0 && p <
> drm_connector->count_props; p++) {
>
> This skips the whole sequence if the property has already been found, or
> stops as soon as it has.
>

That seems good to me.  Unless, of course, DPMS is something we expect to
change over time somehow.  Then again, we don't handle that at all right
now so meh.  Let's go with what you wrote above for now.


> >> +static bool
> >> +wsi_display_fence_wait(struct wsi_fence *fence_wsi,
> >> +                       bool absolute,
> >> +                       uint64_t timeout)
> >>
> >
> > Would it make more sense for this function to return a VkResult?  Then
> you
> > could tell the difference between success, timeout, and some other
> > failure.  I guess the only other thing to return would be
> > VK_ERROR_DEVICE_LOST which seems pretty harsh but, then again,
> > pthread_timed_wait just failed so that's also really bad.
>
> That's a good idea. The boolean return is pretty ambiguous. I copied
> that from the radv internal fence API, which could also benefit from
> this change. I've changed the API and adjusted the anv and radv code to
> match. It reads a lot better now.
>

Cool.


> >> +   if (!absolute)
> >> +      timeout = wsi_rel_to_abs_time(timeout);
> >>
> >
> > Are relative times really useful?  I suspect it doesn't save you more
> than
> > a couple of lines and it makes the interface weird.
>
> No. Relative timeouts aren't actually used anywhere either. I've removed
> them.
>

Sounds good.


> I did catch a mistake in the anv driver looking at this -- the !waitAll
> code wasn't bothering to check the fences if the time had already
> passed, so an application polling would never catch the fences being
> ready. I've changed the while (current_time < timeout) {} to a do {}
> while (current_time < timeout) loop.
>

Yeah, that was going to be a problem for someone if they ever decided to
busy-loop in the app. :-)


> >> +static void
> >> +wsi_display_fence_destroy(struct wsi_fence *fence_wsi)
> >> +{
> >> +   struct wsi_display_fence *fence = (struct wsi_display_fence *)
> >> fence_wsi;
> >> +
> >>
> >
> > An assert(!fence->destroyed) in here might be useful to guard against
> > double-frees.
>
> Sure. I was under the impression that application bugs weren't supposed
> to be rigorously checked in the implementation though? When should I be
> checking API usage issues?
>

They shouldn't be and that's why I'm a fan of making them asserts which get
compiled out instead of actual checks.  Also, I find this pseudo reference
counting to be somewhat confusing and adding asserts informs the reader of
the assumptions made.


> >> +      if (!ret)
> >> +         return VK_SUCCESS;
> >> +
> >> +      if (errno != ENOMEM) {
> >> +         wsi_display_debug("queue vblank event %lu failed\n",
> >> fence->sequence);
> >> +         struct timespec delay = {
> >> +            .tv_sec = 0,
> >> +            .tv_nsec = 100000000ull,
> >> +         };
> >> +         nanosleep(&delay, NULL);
> >> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
> >>
> >
> > Why are we sleeping for 0.1s before we return?  That seems fishy.
>
> Yeah, the kernel API is not great. There's a finite queue which can be
> consumed with both flip events and vblank wait events. If that fills,
> we'll get an error back. The only way to empty it is to have some events
> get delivered, and those will only get delivered after a vblank happens.
>
> It's an application bug that triggers this -- requesting too many vblank
> events. Throttling the application so it doesn't just spin makes it
> possible to stop it.
>
> >> +      pthread_mutex_lock(&wsi->wait_mutex);
> >> +      ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(
> >> 100000000ull));
> >>
> >
> > What's with the magic number?
>
> 0.1s -- a value which is longer than any display time, but short enough
> to catch things like DPMS off or VT switch without unduly delaying the
> application.
>
> >> +VkResult
> >> +wsi_register_device_event(VkDevice device,
> >> +                          struct wsi_device *wsi_device,
> >> +                          const VkDeviceEventInfoEXT
> *device_event_info,
> >> +                          const VkAllocationCallbacks *allocator,
> >> +                          struct wsi_fence **fence_p)
> >> +{
> >> +   return VK_ERROR_FEATURE_NOT_PRESENT;
> >>
> >
> > I don't think we're allowed to just not implemnet this.  At the very
> least,
> > we should accept the event and never trigger it.  Better would be to
> > actually wire up hotplug detection.  I have no idea how insane that would
> > be to do. :-P
>
> It's not a big deal to implement, I just didn't need it. I suppose the
> test suite will be unhappy with this? Let me know if you want to insist
> on having it implemented.
>

What test suite?  Honestly, I know of no code anywhere that actually uses
this API for anything other than VR headsets.

I guess it's kind-of a question of how much effort we want to put into
this.  One option would be to add VK_KHR_display support to vkcube and make
it automatically show up on all your displays using hotplug events.

If we're going to not care, returning VK_ERROR_FEATURE_NOT_PRESENT is
probably the best thing to do since at least the app has feedback.


> > Both RegisterDeviceEvent and RegisterDisplayEvent say they can only
> return
> > VK_SUCCESS.  We should submit a MR against the extensions to also allow
> > OUT_OF_HOST_MEMORY at the very least.
>
> There's already weasel words in the section on memory allocation that
> says the command must generate VK_ERROR_OUT_OF_HOST_MEMORY when
> allocation fails. But, it would be nice for these APIs to be documented
> as possibly returning that value.
>

Yeah.  It's probably a 2-line change.


> > Any particular reason to put these all the way down here?  I think my
> > preference would be to move wsi_display_fence_event_handler to right
> after
> > wsi_display_fence_check_free and give it a predeclaration (instead of
> these
> > two) and then move the sequence and vblank handlers to right above
> > event_context since they're just little wrappers around
> > wsi_display_fence_check_free.  Sorry if that's a bit petty but it was
> hard
> > to find wsi_display_fence_check_free all the way down here and it's
> really
> > needed in order to understand the pseudo reference counting you're doing
> > with fences.
>
> Sure; that's easy enough and reduces us to a single forward function
> declaration instead of two.
>
> I've implemented all of the indicated changes above; I'll send out a
> replacement patch series shortly.
>

Awesome.  I think we're really close on this one.

--Jason
Keith Packard June 20, 2018, 10:49 p.m. UTC | #4
Jason Ekstrand <jason@jlekstrand.net> writes:

> That seems good to me.  Unless, of course, DPMS is something we expect to
> change over time somehow.  Then again, we don't handle that at all right
> now so meh.  Let's go with what you wrote above for now.

It's not even the dpms value, it's the dpms property itself, which
DRM never changes.

> They shouldn't be and that's why I'm a fan of making them asserts which get
> compiled out instead of actual checks.  Also, I find this pseudo reference
> counting to be somewhat confusing and adding asserts informs the reader of
> the assumptions made.

Ok, I've added this.

> What test suite?  Honestly, I know of no code anywhere that actually uses
> this API for anything other than VR headsets.
>
> I guess it's kind-of a question of how much effort we want to put into
> this.  One option would be to add VK_KHR_display support to vkcube and make
> it automatically show up on all your displays using hotplug events.
>
> If we're going to not care, returning VK_ERROR_FEATURE_NOT_PRESENT is
> probably the best thing to do since at least the app has feedback.

Not caring seems best to me -- the Vulkan display API isn't capable of
supporting a "real" window system; for that, you'd really want to use
DRM directly and create some way to share that with Vulkan like the
extension I wrote to pass the DRM master FD into the driver at init time.

> Awesome.  I think we're really close on this one.

I'll send out the current series and you can see if you like it.

Patch
diff mbox

diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
index 054aad23c1c..081fe1dcf8d 100644
--- a/src/vulkan/wsi/wsi_common.h
+++ b/src/vulkan/wsi/wsi_common.h
@@ -66,6 +66,16 @@  struct wsi_format_modifier_properties_list {
    struct wsi_format_modifier_properties *modifier_properties;
 };
 
+struct wsi_fence {
+   VkDevice                     device;
+   const struct wsi_device      *wsi_device;
+   VkDisplayKHR                 display;
+   const VkAllocationCallbacks  *alloc;
+   bool                         (*wait)(struct wsi_fence *fence,
+                                        bool absolute, uint64_t timeout);
+   void                         (*destroy)(struct wsi_fence *fence);
+};
+
 struct wsi_interface;
 
 #define VK_ICD_WSI_PLATFORM_MAX (VK_ICD_WSI_PLATFORM_DISPLAY + 1)
diff --git a/src/vulkan/wsi/wsi_common_display.c b/src/vulkan/wsi/wsi_common_display.c
index 504f7741d73..40eaa6a322e 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -79,6 +79,7 @@  typedef struct wsi_display_connector {
    struct list_head             display_modes;
    wsi_display_mode             *current_mode;
    drmModeModeInfo              current_drm_mode;
+   uint32_t                     dpms_property;
 #ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
    xcb_randr_output_t           output;
 #endif
@@ -132,6 +133,15 @@  struct wsi_display_swapchain {
    struct wsi_display_image     images[0];
 };
 
+struct wsi_display_fence {
+   struct wsi_fence             base;
+   bool                         event_received;
+   bool                         destroyed;
+   uint64_t                     sequence;
+};
+
+static uint64_t fence_sequence;
+
 ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_mode, VkDisplayModeKHR)
 ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_connector, VkDisplayKHR)
 
@@ -307,6 +317,19 @@  wsi_display_get_connector(struct wsi_device *wsi_device,
 
    connector->connected = drm_connector->connection != DRM_MODE_DISCONNECTED;
 
+   /* Look for a DPMS property */
+   for (int p = 0; p < drm_connector->count_props; p++) {
+      drmModePropertyPtr prop = drmModeGetProperty(wsi->fd,
+                                                   drm_connector->props[p]);
+      if (!prop)
+         continue;
+      if (prop->flags & DRM_MODE_PROP_ENUM) {
+         if (!strcmp(prop->name, "DPMS"))
+            connector->dpms_property = drm_connector->props[p];
+      }
+      drmModeFreeProperty(prop);
+   }
+
    /* Mark all connector modes as invalid */
    wsi_display_invalidate_connector_modes(wsi_device, connector);
 
@@ -663,7 +686,7 @@  wsi_display_surface_get_capabilities2ext(VkIcdSurfaceBase *icd_surface,
    caps->currentTransform = khr_caps.currentTransform;
    caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
    caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
-   caps->supportedSurfaceCounters = 0;
+   caps->supportedSurfaceCounters = VK_SURFACE_COUNTER_VBLANK_EXT;
    return ret;
 }
 
@@ -896,12 +919,21 @@  static void wsi_display_page_flip_handler(int fd,
    wsi_display_page_flip_handler2(fd, frame, sec, usec, 0, data);
 }
 
+static void wsi_display_vblank_handler(int fd, unsigned int frame,
+                                       unsigned int sec, unsigned int usec,
+                                       void *data);
+
+static void wsi_display_sequence_handler(int fd, uint64_t frame,
+                                         uint64_t ns, uint64_t user_data);
+
 static drmEventContext event_context = {
    .version = DRM_EVENT_CONTEXT_VERSION,
    .page_flip_handler = wsi_display_page_flip_handler,
 #if DRM_EVENT_CONTEXT_VERSION >= 3
    .page_flip_handler2 = wsi_display_page_flip_handler2,
 #endif
+   .vblank_handler = wsi_display_vblank_handler,
+   .sequence_handler = wsi_display_sequence_handler,
 };
 
 static void *
@@ -1168,6 +1200,147 @@  bail:
 
 }
 
+static bool
+wsi_display_fence_wait(struct wsi_fence *fence_wsi,
+                       bool absolute,
+                       uint64_t timeout)
+{
+   const struct wsi_device *wsi_device = fence_wsi->wsi_device;
+   struct wsi_display *wsi =
+      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
+   struct wsi_display_fence *fence = (struct wsi_display_fence *) fence_wsi;
+
+   if (!absolute)
+      timeout = wsi_rel_to_abs_time(timeout);
+
+   wsi_display_debug("%9lu wait fence %lu %ld\n",
+                     pthread_self(), fence->sequence,
+                     (int64_t) (timeout - wsi_get_current_monotonic()));
+   wsi_display_debug_code(uint64_t start_ns = wsi_get_current_monotonic());
+   pthread_mutex_lock(&wsi->wait_mutex);
+
+   bool value = false;
+   int ret = 0;
+   for (;;) {
+      if (fence->event_received) {
+         wsi_display_debug("%9lu fence %lu passed\n",
+                           pthread_self(), fence->sequence);
+         value = true;
+         break;
+      }
+
+      if (ret == ETIMEDOUT) {
+         wsi_display_debug("%9lu fence %lu timeout\n",
+                           pthread_self(), fence->sequence);
+         value = false;
+         break;
+      }
+
+      ret = wsi_display_wait_for_event(wsi, timeout);
+
+      if (ret && ret != ETIMEDOUT) {
+         wsi_display_debug("%9lu fence %lu error\n",
+                           pthread_self(), fence->sequence);
+         value = false;
+         break;
+      }
+   }
+   pthread_mutex_unlock(&wsi->wait_mutex);
+   wsi_display_debug("%9lu fence wait %f ms\n",
+                     pthread_self(),
+                     ((int64_t) (wsi_get_current_monotonic() - start_ns)) /
+                     1.0e6);
+   return value;
+}
+
+static void
+wsi_display_fence_check_free(struct wsi_display_fence *fence)
+{
+   if (fence->event_received && fence->destroyed)
+      vk_free(fence->base.alloc, fence);
+}
+
+static void
+wsi_display_fence_destroy(struct wsi_fence *fence_wsi)
+{
+   struct wsi_display_fence *fence = (struct wsi_display_fence *) fence_wsi;
+
+   fence->destroyed = true;
+   wsi_display_fence_check_free(fence);
+}
+
+static struct wsi_display_fence *
+wsi_display_fence_alloc(VkDevice device,
+                        const struct wsi_device *wsi_device,
+                        VkDisplayKHR display,
+                        const VkAllocationCallbacks *allocator)
+{
+   struct wsi_display_fence *fence =
+      vk_zalloc(allocator, sizeof (*fence),
+                8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
+
+   if (!fence)
+      return NULL;
+
+   fence->base.device = device;
+   fence->base.display = display;
+   fence->base.wsi_device = wsi_device;
+   fence->base.alloc = allocator;
+   fence->base.wait = wsi_display_fence_wait;
+   fence->base.destroy = wsi_display_fence_destroy;
+   fence->event_received = false;
+   fence->destroyed = false;
+   fence->sequence = ++fence_sequence;
+   return fence;
+}
+
+static VkResult
+wsi_register_vblank_event(struct wsi_display_fence *fence,
+                          const struct wsi_device *wsi_device,
+                          VkDisplayKHR display,
+                          uint32_t flags,
+                          uint64_t frame_requested,
+                          uint64_t *frame_queued)
+{
+   struct wsi_display *wsi =
+      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
+   struct wsi_display_connector *connector =
+      wsi_display_connector_from_handle(display);
+
+   if (wsi->fd < 0)
+      return VK_ERROR_INITIALIZATION_FAILED;
+
+   for (;;) {
+      int ret = drmCrtcQueueSequence(wsi->fd, connector->crtc_id,
+                                     flags,
+                                     frame_requested,
+                                     frame_queued,
+                                     (uint64_t) fence);
+
+      if (!ret)
+         return VK_SUCCESS;
+
+      if (errno != ENOMEM) {
+         wsi_display_debug("queue vblank event %lu failed\n", fence->sequence);
+         struct timespec delay = {
+            .tv_sec = 0,
+            .tv_nsec = 100000000ull,
+         };
+         nanosleep(&delay, NULL);
+         return VK_ERROR_OUT_OF_HOST_MEMORY;
+      }
+
+      pthread_mutex_lock(&wsi->wait_mutex);
+      ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(100000000ull));
+      pthread_mutex_unlock(&wsi->wait_mutex);
+
+      if (ret) {
+         wsi_display_debug("vblank queue full, event wait failed\n");
+         return VK_ERROR_OUT_OF_HOST_MEMORY;
+      }
+   }
+}
+
 /*
  * Check to see if the kernel has no flip queued and if there's an image
  * waiting to be displayed.
@@ -1963,3 +2136,135 @@  wsi_get_randr_output_display(VkPhysicalDevice physical_device,
 }
 
 #endif
+
+/* VK_EXT_display_control */
+VkResult
+wsi_display_power_control(VkDevice device,
+                          struct wsi_device *wsi_device,
+                          VkDisplayKHR display,
+                          const VkDisplayPowerInfoEXT *display_power_info)
+{
+   struct wsi_display *wsi =
+      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
+   struct wsi_display_connector *connector =
+      wsi_display_connector_from_handle(display);
+   int mode;
+
+   if (wsi->fd < 0)
+      return VK_ERROR_INITIALIZATION_FAILED;
+
+   switch (display_power_info->powerState) {
+   case VK_DISPLAY_POWER_STATE_OFF_EXT:
+      mode = DRM_MODE_DPMS_OFF;
+      break;
+   case VK_DISPLAY_POWER_STATE_SUSPEND_EXT:
+      mode = DRM_MODE_DPMS_SUSPEND;
+      break;
+   default:
+      mode = DRM_MODE_DPMS_ON;
+      break;
+   }
+   drmModeConnectorSetProperty(wsi->fd,
+                               connector->id,
+                               connector->dpms_property,
+                               mode);
+   return VK_SUCCESS;
+}
+
+VkResult
+wsi_register_device_event(VkDevice device,
+                          struct wsi_device *wsi_device,
+                          const VkDeviceEventInfoEXT *device_event_info,
+                          const VkAllocationCallbacks *allocator,
+                          struct wsi_fence **fence_p)
+{
+   return VK_ERROR_FEATURE_NOT_PRESENT;
+}
+
+VkResult
+wsi_register_display_event(VkDevice device,
+                           struct wsi_device *wsi_device,
+                           VkDisplayKHR display,
+                           const VkDisplayEventInfoEXT *display_event_info,
+                           const VkAllocationCallbacks *allocator,
+                           struct wsi_fence **fence_p)
+{
+   struct wsi_display_fence *fence;
+   VkResult ret = VK_ERROR_FEATURE_NOT_PRESENT;
+
+   switch (display_event_info->displayEvent) {
+   case VK_DISPLAY_EVENT_TYPE_FIRST_PIXEL_OUT_EXT:
+
+      fence = wsi_display_fence_alloc(device, wsi_device, display, allocator);
+
+      if (!fence)
+         return VK_ERROR_OUT_OF_HOST_MEMORY;
+
+      ret = wsi_register_vblank_event(fence, wsi_device, display,
+                                      DRM_CRTC_SEQUENCE_RELATIVE, 1, NULL);
+
+      if (ret == VK_SUCCESS)
+         *fence_p = &fence->base;
+      else if (fence != NULL)
+         vk_free(allocator, fence);
+
+      break;
+   }
+
+   return ret;
+}
+
+
+VkResult
+wsi_get_swapchain_counter(VkDevice device,
+                          struct wsi_device *wsi_device,
+                          VkSwapchainKHR _swapchain,
+                          VkSurfaceCounterFlagBitsEXT flag_bits,
+                          uint64_t *value)
+{
+   struct wsi_display *wsi =
+      (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
+   struct wsi_display_swapchain *swapchain =
+      (struct wsi_display_swapchain *) wsi_swapchain_from_handle(_swapchain);
+   struct wsi_display_connector *connector =
+      wsi_display_mode_from_handle(swapchain->surface->displayMode)->connector;
+
+   if (wsi->fd < 0)
+      return VK_ERROR_INITIALIZATION_FAILED;
+
+   if (!connector->active) {
+      *value = 0;
+      return VK_SUCCESS;
+   }
+
+   int ret = drmCrtcGetSequence(wsi->fd, connector->crtc_id, value, NULL);
+   if (ret)
+      *value = 0;
+
+   return VK_SUCCESS;
+}
+
+static void wsi_display_fence_event_handler(struct wsi_display_fence *fence)
+{
+   fence->event_received = true;
+   wsi_display_fence_check_free(fence);
+}
+
+static void wsi_display_vblank_handler(int fd, unsigned int frame,
+                                       unsigned int sec, unsigned int usec,
+                                       void *data)
+{
+   struct wsi_display_fence *fence = data;
+
+   wsi_display_fence_event_handler(fence);
+}
+
+static void wsi_display_sequence_handler(int fd, uint64_t frame,
+                                         uint64_t nsec, uint64_t user_data)
+{
+   struct wsi_display_fence *fence =
+      (struct wsi_display_fence *) (uintptr_t) user_data;
+
+   wsi_display_fence_event_handler(fence);
+}
+
diff --git a/src/vulkan/wsi/wsi_common_display.h b/src/vulkan/wsi/wsi_common_display.h
index 58447d2c6eb..2f760f825fb 100644
--- a/src/vulkan/wsi/wsi_common_display.h
+++ b/src/vulkan/wsi/wsi_common_display.h
@@ -96,4 +96,33 @@  wsi_get_randr_output_display(VkPhysicalDevice   physical_device,
 
 #endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
 
+/* VK_EXT_display_control */
+VkResult
+wsi_display_power_control(VkDevice                      device,
+                          struct wsi_device             *wsi_device,
+                          VkDisplayKHR                  display,
+                          const VkDisplayPowerInfoEXT   *display_power_info);
+
+VkResult
+wsi_register_device_event(VkDevice                      device,
+                          struct wsi_device             *wsi_device,
+                          const VkDeviceEventInfoEXT    *device_event_info,
+                          const VkAllocationCallbacks   *allocator,
+                          struct wsi_fence              **fence);
+
+VkResult
+wsi_register_display_event(VkDevice                     device,
+                           struct wsi_device            *wsi_device,
+                           VkDisplayKHR                 display,
+                           const VkDisplayEventInfoEXT  *display_event_info,
+                           const VkAllocationCallbacks  *allocator,
+                           struct wsi_fence             **fence);
+
+VkResult
+wsi_get_swapchain_counter(VkDevice                      device,
+                          struct wsi_device             *wsi_device,
+                          VkSwapchainKHR                swapchain,
+                          VkSurfaceCounterFlagBitsEXT   flag_bits,
+                          uint64_t                      *value);
+
 #endif