<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">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. </div><div class="gmail_quote"> </div><div class="gmail_quote">On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> 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 <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com">keithp@keithp.com</a>> --- 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]; </blockquote><div> </div><div>break? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + 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) </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + 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); </blockquote><div> </div><div>Are relative times really useful? I suspect it doesn't save you more than a couple of lines and it makes the interface weird. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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; + </blockquote><div> </div><div>An assert(!fence->destroyed) in here might be useful to guard against double-frees. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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; </blockquote><div> </div><div>Why are we sleeping for 0.1s before we return? That seems fishy. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + + pthread_mutex_lock(&wsi->wait_mutex); + ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(100000000ull)); </blockquote><div> </div><div>What's with the magic number? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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; </blockquote><div> </div><div>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 </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +} + +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; </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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); </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +} + +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); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +} + 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 <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </blockquote></div> </div></div>