* [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter
@ 2018-06-15 2:52 Keith Packard
2018-06-15 2:52 ` [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4] Keith Packard
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: dri-devel
Here's a couple of reasonably straightforward extensions implemented
for both anv and radv drivers.
VK_EXT_display_surface_counter is a very simple extension which
adds an API, vkGetPhysicalSurfaceCapabilities2EXT, to extend the
existing vkGetPhysicalDeviceSurfaceCapabilitiesKHR and
vkGetPhysicalDeviceSurfaceCapablities2KHR interfaces.
The new interface uses a slightly different structure that includes a
new flags word, "supportedSurfaceCounters". This new field describes
the counters available from the underlying WSI layer. As this
extension doesn't provide any counters itself, each WSI layer
initializes this to zero for now.
VK_EXT_display_control is an extension specific to display surfaces
(those created via the KHR_display extension). This extension adds
DPMS support and fences which signal when vblank or monitor hotplug
events happen.
I've implemented the DPMS support and the vblank fence stuff; I
haven't bothered hooking up the monitor hotplug bits, although it
would be fairly simple.
To make the fences work on anv, I had to refactor the existing fence
waiting code to add the ability to spin waiting for any of a set of
heterogeneous fences (a mixture of syncobj and bo fences) to become
ready; this mirrors the same functionality in the radv driver,
although anv hadn't needed it before as it only supported homogenous
fence types (either all syncobj or all bo).
I've created a separate patch for this refactoring to try and reduce
the difficulty in reviewing that part.
-keith
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-16 18:22 ` Jason Ekstrand
2018-06-15 2:52 ` [PATCH 2/7] anv: Add VK_EXT_display_surface_counter to anv driver [v2] Keith Packard
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
v2: Thanks to kisak
Fix spelling of VkSurfaceCapabilities2EXT in wsi_common_wayland.c,
it was using ext instead of EXT.
Fix spelling of VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT
v3: Fix wayland WSI, regularize spelling
Misspelled 'surface' in get_capabilities2ext
Remove extra _ from get_capabilities_2ext in a couple of places
v4: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace
between types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
src/vulkan/wsi/wsi_common.c | 12 ++++++++++++
src/vulkan/wsi/wsi_common.h | 6 ++++++
src/vulkan/wsi/wsi_common_display.c | 27 +++++++++++++++++++++++++++
src/vulkan/wsi/wsi_common_private.h | 3 +++
src/vulkan/wsi/wsi_common_wayland.c | 27 +++++++++++++++++++++++++++
src/vulkan/wsi/wsi_common_x11.c | 27 +++++++++++++++++++++++++++
6 files changed, 102 insertions(+)
diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
index 142c5d8fe58..91d0b72e1ba 100644
--- a/src/vulkan/wsi/wsi_common.c
+++ b/src/vulkan/wsi/wsi_common.c
@@ -710,6 +710,18 @@ wsi_common_get_surface_capabilities2(struct wsi_device *wsi_device,
pSurfaceCapabilities);
}
+VkResult
+wsi_common_get_surface_capabilities2ext(
+ struct wsi_device *wsi_device,
+ VkSurfaceKHR _surface,
+ VkSurfaceCapabilities2EXT *pSurfaceCapabilities)
+{
+ ICD_FROM_HANDLE(VkIcdSurfaceBase, surface, _surface);
+ struct wsi_interface *iface = wsi_device->wsi[surface->platform];
+
+ return iface->get_capabilities2ext(surface, pSurfaceCapabilities);
+}
+
VkResult
wsi_common_get_surface_formats(struct wsi_device *wsi_device,
VkSurfaceKHR _surface,
diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
index 61b1de59d7f..054aad23c1c 100644
--- a/src/vulkan/wsi/wsi_common.h
+++ b/src/vulkan/wsi/wsi_common.h
@@ -178,6 +178,12 @@ wsi_common_get_surface_present_modes(struct wsi_device *wsi_device,
uint32_t *pPresentModeCount,
VkPresentModeKHR *pPresentModes);
+VkResult
+wsi_common_get_surface_capabilities2ext(
+ struct wsi_device *wsi_device,
+ VkSurfaceKHR surface,
+ VkSurfaceCapabilities2EXT *pSurfaceCapabilities);
+
VkResult
wsi_common_get_images(VkSwapchainKHR _swapchain,
uint32_t *pSwapchainImageCount,
diff --git a/src/vulkan/wsi/wsi_common_display.c b/src/vulkan/wsi/wsi_common_display.c
index e140e71c518..504f7741d73 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -641,6 +641,32 @@ wsi_display_surface_get_capabilities2(VkIcdSurfaceBase *icd_surface,
&caps->surfaceCapabilities);
}
+static VkResult
+wsi_display_surface_get_capabilities2ext(VkIcdSurfaceBase *icd_surface,
+ VkSurfaceCapabilities2EXT *caps)
+{
+ VkSurfaceCapabilitiesKHR khr_caps;
+ VkResult ret;
+
+ assert(caps->sType == VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT);
+ ret = wsi_display_surface_get_capabilities(icd_surface, &khr_caps);
+ if (ret)
+ return ret;
+
+ caps->minImageCount = khr_caps.minImageCount;
+ caps->maxImageCount = khr_caps.maxImageCount;
+ caps->currentExtent = khr_caps.currentExtent;
+ caps->minImageExtent = khr_caps.minImageExtent;
+ caps->maxImageExtent = khr_caps.maxImageExtent;
+ caps->maxImageArrayLayers = khr_caps.maxImageArrayLayers;
+ caps->supportedTransforms = khr_caps.supportedTransforms;
+ caps->currentTransform = khr_caps.currentTransform;
+ caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
+ caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
+ caps->supportedSurfaceCounters = 0;
+ return ret;
+}
+
static const struct {
VkFormat format;
uint32_t drm_format;
@@ -1393,6 +1419,7 @@ wsi_display_init_wsi(struct wsi_device *wsi_device,
wsi->base.get_support = wsi_display_surface_get_support;
wsi->base.get_capabilities = wsi_display_surface_get_capabilities;
wsi->base.get_capabilities2 = wsi_display_surface_get_capabilities2;
+ wsi->base.get_capabilities2ext = wsi_display_surface_get_capabilities2ext;
wsi->base.get_formats = wsi_display_surface_get_formats;
wsi->base.get_formats2 = wsi_display_surface_get_formats2;
wsi->base.get_present_modes = wsi_display_surface_get_present_modes;
diff --git a/src/vulkan/wsi/wsi_common_private.h b/src/vulkan/wsi/wsi_common_private.h
index 3d502b9fc9d..b97d2d8ba06 100644
--- a/src/vulkan/wsi/wsi_common_private.h
+++ b/src/vulkan/wsi/wsi_common_private.h
@@ -109,6 +109,9 @@ struct wsi_interface {
VkResult (*get_capabilities2)(VkIcdSurfaceBase *surface,
const void *info_next,
VkSurfaceCapabilities2KHR* pSurfaceCapabilities);
+ VkResult (*get_capabilities2ext)(VkIcdSurfaceBase *surface,
+ VkSurfaceCapabilities2EXT*
+ pSurfaceCapabilities);
VkResult (*get_formats)(VkIcdSurfaceBase *surface,
struct wsi_device *wsi_device,
uint32_t* pSurfaceFormatCount,
diff --git a/src/vulkan/wsi/wsi_common_wayland.c b/src/vulkan/wsi/wsi_common_wayland.c
index ec38a4e578f..908d8676e2a 100644
--- a/src/vulkan/wsi/wsi_common_wayland.c
+++ b/src/vulkan/wsi/wsi_common_wayland.c
@@ -524,6 +524,32 @@ wsi_wl_surface_get_capabilities2(VkIcdSurfaceBase *surface,
return wsi_wl_surface_get_capabilities(surface, &caps->surfaceCapabilities);
}
+static VkResult
+wsi_wl_surface_get_capabilities2ext(VkIcdSurfaceBase *surface,
+ VkSurfaceCapabilities2EXT* caps)
+{
+ VkSurfaceCapabilitiesKHR khr_caps;
+ VkResult ret;
+
+ assert(caps->sType == VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT);
+ ret = wsi_wl_surface_get_capabilities(surface, &khr_caps);
+ if (ret)
+ return ret;
+
+ caps->minImageCount = khr_caps.minImageCount;
+ caps->maxImageCount = khr_caps.maxImageCount;
+ caps->currentExtent = khr_caps.currentExtent;
+ caps->minImageExtent = khr_caps.minImageExtent;
+ caps->maxImageExtent = khr_caps.maxImageExtent;
+ caps->maxImageArrayLayers = khr_caps.maxImageArrayLayers;
+ caps->supportedTransforms = khr_caps.supportedTransforms;
+ caps->currentTransform = khr_caps.currentTransform;
+ caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
+ caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
+ caps->supportedSurfaceCounters = 0;
+ return ret;
+}
+
static VkResult
wsi_wl_surface_get_formats(VkIcdSurfaceBase *icd_surface,
struct wsi_device *wsi_device,
@@ -1012,6 +1038,7 @@ wsi_wl_init_wsi(struct wsi_device *wsi_device,
wsi->base.get_support = wsi_wl_surface_get_support;
wsi->base.get_capabilities = wsi_wl_surface_get_capabilities;
wsi->base.get_capabilities2 = wsi_wl_surface_get_capabilities2;
+ wsi->base.get_capabilities2ext = wsi_wl_surface_get_capabilities2ext;
wsi->base.get_formats = wsi_wl_surface_get_formats;
wsi->base.get_formats2 = wsi_wl_surface_get_formats2;
wsi->base.get_present_modes = wsi_wl_surface_get_present_modes;
diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c
index 20d7cf5a2c8..129e30913c3 100644
--- a/src/vulkan/wsi/wsi_common_x11.c
+++ b/src/vulkan/wsi/wsi_common_x11.c
@@ -547,6 +547,32 @@ x11_surface_get_capabilities2(VkIcdSurfaceBase *icd_surface,
return x11_surface_get_capabilities(icd_surface, &caps->surfaceCapabilities);
}
+static VkResult
+x11_surface_get_capabilities2ext(VkIcdSurfaceBase *icd_surface,
+ VkSurfaceCapabilities2EXT *caps)
+{
+ VkSurfaceCapabilitiesKHR khr_caps;
+ VkResult ret;
+
+ assert(caps->sType == VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT);
+ ret = x11_surface_get_capabilities(icd_surface, &khr_caps);
+ if (ret)
+ return ret;
+
+ caps->minImageCount = khr_caps.minImageCount;
+ caps->maxImageCount = khr_caps.maxImageCount;
+ caps->currentExtent = khr_caps.currentExtent;
+ caps->minImageExtent = khr_caps.minImageExtent;
+ caps->maxImageExtent = khr_caps.maxImageExtent;
+ caps->maxImageArrayLayers = khr_caps.maxImageArrayLayers;
+ caps->supportedTransforms = khr_caps.supportedTransforms;
+ caps->currentTransform = khr_caps.currentTransform;
+ caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
+ caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
+ caps->supportedSurfaceCounters = 0;
+ return ret;
+}
+
static VkResult
x11_surface_get_formats(VkIcdSurfaceBase *surface,
struct wsi_device *wsi_device,
@@ -1471,6 +1497,7 @@ wsi_x11_init_wsi(struct wsi_device *wsi_device,
wsi->base.get_support = x11_surface_get_support;
wsi->base.get_capabilities = x11_surface_get_capabilities;
wsi->base.get_capabilities2 = x11_surface_get_capabilities2;
+ wsi->base.get_capabilities2ext = x11_surface_get_capabilities2ext;
wsi->base.get_formats = x11_surface_get_formats;
wsi->base.get_formats2 = x11_surface_get_formats2;
wsi->base.get_present_modes = x11_surface_get_present_modes;
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/7] anv: Add VK_EXT_display_surface_counter to anv driver [v2]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
2018-06-15 2:52 ` [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4] Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-15 2:52 ` [PATCH 3/7] radv: Add VK_EXT_display_surface_counter to radv driver Keith Packard
` (4 subsequent siblings)
6 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: dri-devel
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
v2:
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand <jason@jlekstrand.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_wsi.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py b/src/intel/vulkan/anv_extensions.py
index 4bffbcb5a2a..68e545a40f8 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -111,6 +111,7 @@ EXTENSIONS = [
Extension('VK_EXT_acquire_xlib_display', 1, 'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
Extension('VK_EXT_debug_report', 8, True),
Extension('VK_EXT_direct_mode_display', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
+ Extension('VK_EXT_display_surface_counter', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_external_memory_dma_buf', 1, True),
Extension('VK_EXT_global_priority', 1,
'device->has_context_priority'),
diff --git a/src/intel/vulkan/anv_wsi.c b/src/intel/vulkan/anv_wsi.c
index a7efb1416fa..1403601e9c0 100644
--- a/src/intel/vulkan/anv_wsi.c
+++ b/src/intel/vulkan/anv_wsi.c
@@ -120,6 +120,18 @@ VkResult anv_GetPhysicalDeviceSurfaceCapabilities2KHR(
pSurfaceCapabilities);
}
+VkResult anv_GetPhysicalDeviceSurfaceCapabilities2EXT(
+ VkPhysicalDevice physicalDevice,
+ VkSurfaceKHR surface,
+ VkSurfaceCapabilities2EXT* pSurfaceCapabilities)
+{
+ ANV_FROM_HANDLE(anv_physical_device, device, physicalDevice);
+
+ return wsi_common_get_surface_capabilities2ext(&device->wsi_device,
+ surface,
+ pSurfaceCapabilities);
+}
+
VkResult anv_GetPhysicalDeviceSurfaceFormatsKHR(
VkPhysicalDevice physicalDevice,
VkSurfaceKHR surface,
--
2.17.1
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/7] radv: Add VK_EXT_display_surface_counter to radv driver
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
2018-06-15 2:52 ` [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4] Keith Packard
2018-06-15 2:52 ` [PATCH 2/7] anv: Add VK_EXT_display_surface_counter to anv driver [v2] Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-15 2:52 ` [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2] Keith Packard
` (3 subsequent siblings)
6 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
Signed-off-by: Keith Packard <keithp@keithp.com>
---
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_wsi.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/src/amd/vulkan/radv_extensions.py b/src/amd/vulkan/radv_extensions.py
index 65ce7349016..601a345b114 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -89,6 +89,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_display_surface_counter', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_debug_report', 9, True),
Extension('VK_EXT_depth_range_unrestricted', 1, True),
Extension('VK_EXT_descriptor_indexing', 2, True),
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index 2840b666727..20484177135 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -103,6 +103,18 @@ VkResult radv_GetPhysicalDeviceSurfaceCapabilities2KHR(
pSurfaceCapabilities);
}
+VkResult radv_GetPhysicalDeviceSurfaceCapabilities2EXT(
+ VkPhysicalDevice physicalDevice,
+ VkSurfaceKHR surface,
+ VkSurfaceCapabilities2EXT* pSurfaceCapabilities)
+{
+ RADV_FROM_HANDLE(radv_physical_device, device, physicalDevice);
+
+ return wsi_common_get_surface_capabilities2ext(&device->wsi_device,
+ surface,
+ pSurfaceCapabilities);
+}
+
VkResult radv_GetPhysicalDeviceSurfaceFormatsKHR(
VkPhysicalDevice physicalDevice,
VkSurfaceKHR surface,
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
` (2 preceding siblings ...)
2018-06-15 2:52 ` [PATCH 3/7] radv: Add VK_EXT_display_surface_counter to radv driver Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-19 23:26 ` Jason Ekstrand
2018-06-15 2:52 ` [PATCH 5/7] vulkan: add VK_EXT_display_control [v5] Keith Packard
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
Handle the case where the set of fences to wait for is not all of the
same type by either waiting for them sequentially (waitAll), or
polling them until the timer has expired (!waitAll). We hope the
latter case is not common.
While the current code makes sure that it always has fences of only
one type, that will not be true when we add WSI fences. Split out this
refactoring to make merging that clearer.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
src/intel/vulkan/anv_queue.c | 105 +++++++++++++++++++++++++++++------
1 file changed, 88 insertions(+), 17 deletions(-)
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 6fe04a0a19d..8df99c84549 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -459,12 +459,32 @@ gettime_ns(void)
return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;
}
+static uint64_t anv_get_absolute_timeout(uint64_t timeout)
+{
+ if (timeout == 0)
+ return 0;
+ uint64_t current_time = gettime_ns();
+
+ timeout = MIN2(INT64_MAX - current_time, timeout);
+
+ return (current_time + timeout);
+}
+
+static int64_t anv_get_relative_timeout(uint64_t abs_timeout)
+{
+ uint64_t now = gettime_ns();
+
+ if (abs_timeout < now)
+ return 0;
+ return abs_timeout - now;
+}
+
static VkResult
anv_wait_for_syncobj_fences(struct anv_device *device,
uint32_t fenceCount,
const VkFence *pFences,
bool waitAll,
- uint64_t _timeout)
+ uint64_t abs_timeout_ns)
{
uint32_t *syncobjs = vk_zalloc(&device->alloc,
sizeof(*syncobjs) * fenceCount, 8,
@@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(struct anv_device *device,
syncobjs[i] = impl->syncobj;
}
- int64_t abs_timeout_ns = 0;
- if (_timeout > 0) {
- uint64_t current_ns = gettime_ns();
-
- /* Add but saturate to INT32_MAX */
- if (current_ns + _timeout < current_ns)
- abs_timeout_ns = INT64_MAX;
- else if (current_ns + _timeout > INT64_MAX)
- abs_timeout_ns = INT64_MAX;
- else
- abs_timeout_ns = current_ns + _timeout;
- }
-
/* The gem_syncobj_wait ioctl may return early due to an inherent
* limitation in the way it computes timeouts. Loop until we've actually
* passed the timeout.
@@ -665,6 +672,67 @@ done:
return result;
}
+static VkResult
+anv_wait_for_fences(struct anv_device *device,
+ uint32_t fenceCount,
+ const VkFence *pFences,
+ bool waitAll,
+ uint64_t abs_timeout)
+{
+ VkResult result = VK_SUCCESS;
+
+ if (fenceCount <= 1 || waitAll) {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ switch (fence->permanent.type) {
+ case ANV_FENCE_TYPE_BO:
+ result = anv_wait_for_bo_fences(
+ device, 1, &pFences[i], true,
+ anv_get_relative_timeout(abs_timeout));
+ break;
+ case ANV_FENCE_TYPE_SYNCOBJ:
+ result = anv_wait_for_syncobj_fences(device, 1, &pFences[i],
+ true, abs_timeout);
+ break;
+ case ANV_FENCE_TYPE_NONE:
+ result = VK_SUCCESS;
+ break;
+ }
+ if (result != VK_SUCCESS)
+ return result;
+ }
+ } else {
+ while (gettime_ns() < abs_timeout) {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+ if (anv_wait_for_fences(device, 1, &pFences[i], true, 0) == VK_SUCCESS)
+ return VK_SUCCESS;
+ }
+ }
+ result = VK_TIMEOUT;
+ }
+ return result;
+}
+
+static bool anv_all_fences_syncobj(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ if (fence->permanent.type != ANV_FENCE_TYPE_SYNCOBJ)
+ return false;
+ }
+ return true;
+}
+
+static bool anv_all_fences_bo(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ if (fence->permanent.type != ANV_FENCE_TYPE_BO)
+ return false;
+ }
+ return true;
+}
+
VkResult anv_WaitForFences(
VkDevice _device,
uint32_t fenceCount,
@@ -677,12 +745,15 @@ VkResult anv_WaitForFences(
if (unlikely(device->lost))
return VK_ERROR_DEVICE_LOST;
- if (device->instance->physicalDevice.has_syncobj_wait) {
+ if (anv_all_fences_syncobj(fenceCount, pFences)) {
return anv_wait_for_syncobj_fences(device, fenceCount, pFences,
- waitAll, timeout);
- } else {
+ waitAll, anv_get_absolute_timeout(timeout));
+ } else if (anv_all_fences_bo(fenceCount, pFences)) {
return anv_wait_for_bo_fences(device, fenceCount, pFences,
waitAll, timeout);
+ } else {
+ return anv_wait_for_fences(device, fenceCount, pFences,
+ waitAll, anv_get_absolute_timeout(timeout));
}
}
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
` (3 preceding siblings ...)
2018-06-15 2:52 ` [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2] Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-20 0:28 ` Jason Ekstrand
2018-06-15 2:52 ` [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2] Keith Packard
2018-06-15 2:52 ` [PATCH 7/7] radv: add VK_EXT_display_control to radv driver [v3] Keith Packard
6 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
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];
+ }
+ 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
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
` (4 preceding siblings ...)
2018-06-15 2:52 ` [PATCH 5/7] vulkan: add VK_EXT_display_control [v5] Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
2018-06-20 0:33 ` Jason Ekstrand
2018-06-15 2:52 ` [PATCH 7/7] radv: add VK_EXT_display_control to radv driver [v3] Keith Packard
6 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
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: 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>
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_private.h | 4 ++
src/intel/vulkan/anv_queue.c | 22 +++++++
src/intel/vulkan/anv_wsi_display.c | 97 ++++++++++++++++++++++++++++++
4 files changed, 124 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py b/src/intel/vulkan/anv_extensions.py
index 68e545a40f8..8c010e9280b 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -111,6 +111,7 @@ EXTENSIONS = [
Extension('VK_EXT_acquire_xlib_display', 1, 'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
Extension('VK_EXT_debug_report', 8, True),
Extension('VK_EXT_direct_mode_display', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
+ Extension('VK_EXT_display_control', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_display_surface_counter', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_external_memory_dma_buf', 1, True),
Extension('VK_EXT_global_priority', 1,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index fb91bc33046..c81885979ad 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2133,6 +2133,7 @@ enum anv_fence_type {
ANV_FENCE_TYPE_NONE = 0,
ANV_FENCE_TYPE_BO,
ANV_FENCE_TYPE_SYNCOBJ,
+ ANV_FENCE_TYPE_WSI,
};
enum anv_bo_fence_state {
@@ -2167,6 +2168,9 @@ struct anv_fence_impl {
/** DRM syncobj handle for syncobj-based fences */
uint32_t syncobj;
+
+ /** WSI fence */
+ struct wsi_fence *fence_wsi;
};
};
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 8df99c84549..073e65acf5e 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -324,6 +324,10 @@ anv_fence_impl_cleanup(struct anv_device *device,
anv_gem_syncobj_destroy(device, impl->syncobj);
break;
+ case ANV_FENCE_TYPE_WSI:
+ impl->fence_wsi->destroy(impl->fence_wsi);
+ break;
+
default:
unreachable("Invalid fence type");
}
@@ -672,6 +676,21 @@ done:
return result;
}
+static VkResult
+anv_wait_for_wsi_fence(struct anv_device *device,
+ const VkFence _fence,
+ uint64_t abs_timeout)
+{
+ ANV_FROM_HANDLE(anv_fence, fence, _fence);
+
+ struct anv_fence_impl *impl = &fence->permanent;
+ bool expired = impl->fence_wsi->wait(impl->fence_wsi, true, abs_timeout);
+
+ if (!expired)
+ return VK_TIMEOUT;
+ return VK_SUCCESS;
+}
+
static VkResult
anv_wait_for_fences(struct anv_device *device,
uint32_t fenceCount,
@@ -694,6 +713,9 @@ anv_wait_for_fences(struct anv_device *device,
result = anv_wait_for_syncobj_fences(device, 1, &pFences[i],
true, abs_timeout);
break;
+ case ANV_FENCE_TYPE_WSI:
+ result = anv_wait_for_wsi_fence(device, pFences[i], abs_timeout);
+ break;
case ANV_FENCE_TYPE_NONE:
result = VK_SUCCESS;
break;
diff --git a/src/intel/vulkan/anv_wsi_display.c b/src/intel/vulkan/anv_wsi_display.c
index f749a8d98f7..cd736bcdd74 100644
--- a/src/intel/vulkan/anv_wsi_display.c
+++ b/src/intel/vulkan/anv_wsi_display.c
@@ -168,3 +168,100 @@ anv_GetRandROutputDisplayEXT(VkPhysicalDevice physical_device,
display);
}
#endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
+
+/* VK_EXT_display_control */
+
+VkResult
+anv_DisplayPowerControlEXT(VkDevice _device,
+ VkDisplayKHR display,
+ const VkDisplayPowerInfoEXT *display_power_info)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+
+ return wsi_display_power_control(
+ _device, &device->instance->physicalDevice.wsi_device,
+ display, display_power_info);
+}
+
+VkResult
+anv_RegisterDeviceEventEXT(VkDevice _device,
+ const VkDeviceEventInfoEXT *device_event_info,
+ const VkAllocationCallbacks *allocator,
+ VkFence *_fence)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ const VkAllocationCallbacks *alloc;
+ struct anv_fence *fence;
+ VkResult ret;
+
+ if (allocator)
+ alloc = allocator;
+ else
+ alloc = &device->instance->alloc;
+
+ fence = vk_alloc(alloc, sizeof (*fence), 8,
+ VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
+ if (!fence)
+ return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
+
+ fence->permanent.type = ANV_FENCE_TYPE_WSI;
+
+ ret = wsi_register_device_event(_device,
+ &device->instance->physicalDevice.wsi_device,
+ device_event_info,
+ alloc,
+ &fence->permanent.fence_wsi);
+ if (ret == VK_SUCCESS)
+ *_fence = anv_fence_to_handle(fence);
+ else
+ vk_free(alloc, fence);
+ return ret;
+}
+
+VkResult
+anv_RegisterDisplayEventEXT(VkDevice _device,
+ VkDisplayKHR display,
+ const VkDisplayEventInfoEXT *display_event_info,
+ const VkAllocationCallbacks *allocator,
+ VkFence *_fence)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ const VkAllocationCallbacks *alloc;
+ struct anv_fence *fence;
+ VkResult ret;
+
+ if (allocator)
+ alloc = allocator;
+ else
+ alloc = &device->instance->alloc;
+
+ fence = vk_zalloc2(&device->alloc, allocator, sizeof (*fence), 8,
+ VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
+ if (!fence)
+ return VK_ERROR_OUT_OF_HOST_MEMORY;
+
+ fence->permanent.type = ANV_FENCE_TYPE_WSI;
+
+ ret = wsi_register_display_event(
+ _device, &device->instance->physicalDevice.wsi_device,
+ display, display_event_info, alloc, &(fence->permanent.fence_wsi));
+
+ if (ret == VK_SUCCESS)
+ *_fence = anv_fence_to_handle(fence);
+ else
+ vk_free(alloc, fence);
+ return ret;
+}
+
+VkResult
+anv_GetSwapchainCounterEXT(VkDevice _device,
+ VkSwapchainKHR swapchain,
+ VkSurfaceCounterFlagBitsEXT flag_bits,
+ uint64_t *value)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+
+ return wsi_get_swapchain_counter(
+ _device, &device->instance->physicalDevice.wsi_device,
+ swapchain, flag_bits, value);
+}
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 7/7] radv: add VK_EXT_display_control to radv driver [v3]
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
` (5 preceding siblings ...)
2018-06-15 2:52 ` [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2] Keith Packard
@ 2018-06-15 2:52 ` Keith Packard
6 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-15 2:52 UTC (permalink / raw)
To: mesa-dev; +Cc: Keith Packard, dri-devel
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:
Rework fence integration into the driver so that waiting for
any of a mixture of fence types (wsi, driver or syncobjs)
causes the driver to poll, while a list of just syncobjs or
just driver fences will block. When we get syncobjs for wsi
fences, we'll adapt to use them.
v3: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
src/amd/vulkan/radv_device.c | 68 +++++++++++++-----
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
src/amd/vulkan/radv_wsi_display.c | 113 ++++++++++++++++++++++++++++++
4 files changed, 167 insertions(+), 16 deletions(-)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 59ee503c8c2..fd80db1c9b4 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -3240,6 +3240,7 @@ VkResult radv_CreateFence(
if (!fence)
return vk_error(device->instance, VK_ERROR_OUT_OF_HOST_MEMORY);
+ fence->fence_wsi = NULL;
fence->submitted = false;
fence->signalled = !!(pCreateInfo->flags & VK_FENCE_CREATE_SIGNALED_BIT);
fence->temp_syncobj = 0;
@@ -3284,6 +3285,8 @@ void radv_DestroyFence(
device->ws->destroy_syncobj(device->ws, fence->syncobj);
if (fence->fence)
device->ws->destroy_fence(fence->fence);
+ if (fence->fence_wsi)
+ fence->fence_wsi->destroy(fence->fence_wsi);
vk_free2(&device->alloc, pAllocator, fence);
}
@@ -3309,7 +3312,19 @@ static bool radv_all_fences_plain_and_submitted(uint32_t fenceCount, const VkFen
{
for (uint32_t i = 0; i < fenceCount; ++i) {
RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
- if (fence->syncobj || fence->temp_syncobj || (!fence->signalled && !fence->submitted))
+ if (fence->fence == NULL || fence->syncobj ||
+ fence->temp_syncobj ||
+ (!fence->signalled && !fence->submitted))
+ return false;
+ }
+ return true;
+}
+
+static bool radv_all_fences_syncobj(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
+ if (fence->syncobj == 0 && fence->temp_syncobj == 0)
return false;
}
return true;
@@ -3325,7 +3340,9 @@ VkResult radv_WaitForFences(
RADV_FROM_HANDLE(radv_device, device, _device);
timeout = radv_get_absolute_timeout(timeout);
- if (device->always_use_syncobj) {
+ if (device->always_use_syncobj &&
+ radv_all_fences_syncobj(fenceCount, pFences))
+ {
uint32_t *handles = malloc(sizeof(uint32_t) * fenceCount);
if (!handles)
return vk_error(device->instance, VK_ERROR_OUT_OF_HOST_MEMORY);
@@ -3395,21 +3412,35 @@ VkResult radv_WaitForFences(
if (fence->signalled)
continue;
- if (!fence->submitted) {
- while(radv_get_current_time() <= timeout && !fence->submitted)
- /* Do nothing */;
+ if (fence->fence) {
+ if (!fence->submitted) {
+ while(radv_get_current_time() <= timeout &&
+ !fence->submitted)
+ /* Do nothing */;
- if (!fence->submitted)
- return VK_TIMEOUT;
+ if (!fence->submitted)
+ return VK_TIMEOUT;
- /* Recheck as it may have been set by submitting operations. */
- if (fence->signalled)
- continue;
+ /* Recheck as it may have been set by
+ * submitting operations. */
+
+ if (fence->signalled)
+ continue;
+ }
+
+ expired = device->ws->fence_wait(device->ws,
+ fence->fence,
+ true, timeout);
+ if (!expired)
+ return VK_TIMEOUT;
}
- expired = device->ws->fence_wait(device->ws, fence->fence, true, timeout);
- if (!expired)
- return VK_TIMEOUT;
+ if (fence->fence_wsi) {
+ expired = fence->fence_wsi->wait(fence->fence_wsi,
+ true, timeout);
+ if (!expired)
+ return VK_TIMEOUT;
+ }
fence->signalled = true;
}
@@ -3461,9 +3492,14 @@ VkResult radv_GetFenceStatus(VkDevice _device, VkFence _fence)
return VK_SUCCESS;
if (!fence->submitted)
return VK_NOT_READY;
- if (!device->ws->fence_wait(device->ws, fence->fence, false, 0))
- return VK_NOT_READY;
-
+ if (fence->fence) {
+ if (!device->ws->fence_wait(device->ws, fence->fence, false, 0))
+ return VK_NOT_READY;
+ }
+ if (fence->fence_wsi) {
+ if (!fence->fence_wsi->wait(fence->fence_wsi, false, 0))
+ return VK_NOT_READY;
+ }
return VK_SUCCESS;
}
diff --git a/src/amd/vulkan/radv_extensions.py b/src/amd/vulkan/radv_extensions.py
index 601a345b114..ebc3f6bc2b5 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -90,6 +90,7 @@ EXTENSIONS = [
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_display_surface_counter', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
+ Extension('VK_EXT_display_control', 1, 'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_debug_report', 9, True),
Extension('VK_EXT_depth_range_unrestricted', 1, True),
Extension('VK_EXT_descriptor_indexing', 2, True),
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index 3d71d3bdb03..1e881a61018 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -1778,6 +1778,7 @@ void radv_initialize_dcc(struct radv_cmd_buffer *cmd_buffer,
struct radv_fence {
struct radeon_winsys_fence *fence;
+ struct wsi_fence *fence_wsi;
bool submitted;
bool signalled;
diff --git a/src/amd/vulkan/radv_wsi_display.c b/src/amd/vulkan/radv_wsi_display.c
index 0f88ce13942..5c0460378af 100644
--- a/src/amd/vulkan/radv_wsi_display.c
+++ b/src/amd/vulkan/radv_wsi_display.c
@@ -188,3 +188,116 @@ radv_GetRandROutputDisplayEXT(VkPhysicalDevice physical_device,
display);
}
#endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
+
+/* VK_EXT_display_control */
+
+VkResult
+radv_DisplayPowerControlEXT(VkDevice _device,
+ VkDisplayKHR display,
+ const VkDisplayPowerInfoEXT *display_power_info)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+
+ return wsi_display_power_control(_device,
+ &device->physical_device->wsi_device,
+ display,
+ display_power_info);
+}
+
+VkResult
+radv_RegisterDeviceEventEXT(VkDevice _device,
+ const VkDeviceEventInfoEXT *device_event_info,
+ const VkAllocationCallbacks *allocator,
+ VkFence *_fence)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ const VkAllocationCallbacks *alloc;
+ struct radv_fence *fence;
+ VkResult ret;
+
+ if (allocator)
+ alloc = allocator;
+ else
+ alloc = &device->instance->alloc;
+
+ fence = vk_alloc(alloc, sizeof (*fence), 8,
+ VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
+ if (!fence)
+ return VK_ERROR_OUT_OF_HOST_MEMORY;
+
+ fence->fence = NULL;
+ fence->submitted = true;
+ fence->signalled = false;
+ fence->syncobj = 0;
+ fence->temp_syncobj = 0;
+
+ ret = wsi_register_device_event(_device,
+ &device->physical_device->wsi_device,
+ device_event_info,
+ alloc,
+ &fence->fence_wsi);
+ if (ret == VK_SUCCESS)
+ *_fence = radv_fence_to_handle(fence);
+ else
+ vk_free(alloc, fence);
+ return ret;
+}
+
+VkResult
+radv_RegisterDisplayEventEXT(VkDevice _device,
+ VkDisplayKHR display,
+ const VkDisplayEventInfoEXT *display_event_info,
+ const VkAllocationCallbacks *allocator,
+ VkFence *_fence)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+
+ const VkAllocationCallbacks *alloc;
+ struct radv_fence *fence;
+ VkResult ret;
+
+ if (allocator)
+ alloc = allocator;
+ else
+ alloc = &device->instance->alloc;
+
+ fence = vk_alloc(alloc, sizeof (*fence), 8,
+ VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
+ if (!fence)
+ return VK_ERROR_OUT_OF_HOST_MEMORY;
+
+ fence->fence = NULL;
+ fence->submitted = true;
+ fence->signalled = false;
+ fence->syncobj = 0;
+ fence->temp_syncobj = 0;
+
+ ret = wsi_register_display_event(_device,
+ &device->physical_device->wsi_device,
+ display,
+ display_event_info,
+ alloc,
+ &(fence->fence_wsi));
+
+ if (ret == VK_SUCCESS)
+ *_fence = radv_fence_to_handle(fence);
+ else
+ vk_free(alloc, fence);
+ return ret;
+}
+
+VkResult
+radv_GetSwapchainCounterEXT(VkDevice _device,
+ VkSwapchainKHR swapchain,
+ VkSurfaceCounterFlagBitsEXT flag_bits,
+ uint64_t *value)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+
+ return wsi_get_swapchain_counter(_device,
+ &device->physical_device->wsi_device,
+ swapchain,
+ flag_bits,
+ value);
+}
+
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4]
2018-06-15 2:52 ` [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4] Keith Packard
@ 2018-06-16 18:22 ` Jason Ekstrand
2018-06-16 18:55 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-16 18:22 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 2297 bytes --]
On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <keithp@keithp.com> wrote:
> This extension is required to support EXT_display_control as it offers
> a way to query whether the vblank counter is supported.
>
> v2: Thanks to kisak
>
> Fix spelling of VkSurfaceCapabilities2EXT in wsi_common_wayland.c,
> it was using ext instead of EXT.
>
> Fix spelling of VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT
>
> v3: Fix wayland WSI, regularize spelling
>
> Misspelled 'surface' in get_capabilities2ext
> Remove extra _ from get_capabilities_2ext in a couple of places
>
> v4: Adopt Jason Ekstrand's coding conventions
>
> Declare variables at first use, eliminate extra whitespace
> between types and names. Wrap lines to 80 columns.
>
> Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> src/vulkan/wsi/wsi_common.c | 12 ++++++++++++
> src/vulkan/wsi/wsi_common.h | 6 ++++++
> src/vulkan/wsi/wsi_common_display.c | 27 +++++++++++++++++++++++++++
> src/vulkan/wsi/wsi_common_private.h | 3 +++
> src/vulkan/wsi/wsi_common_wayland.c | 27 +++++++++++++++++++++++++++
> src/vulkan/wsi/wsi_common_x11.c | 27 +++++++++++++++++++++++++++
> 6 files changed, 102 insertions(+)
>
> diff --git a/src/vulkan/wsi/wsi_common_private.h
> b/src/vulkan/wsi/wsi_common_private.h
> index 3d502b9fc9d..b97d2d8ba06 100644
> --- a/src/vulkan/wsi/wsi_common_private.h
> +++ b/src/vulkan/wsi/wsi_common_private.h
> @@ -109,6 +109,9 @@ struct wsi_interface {
> VkResult (*get_capabilities2)(VkIcdSurfaceBase *surface,
> const void *info_next,
> VkSurfaceCapabilities2KHR*
> pSurfaceCapabilities);
> + VkResult (*get_capabilities2ext)(VkIcdSurfaceBase *surface,
> + VkSurfaceCapabilities2EXT*
> + pSurfaceCapabilities);
>
I really don't like adding a third get_capabilities hook. An alternative
way to do this would be to add a pseud-extension which allows you do get he
extra bit of data with a chain-in on vkGetSurfaceCapabilities2KHR. I sent
two patches which do just that. If you like them, I'm happy do do the
reabase for you if you'd like.
--Jason
[-- Attachment #1.2: Type: text/html, Size: 3061 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4]
2018-06-16 18:22 ` Jason Ekstrand
@ 2018-06-16 18:55 ` Keith Packard
0 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-16 18:55 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 633 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> I really don't like adding a third get_capabilities hook.
Yeah, but this new function takes a different struct parameter which has
a different (but not strict superset) of contents from either of the
existing functions. Annoying.
> An alternative way to do this would be to add a pseud-extension which
> allows you do get he extra bit of data with a chain-in on
> vkGetSurfaceCapabilities2KHR. I sent two patches which do just that.
> If you like them, I'm happy do do the reabase for you if you'd like.
I'll review that when I have some time Monday.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-15 2:52 ` [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2] Keith Packard
@ 2018-06-19 23:26 ` Jason Ekstrand
2018-06-20 0:36 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-19 23:26 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 7323 bytes --]
I still don't really like this but I agree that this code really should not
be getting hit very often so it's probably not too bad. Maybe one day
we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
nice. In the mean time, this is probably fine. I did see one issue below
with time conversions that should be fixed though.
On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <keithp@keithp.com> wrote:
> Handle the case where the set of fences to wait for is not all of the
> same type by either waiting for them sequentially (waitAll), or
> polling them until the timer has expired (!waitAll). We hope the
> latter case is not common.
>
> While the current code makes sure that it always has fences of only
> one type, that will not be true when we add WSI fences. Split out this
> refactoring to make merging that clearer.
>
> v2: Adopt Jason Ekstrand's coding conventions
>
> Declare variables at first use, eliminate extra whitespace between
> types and names. Wrap lines to 80 columns.
>
> Suggested-by: Jason Ekstrand <jason.ekstrand@intel.com>
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> src/intel/vulkan/anv_queue.c | 105 +++++++++++++++++++++++++++++------
> 1 file changed, 88 insertions(+), 17 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
> index 6fe04a0a19d..8df99c84549 100644
> --- a/src/intel/vulkan/anv_queue.c
> +++ b/src/intel/vulkan/anv_queue.c
> @@ -459,12 +459,32 @@ gettime_ns(void)
> return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;
> }
>
> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
> +{
> + if (timeout == 0)
> + return 0;
> + uint64_t current_time = gettime_ns();
> +
> + timeout = MIN2(INT64_MAX - current_time, timeout);
> +
> + return (current_time + timeout);
> +}
>
This does not have the same behavior as the code it replaces. In
particular, the old code saturates at INT64_MAX whereas this code does not
do so properly anymore. If UINT64_MAX gets passed into timeout, I believe
that will be implicitly casted to signed and the MIN will yield -1 which
gets casted back to unsigned and you get UINT64_MAX again. This is a
problem because the value we pass into the kernel ioctls is signed. The
behavior of the kernel *should* be infinite when timeout < 0 but, on some
older kernels, -1 is treated as 0 and you get no timeout.
That said, I think I just saw a bug in the old code which I'll point out
below.
> +
> +static int64_t anv_get_relative_timeout(uint64_t abs_timeout)
> +{
> + uint64_t now = gettime_ns();
> +
> + if (abs_timeout < now)
> + return 0;
> + return abs_timeout - now;
> +}
> +
> static VkResult
> anv_wait_for_syncobj_fences(struct anv_device *device,
> uint32_t fenceCount,
> const VkFence *pFences,
> bool waitAll,
> - uint64_t _timeout)
> + uint64_t abs_timeout_ns)
> {
> uint32_t *syncobjs = vk_zalloc(&device->alloc,
> sizeof(*syncobjs) * fenceCount, 8,
> @@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(struct anv_device
> *device,
> syncobjs[i] = impl->syncobj;
> }
>
> - int64_t abs_timeout_ns = 0;
> - if (_timeout > 0) {
> - uint64_t current_ns = gettime_ns();
> -
> - /* Add but saturate to INT32_MAX */
> - if (current_ns + _timeout < current_ns)
> - abs_timeout_ns = INT64_MAX;
> - else if (current_ns + _timeout > INT64_MAX)
>
I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't
accidentally get an implicit conversion to signed.
> - abs_timeout_ns = INT64_MAX;
> - else
> - abs_timeout_ns = current_ns + _timeout;
> - }
> -
> /* The gem_syncobj_wait ioctl may return early due to an inherent
> * limitation in the way it computes timeouts. Loop until we've
> actually
> * passed the timeout.
> @@ -665,6 +672,67 @@ done:
> return result;
> }
>
> +static VkResult
> +anv_wait_for_fences(struct anv_device *device,
> + uint32_t fenceCount,
> + const VkFence *pFences,
> + bool waitAll,
> + uint64_t abs_timeout)
> +{
> + VkResult result = VK_SUCCESS;
> +
> + if (fenceCount <= 1 || waitAll) {
> + for (uint32_t i = 0; i < fenceCount; i++) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + switch (fence->permanent.type) {
> + case ANV_FENCE_TYPE_BO:
> + result = anv_wait_for_bo_fences(
> + device, 1, &pFences[i], true,
> + anv_get_relative_timeout(abs_timeout));
> + break;
> + case ANV_FENCE_TYPE_SYNCOBJ:
> + result = anv_wait_for_syncobj_fences(device, 1, &pFences[i],
> + true, abs_timeout);
> + break;
> + case ANV_FENCE_TYPE_NONE:
> + result = VK_SUCCESS;
> + break;
> + }
> + if (result != VK_SUCCESS)
> + return result;
> + }
> + } else {
> + while (gettime_ns() < abs_timeout) {
> + for (uint32_t i = 0; i < fenceCount; i++) {
> + if (anv_wait_for_fences(device, 1, &pFences[i], true, 0) ==
> VK_SUCCESS)
> + return VK_SUCCESS;
> + }
> + }
> + result = VK_TIMEOUT;
> + }
> + return result;
> +}
> +
> +static bool anv_all_fences_syncobj(uint32_t fenceCount, const VkFence
> *pFences)
> +{
> + for (uint32_t i = 0; i < fenceCount; ++i) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + if (fence->permanent.type != ANV_FENCE_TYPE_SYNCOBJ)
> + return false;
> + }
> + return true;
> +}
> +
> +static bool anv_all_fences_bo(uint32_t fenceCount, const VkFence *pFences)
> +{
> + for (uint32_t i = 0; i < fenceCount; ++i) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + if (fence->permanent.type != ANV_FENCE_TYPE_BO)
> + return false;
> + }
> + return true;
> +}
> +
> VkResult anv_WaitForFences(
> VkDevice _device,
> uint32_t fenceCount,
> @@ -677,12 +745,15 @@ VkResult anv_WaitForFences(
> if (unlikely(device->lost))
> return VK_ERROR_DEVICE_LOST;
>
> - if (device->instance->physicalDevice.has_syncobj_wait) {
> + if (anv_all_fences_syncobj(fenceCount, pFences)) {
> return anv_wait_for_syncobj_fences(device, fenceCount, pFences,
> - waitAll, timeout);
> - } else {
> + waitAll,
> anv_get_absolute_timeout(timeout));
> + } else if (anv_all_fences_bo(fenceCount, pFences)) {
> return anv_wait_for_bo_fences(device, fenceCount, pFences,
> waitAll, timeout);
> + } else {
> + return anv_wait_for_fences(device, fenceCount, pFences,
> + waitAll, anv_get_absolute_timeout(
> timeout));
> }
> }
>
> --
> 2.17.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: 9606 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
2018-06-15 2:52 ` [PATCH 5/7] vulkan: add VK_EXT_display_control [v5] Keith Packard
@ 2018-06-20 0:28 ` Jason Ekstrand
2018-06-20 4:44 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 0:28 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 18859 bytes --]
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
>
[-- Attachment #1.2: Type: text/html, Size: 25013 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
2018-06-15 2:52 ` [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2] Keith Packard
@ 2018-06-20 0:33 ` Jason Ekstrand
2018-06-20 5:31 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 0:33 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 8225 bytes --]
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: 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>
> ---
> src/intel/vulkan/anv_extensions.py | 1 +
> src/intel/vulkan/anv_private.h | 4 ++
> src/intel/vulkan/anv_queue.c | 22 +++++++
> src/intel/vulkan/anv_wsi_display.c | 97 ++++++++++++++++++++++++++++++
> 4 files changed, 124 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_extensions.py b/src/intel/vulkan/anv_
> extensions.py
> index 68e545a40f8..8c010e9280b 100644
> --- a/src/intel/vulkan/anv_extensions.py
> +++ b/src/intel/vulkan/anv_extensions.py
> @@ -111,6 +111,7 @@ EXTENSIONS = [
> Extension('VK_EXT_acquire_xlib_display', 1,
> 'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
> Extension('VK_EXT_debug_report', 8, True),
> Extension('VK_EXT_direct_mode_display', 1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
> + Extension('VK_EXT_display_control', 1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
> Extension('VK_EXT_display_surface_counter', 1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),
> Extension('VK_EXT_external_memory_dma_buf', 1, True),
> Extension('VK_EXT_global_priority', 1,
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index fb91bc33046..c81885979ad 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2133,6 +2133,7 @@ enum anv_fence_type {
> ANV_FENCE_TYPE_NONE = 0,
> ANV_FENCE_TYPE_BO,
> ANV_FENCE_TYPE_SYNCOBJ,
> + ANV_FENCE_TYPE_WSI,
> };
>
> enum anv_bo_fence_state {
> @@ -2167,6 +2168,9 @@ struct anv_fence_impl {
>
> /** DRM syncobj handle for syncobj-based fences */
> uint32_t syncobj;
> +
> + /** WSI fence */
> + struct wsi_fence *fence_wsi;
> };
> };
>
> diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
> index 8df99c84549..073e65acf5e 100644
> --- a/src/intel/vulkan/anv_queue.c
> +++ b/src/intel/vulkan/anv_queue.c
> @@ -324,6 +324,10 @@ anv_fence_impl_cleanup(struct anv_device *device,
> anv_gem_syncobj_destroy(device, impl->syncobj);
> break;
>
> + case ANV_FENCE_TYPE_WSI:
> + impl->fence_wsi->destroy(impl->fence_wsi);
> + break;
> +
> default:
> unreachable("Invalid fence type");
> }
> @@ -672,6 +676,21 @@ done:
> return result;
> }
>
> +static VkResult
> +anv_wait_for_wsi_fence(struct anv_device *device,
> + const VkFence _fence,
> + uint64_t abs_timeout)
> +{
> + ANV_FROM_HANDLE(anv_fence, fence, _fence);
> +
> + struct anv_fence_impl *impl = &fence->permanent;
> + bool expired = impl->fence_wsi->wait(impl->fence_wsi, true,
> abs_timeout);
> +
> + if (!expired)
> + return VK_TIMEOUT;
> + return VK_SUCCESS;
> +}
> +
> static VkResult
> anv_wait_for_fences(struct anv_device *device,
> uint32_t fenceCount,
> @@ -694,6 +713,9 @@ anv_wait_for_fences(struct anv_device *device,
> result = anv_wait_for_syncobj_fences(device, 1, &pFences[i],
> true, abs_timeout);
> break;
> + case ANV_FENCE_TYPE_WSI:
> + result = anv_wait_for_wsi_fence(device, pFences[i],
> abs_timeout);
> + break;
> case ANV_FENCE_TYPE_NONE:
> result = VK_SUCCESS;
> break;
> diff --git a/src/intel/vulkan/anv_wsi_display.c
> b/src/intel/vulkan/anv_wsi_display.c
> index f749a8d98f7..cd736bcdd74 100644
> --- a/src/intel/vulkan/anv_wsi_display.c
> +++ b/src/intel/vulkan/anv_wsi_display.c
> @@ -168,3 +168,100 @@ anv_GetRandROutputDisplayEXT(VkPhysicalDevice
> physical_device,
> display);
> }
> #endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
> +
> +/* VK_EXT_display_control */
> +
> +VkResult
> +anv_DisplayPowerControlEXT(VkDevice _device,
> + VkDisplayKHR display,
> + const VkDisplayPowerInfoEXT
> *display_power_info)
> +{
> + ANV_FROM_HANDLE(anv_device, device, _device);
> +
> + return wsi_display_power_control(
> + _device, &device->instance->physicalDevice.wsi_device,
> + display, display_power_info);
> +}
> +
> +VkResult
> +anv_RegisterDeviceEventEXT(VkDevice _device,
> + const VkDeviceEventInfoEXT *device_event_info,
> + const VkAllocationCallbacks *allocator,
> + VkFence *_fence)
> +{
> + ANV_FROM_HANDLE(anv_device, device, _device);
> + const VkAllocationCallbacks *alloc;
> + struct anv_fence *fence;
> + VkResult ret;
> +
> + if (allocator)
> + alloc = allocator;
> + else
> + alloc = &device->instance->alloc;
>
This is what vk_alloc2 is for. :-)
> +
> + fence = vk_alloc(alloc, sizeof (*fence), 8,
> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> + if (!fence)
> + return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> +
> + fence->permanent.type = ANV_FENCE_TYPE_WSI;
> +
> + ret = wsi_register_device_event(_device,
> + &device->instance->
> physicalDevice.wsi_device,
> + device_event_info,
> + alloc,
> + &fence->permanent.fence_wsi);
> + if (ret == VK_SUCCESS)
> + *_fence = anv_fence_to_handle(fence);
> + else
> + vk_free(alloc, fence);
And vk_free2
> + return ret;
> +}
> +
> +VkResult
> +anv_RegisterDisplayEventEXT(VkDevice _device,
> + VkDisplayKHR display,
> + const VkDisplayEventInfoEXT
> *display_event_info,
> + const VkAllocationCallbacks *allocator,
> + VkFence *_fence)
> +{
> + ANV_FROM_HANDLE(anv_device, device, _device);
> + const VkAllocationCallbacks *alloc;
> + struct anv_fence *fence;
> + VkResult ret;
> +
> + if (allocator)
> + alloc = allocator;
> + else
> + alloc = &device->instance->alloc;
>
This isn't needed if you're using vk_alloc2
> +
> + fence = vk_zalloc2(&device->alloc, allocator, sizeof (*fence), 8,
> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
>
Above you used vk_alloc and here you're using vk_zalloc. Mind picking
one? I don't think zalloc is needed but it doesn't hurt so I don't care
which.
> + if (!fence)
> + return VK_ERROR_OUT_OF_HOST_MEMORY;
> +
> + fence->permanent.type = ANV_FENCE_TYPE_WSI;
> +
> + ret = wsi_register_display_event(
> + _device, &device->instance->physicalDevice.wsi_device,
> + display, display_event_info, alloc, &(fence->permanent.fence_wsi));
>
Indentation could be consistent between the two functions you add. I don't
care which style.
> +
> + if (ret == VK_SUCCESS)
> + *_fence = anv_fence_to_handle(fence);
> + else
> + vk_free(alloc, fence);
>
vk_free2?
> + return ret;
> +}
> +
> +VkResult
> +anv_GetSwapchainCounterEXT(VkDevice _device,
> + VkSwapchainKHR swapchain,
> + VkSurfaceCounterFlagBitsEXT flag_bits,
> + uint64_t *value)
> +{
> + ANV_FROM_HANDLE(anv_device, device, _device);
> +
> + return wsi_get_swapchain_counter(
> + _device, &device->instance->physicalDevice.wsi_device,
> + swapchain, flag_bits, value);
> +}
> --
> 2.17.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: 11551 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-19 23:26 ` Jason Ekstrand
@ 2018-06-20 0:36 ` Keith Packard
2018-06-20 0:42 ` Jason Ekstrand
0 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-20 0:36 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 3195 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> I still don't really like this but I agree that this code really should not
> be getting hit very often so it's probably not too bad. Maybe one day
> we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
> nice.
I agree. What I want to have is kernel-side syncobj support for all of
the WSI fences that we need here.
I thought about using syncobjs and signaling them from user-space, but
realized that we couldn't eliminate the non-syncobj path quite yet as
that requires a new enough kernel. And, if we want to get to
kernel-native WSI syncobjs, that would mean we'd eventually have three
code paths. I think it's better to have one 'legacy' path, which works
on all kernels, and then one 'modern' path which does what we want.
> In the mean time, this is probably fine. I did see one issue below
> with time conversions that should be fixed though.
Always a hard thing to get right.
>> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
>> +{
>> + if (timeout == 0)
>> + return 0;
>> + uint64_t current_time = gettime_ns();
>> +
>> + timeout = MIN2(INT64_MAX - current_time, timeout);
>> +
>> + return (current_time + timeout);
>> +}
>>
>
> This does not have the same behavior as the code it replaces. In
> particular, the old code saturates at INT64_MAX whereas this code does not
> do so properly anymore. If UINT64_MAX gets passed into timeout, I believe
> that will be implicitly casted to signed and the MIN will yield -1 which
> gets casted back to unsigned and you get UINT64_MAX again.
It won't not get implicitly casted to signed; the implicit cast works
the other way where <signed> OP <unsigned> implicitly casts the <signed>
operand to unsigned for types of the same 'rank' (where 'rank' is not
quite equivalent to size). Here's a fine article on this:
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules
However, this is a rather obscure part of the ISO standard, and I don't
think we should expect that people reading the code know it well. Adding
a cast to make it clear what we want is a good idea. How about this?
static uint64_t anv_get_absolute_timeout(uint64_t timeout)
{
if (timeout == 0)
return 0;
uint64_t current_time = gettime_ns();
uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;
timeout = MIN2(max_timeout, timeout);
return (current_time + timeout);
}
> This is a problem because the value we pass into the kernel ioctls is
> signed. The behavior of the kernel *should* be infinite when timeout
> < 0 but, on some older kernels, -1 is treated as 0 and you get no
> timeout.
Yeah, we definitely want to cap the values to INT64_MAX.
>> - else if (current_ns + _timeout > INT64_MAX)
>>
>
> I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't
> accidentally get an implicit conversion to signed.
Again, it's not necessary given the C conversion rules, but it is a good
way to clarify the intent.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 0:36 ` Keith Packard
@ 2018-06-20 0:42 ` Jason Ekstrand
2018-06-20 1:22 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 0:42 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 2979 bytes --]
On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard <keithp@keithp.com> wrote:
> Jason Ekstrand <jason@jlekstrand.net> writes:
>
> > I still don't really like this but I agree that this code really should
> not
> > be getting hit very often so it's probably not too bad. Maybe one day
> > we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
> > nice.
>
> I agree. What I want to have is kernel-side syncobj support for all of
> the WSI fences that we need here.
>
> I thought about using syncobjs and signaling them from user-space, but
> realized that we couldn't eliminate the non-syncobj path quite yet as
> that requires a new enough kernel. And, if we want to get to
> kernel-native WSI syncobjs, that would mean we'd eventually have three
> code paths. I think it's better to have one 'legacy' path, which works
> on all kernels, and then one 'modern' path which does what we want.
>
> > In the mean time, this is probably fine. I did see one issue below
> > with time conversions that should be fixed though.
>
> Always a hard thing to get right.
>
> >> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
> >> +{
> >> + if (timeout == 0)
> >> + return 0;
> >> + uint64_t current_time = gettime_ns();
> >> +
> >> + timeout = MIN2(INT64_MAX - current_time, timeout);
> >> +
> >> + return (current_time + timeout);
> >> +}
> >>
> >
> > This does not have the same behavior as the code it replaces. In
> > particular, the old code saturates at INT64_MAX whereas this code does
> not
> > do so properly anymore. If UINT64_MAX gets passed into timeout, I
> believe
> > that will be implicitly casted to signed and the MIN will yield -1 which
> > gets casted back to unsigned and you get UINT64_MAX again.
>
> It won't not get implicitly casted to signed; the implicit cast works
> the other way where <signed> OP <unsigned> implicitly casts the <signed>
> operand to unsigned for types of the same 'rank' (where 'rank' is not
> quite equivalent to size). Here's a fine article on this:
>
> https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+
> Understand+integer+conversion+rules
>
> However, this is a rather obscure part of the ISO standard, and I don't
> think we should expect that people reading the code know it well.
This is why I insert casts like mad in these scenarios. :-)
> Adding
> a cast to make it clear what we want is a good idea. How about this?
>
> static uint64_t anv_get_absolute_timeout(uint64_t timeout)
> {
> if (timeout == 0)
> return 0;
> uint64_t current_time = gettime_ns();
> uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;
>
I suppose we probably shouldn't worry about current_time being greater than
INT64_MAX? I guess if that happens we have pretty big problems...
> timeout = MIN2(max_timeout, timeout);
>
> return (current_time + timeout);
> }
>
Yeah, I think that's better.
--Jason
[-- Attachment #1.2: Type: text/html, Size: 4398 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 0:42 ` Jason Ekstrand
@ 2018-06-20 1:22 ` Keith Packard
2018-06-20 2:22 ` Jason Ekstrand
0 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-20 1:22 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> I suppose we probably shouldn't worry about current_time being greater than
> INT64_MAX? I guess if that happens we have pretty big problems...
Nope, we've already given up on that working -- it's a couple hundred
years of system uptime. Neither of us have any concerns in this area.
>> timeout = MIN2(max_timeout, timeout);
>>
>> return (current_time + timeout);
>> }
>>
>
> Yeah, I think that's better.
Cool. I'll wait for further review :-)
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 1:22 ` Keith Packard
@ 2018-06-20 2:22 ` Jason Ekstrand
2018-06-20 5:16 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 2:22 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 731 bytes --]
On Tue, Jun 19, 2018 at 6:22 PM, Keith Packard <keithp@keithp.com> wrote:
> Jason Ekstrand <jason@jlekstrand.net> writes:
>
> > I suppose we probably shouldn't worry about current_time being greater
> than
> > INT64_MAX? I guess if that happens we have pretty big problems...
>
> Nope, we've already given up on that working -- it's a couple hundred
> years of system uptime. Neither of us have any concerns in this area.
>
> >> timeout = MIN2(max_timeout, timeout);
> >>
> >> return (current_time + timeout);
> >> }
> >>
> >
> > Yeah, I think that's better.
>
> Cool. I'll wait for further review :-)
>
I don't think I have any more comments on this patch. It's gross but I
think it should work.
[-- Attachment #1.2: Type: text/html, Size: 1332 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
2018-06-20 0:28 ` Jason Ekstrand
@ 2018-06-20 4:44 ` Keith Packard
2018-06-20 21:52 ` Jason Ekstrand
0 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-20 4:44 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 5781 bytes --]
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.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 2:22 ` Jason Ekstrand
@ 2018-06-20 5:16 ` Keith Packard
2018-06-20 5:22 ` Jason Ekstrand
0 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-20 5:16 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 324 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> I don't think I have any more comments on this patch. It's gross but I
> think it should work.
I'll mark you down as 'Acked-by' then. Neither of us loves the
implementation; I'll see about creating the kernel infrastructure
necessary to supplant it.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 5:16 ` Keith Packard
@ 2018-06-20 5:22 ` Jason Ekstrand
2018-06-20 5:36 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 5:22 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
On June 19, 2018 22:16:37 "Keith Packard" <keithp@keithp.com> wrote:
> Jason Ekstrand <jason@jlekstrand.net> writes:
>
>> I don't think I have any more comments on this patch. It's gross but I
>> think it should work.
>
> I'll mark you down as 'Acked-by' then. Neither of us loves the
> implementation; I'll see about creating the kernel infrastructure
> necessary to supplant it.
You can have a full reviewed-by
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
2018-06-20 0:33 ` Jason Ekstrand
@ 2018-06-20 5:31 ` Keith Packard
2018-06-20 17:13 ` Jason Ekstrand
0 siblings, 1 reply; 26+ messages in thread
From: Keith Packard @ 2018-06-20 5:31 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
>> + if (allocator)
>> + alloc = allocator;
>> + else
>> + alloc = &device->instance->alloc;
>>
>
> This is what vk_alloc2 is for. :-)
...
> And vk_free2
...
> This isn't needed if you're using vk_alloc2
Yeah, but I need to pass the allocator down to the wsi common code, and
that doesn't have any way to touch the device driver allocator
pointer. I bet I'm just missing something here. Help?
>> +
>> + fence = vk_zalloc2(&device->alloc, allocator, sizeof (*fence), 8,
>> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
>>
>
> Above you used vk_alloc and here you're using vk_zalloc. Mind picking
> one? I don't think zalloc is needed but it doesn't hurt so I don't care
> which.
Thanks. Existing code is using zalloc for fences; I'll use that everywhere.
> Indentation could be consistent between the two functions you add. I don't
> care which style.
Sure; these function names are kinda long. I've wrapped the first call
after the (
> vk_free2?
I've had to compute 'alloc' to pass into wsi_common; I figured I might
as well use it.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
2018-06-20 5:22 ` Jason Ekstrand
@ 2018-06-20 5:36 ` Keith Packard
0 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-20 5:36 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 122 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> You can have a full reviewed-by
You're too kind :-)
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
2018-06-20 5:31 ` Keith Packard
@ 2018-06-20 17:13 ` Jason Ekstrand
2018-06-20 22:45 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 17:13 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 822 bytes --]
On Tue, Jun 19, 2018 at 10:31 PM, Keith Packard <keithp@keithp.com> wrote:
> Jason Ekstrand <jason@jlekstrand.net> writes:
>
> >> + if (allocator)
> >> + alloc = allocator;
> >> + else
> >> + alloc = &device->instance->alloc;
> >>
> >
> > This is what vk_alloc2 is for. :-)
> ...
> > And vk_free2
> ...
> > This isn't needed if you're using vk_alloc2
>
> Yeah, but I need to pass the allocator down to the wsi common code, and
> that doesn't have any way to touch the device driver allocator
> pointer. I bet I'm just missing something here. Help?
>
I believe that the WSI common code should be capable of fishing the
instance allocator out of the wsi_display so we need only pass the
allocator argument unmodified through to the core WSI code. Make sense?
Yeah, Vulkan allocator fishing is weird.
--Jason
[-- Attachment #1.2: Type: text/html, Size: 1408 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
2018-06-20 4:44 ` Keith Packard
@ 2018-06-20 21:52 ` Jason Ekstrand
2018-06-20 22:49 ` Keith Packard
0 siblings, 1 reply; 26+ messages in thread
From: Jason Ekstrand @ 2018-06-20 21:52 UTC (permalink / raw)
To: Keith Packard; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 7150 bytes --]
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
[-- Attachment #1.2: Type: text/html, Size: 10154 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
2018-06-20 17:13 ` Jason Ekstrand
@ 2018-06-20 22:45 ` Keith Packard
0 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-20 22:45 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 629 bytes --]
Jason Ekstrand <jason@jlekstrand.net> writes:
> I believe that the WSI common code should be capable of fishing the
> instance allocator out of the wsi_display so we need only pass the
> allocator argument unmodified through to the core WSI code. Make sense?
Thanks, I think I've sorted it out. I've pushed an updated series with
this change.
> Yeah, Vulkan allocator fishing is weird.
Allowing custom allocators is one of the bad parts of the Vulkan spec;
it will "never" get used, and the chances of it working correctly in any
driver are pretty small. But, we do what we can to implement it.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
2018-06-20 21:52 ` Jason Ekstrand
@ 2018-06-20 22:49 ` Keith Packard
0 siblings, 0 replies; 26+ messages in thread
From: Keith Packard @ 2018-06-20 22:49 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: ML mesa-dev, Maling list - DRI developers
[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]
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.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-06-20 22:49 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15 2:52 [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter Keith Packard
2018-06-15 2:52 ` [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4] Keith Packard
2018-06-16 18:22 ` Jason Ekstrand
2018-06-16 18:55 ` Keith Packard
2018-06-15 2:52 ` [PATCH 2/7] anv: Add VK_EXT_display_surface_counter to anv driver [v2] Keith Packard
2018-06-15 2:52 ` [PATCH 3/7] radv: Add VK_EXT_display_surface_counter to radv driver Keith Packard
2018-06-15 2:52 ` [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2] Keith Packard
2018-06-19 23:26 ` Jason Ekstrand
2018-06-20 0:36 ` Keith Packard
2018-06-20 0:42 ` Jason Ekstrand
2018-06-20 1:22 ` Keith Packard
2018-06-20 2:22 ` Jason Ekstrand
2018-06-20 5:16 ` Keith Packard
2018-06-20 5:22 ` Jason Ekstrand
2018-06-20 5:36 ` Keith Packard
2018-06-15 2:52 ` [PATCH 5/7] vulkan: add VK_EXT_display_control [v5] Keith Packard
2018-06-20 0:28 ` Jason Ekstrand
2018-06-20 4:44 ` Keith Packard
2018-06-20 21:52 ` Jason Ekstrand
2018-06-20 22:49 ` Keith Packard
2018-06-15 2:52 ` [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2] Keith Packard
2018-06-20 0:33 ` Jason Ekstrand
2018-06-20 5:31 ` Keith Packard
2018-06-20 17:13 ` Jason Ekstrand
2018-06-20 22:45 ` Keith Packard
2018-06-15 2:52 ` [PATCH 7/7] radv: add VK_EXT_display_control to radv driver [v3] Keith Packard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.