<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 21, 2018 at 11:53 AM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">For this one, I'm making live edits to see if I like the suggested changes. I'll send out the v2 with the comments incorporated when I'm done.<br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Wed, Feb 21, 2018 at 11:32 AM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>><br>
<br>
Use a helper function for updating the swapchain status. This will be<br>
used later to handle VK_SUBOPTIMAL_KHR, where we need to make a<br>
non-error status stick to the swapchain until recreation. Instead of<br>
direct comparisons to VK_SUCCESS to check for error, test for negative<br>
numbers meaning an error status, and positive numbers indicating<br>
non-error statuses.<br>
<br>
Signed-off-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>><br>
---<br>
src/vulkan/wsi/wsi_common_x11<wbr>.c | 104 ++++++++++++++++++++++++++++++<wbr>----------<br>
1 file changed, 79 insertions(+), 25 deletions(-)<br>
<br>
diff --git a/src/vulkan/wsi/wsi_common_x1<wbr>1.c b/src/vulkan/wsi/wsi_common_x1<wbr>1.c<br>
index 2cc7a67..e845728 100644<br>
--- a/src/vulkan/wsi/wsi_common_x1<wbr>1.c<br>
+++ b/src/vulkan/wsi/wsi_common_x1<wbr>1.c<br>
@@ -641,6 +641,36 @@ struct x11_swapchain {<br>
struct x11_image images[0];<br>
};<br>
<br>
+/**<br>
+ * Update the swapchain status with the result of an operation, and return<br>
+ * the combined status. The chain status will eventually be returned from<br>
+ * AcquireNextImage and QueuePresent.<br>
+ *<br>
+ * We make sure to 'stick' more pessimistic statuses: an out-of-date error<br>
+ * is permanent once seen, and every subsequent call will return this. If<br>
+ * this has not been seen, success will be returned.<br>
+ */<br>
+static VkResult<br>
+x11_swapchain_result(struct x11_swapchain *chain, VkResult result)<br>
+{<br>
+ /* Prioritise returning existing errors for consistency. */<br>
+ if (chain->status < 0)<br>
+ return chain->status;<br>
+<br>
+ /* If we have a new error, mark it as permanent on the chain and return. */<br>
+ if (result < 0) {<br>
+ chain->status = result;<br>
+ return result;<br>
+ }<br>
+<br>
+ /* Return temporary errors, but don't persist them. */<br>
+ if (result == VK_TIMEOUT || result == VK_NOT_READY)<br>
+ return result;<br>
+<br>
+ /* No changes, so return the last status. */<br>
+ return chain->status;<br>
+}<br>
+<br>
static struct wsi_image *<br>
x11_get_wsi_image(struct wsi_swapchain *wsi_chain, uint32_t image_index)<br>
{<br>
@@ -648,6 +678,9 @@ x11_get_wsi_image(struct wsi_swapchain *wsi_chain, uint32_t image_index)<br>
return &chain->images[image_index].ba<wbr>se;<br>
}<br>
<br>
+/**<br>
+ * Process an X11 Present event. Does not update chain->status.<br>
+ */<br>
static VkResult<br>
x11_handle_dri3_present_event<wbr>(struct x11_swapchain *chain,<br>
xcb_present_generic_event_t *event)<br>
@@ -719,6 +752,8 @@ x11_acquire_next_image_poll_x1<wbr>1(struct x11_swapchain *chain,<br>
xcb_generic_event_t *event;<br>
struct pollfd pfds;<br>
uint64_t atimeout;<br>
+ VkResult result = VK_SUCCESS;<br>
+<br>
while (1) {<br>
for (uint32_t i = 0; i < chain->base.image_count; i++) {<br>
if (!chain->images[i].busy) {<br>
@@ -726,7 +761,8 @@ x11_acquire_next_image_poll_x1<wbr>1(struct x11_swapchain *chain,<br>
xshmfence_await(chain->images<wbr>[i].shm_fence);<br>
*image_index = i;<br>
chain->images[i].busy = true;<br>
- return VK_SUCCESS;<br>
+ result = VK_SUCCESS;<br></blockquote><div><br></div></div></div><div>Since our neat little helper returns the accumulated result, these can all be<br><br></div><div>- return VK_WHATEVER;<br></div><div>+ return x11_swapchain_result(chain, VK_WHATEVER);<br><br></div><div>That makes things quite a bit cleaner.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ goto out;<br>
}<br>
}<br>
<br>
@@ -734,24 +770,31 @@ x11_acquire_next_image_poll_x1<wbr>1(struct x11_swapchain *chain,<br>
<br>
if (timeout == UINT64_MAX) {<br>
event = xcb_wait_for_special_event(cha<wbr>in->conn, chain->special_event);<br>
- if (!event)<br>
- return VK_ERROR_OUT_OF_DATE_KHR;<br>
+ if (!event) {<br>
+ result = VK_ERROR_OUT_OF_DATE_KHR;<br>
+ goto out;<br>
+ }<br>
} else {<br>
event = xcb_poll_for_special_event(cha<wbr>in->conn, chain->special_event);<br>
if (!event) {<br>
int ret;<br>
- if (timeout == 0)<br>
- return VK_NOT_READY;<br>
+ if (timeout == 0) {<br>
+ result = VK_NOT_READY;<br>
+ goto out;<br>
+ }<br>
<br>
atimeout = wsi_get_absolute_timeout(timeo<wbr>ut);<br>
<br>
pfds.fd = xcb_get_file_descriptor(chain-<wbr>>conn);<br>
pfds.events = POLLIN;<br>
ret = poll(&pfds, 1, timeout / 1000 / 1000);<br>
- if (ret == 0)<br>
- return VK_TIMEOUT;<br>
- if (ret == -1)<br>
- return VK_ERROR_OUT_OF_DATE_KHR;<br>
+ if (ret == 0) {<br>
+ result = VK_TIMEOUT;<br>
+ goto out;<br>
+ } else if (ret == -1) {<br>
+ result = VK_ERROR_OUT_OF_DATE_KHR;<br>
+ goto out;<br>
+ }<br>
<br>
/* If a non-special event happens, the fd will still<br>
* poll. So recalculate the timeout now just in case.<br>
@@ -765,11 +808,18 @@ x11_acquire_next_image_poll_x1<wbr>1(struct x11_swapchain *chain,<br>
}<br>
}<br>
<br>
+ /* Update the swapchain status here. We may catch non-fatal errors here,<br>
+ * in which case we need to update the status and continue.<br>
+ */<br>
VkResult result = x11_handle_dri3_present_event(<wbr>chain, (void *)event);<br></blockquote><div><br></div></div></div><div>This shadows the function-scope result variable. I don't think that's what you intended.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ result = x11_swapchain_result(chain, result);<br>
free(event);<br>
- if (result != VK_SUCCESS)<br>
- return result;<br>
+ if (result < 0)<br>
+ goto out;<br>
}<br>
+<br>
+out:<br>
+ return x11_swapchain_result(chain, result);<br>
}<br>
<br>
static VkResult<br>
@@ -781,11 +831,9 @@ x11_acquire_next_image_from_qu<wbr>eue(struct x11_swapchain *chain,<br>
uint32_t image_index;<br>
VkResult result = wsi_queue_pull(&chain->acquire<wbr>_queue,<br>
&image_index, timeout);<br>
- if (result != VK_SUCCESS) {<br>
- return result;<br>
- } else if (chain->status != VK_SUCCESS) {<br>
- return chain->status;<br>
- }<br>
+ /* On error, the thread has shut down, so safe to update chain->status */<br>
+ if (result < 0)<br>
+ return x11_swapchain_result(chain, result);<br></blockquote><div><br></div></span><div>I think we also want<br><br></div><div>if (result == VK_TIMEOUT)<br></div><div> return result;<br></div></div></div></div></blockquote><div><br></div><div>No, you were right in your original comment, "|| result == VK_TIMEOUT" is better.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
assert(image_index < chain->base.image_count);<br>
xshmfence_await(chain->images[<wbr>image_index].shm_fence);<br>
@@ -859,13 +907,16 @@ x11_queue_present(struct wsi_swapchain *anv_chain,<br>
const VkPresentRegionKHR *damage)<br>
{<br>
struct x11_swapchain *chain = (struct x11_swapchain *)anv_chain;<br>
+ VkResult result;<br>
<br>
if (chain->threaded) {<br>
wsi_queue_push(&chain->presen<wbr>t_queue, image_index);<br>
- return chain->status;<br>
+ result = chain->status;<br>
} else {<br>
- return x11_present_to_x11(chain, image_index, 0);<br>
+ result = x11_present_to_x11(chain, image_index, 0);<br></blockquote><div><br></div></span><div>Fun fact: x11_present_to_x11 always returns VK_SUCCESS. I don't think this function needs to be modified at all.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
+<br>
+ return x11_swapchain_result(chain, result);<br>
}<br>
<br>
static void *<br>
@@ -876,7 +927,7 @@ x11_manage_fifo_queues(void *state)<br>
<br>
assert(chain->base.present_mod<wbr>e == VK_PRESENT_MODE_FIFO_KHR);<br>
<br>
- while (chain->status == VK_SUCCESS) {<br>
+ while (chain->status >= 0) {<br>
/* It should be safe to unconditionally block here. Later in the loop<br>
* we blocks until the previous present has landed on-screen. At that<br>
* point, we should have received IDLE_NOTIFY on all images presented<br>
@@ -885,15 +936,18 @@ x11_manage_fifo_queues(void *state)<br>
*/<br>
uint32_t image_index;<br>
result = wsi_queue_pull(&chain->present<wbr>_queue, &image_index, INT64_MAX);<br>
- if (result != VK_SUCCESS) {<br>
+ if (result < 0) {<br>
goto fail;<br>
- } else if (chain->status != VK_SUCCESS) {<br>
+ } else if (chain->status < 0) {<br>
+ /* The status can change underneath us if the swapchain is destroyed<br>
+ * from another thread.<br>
+ */<br>
return NULL;<br>
}<br>
<br>
uint64_t target_msc = chain->last_present_msc + 1;<br>
result = x11_present_to_x11(chain, image_index, target_msc);<br>
- if (result != VK_SUCCESS)<br>
+ if (result < 0)<br>
goto fail;<br>
<br>
while (chain->last_present_msc < target_msc) {<br>
@@ -904,13 +958,13 @@ x11_manage_fifo_queues(void *state)<br>
<br>
result = x11_handle_dri3_present_event(<wbr>chain, (void *)event);<br>
free(event);<br>
- if (result != VK_SUCCESS)<br>
+ if (result < 0)<br>
goto fail;<br>
}<br>
}<br>
<br>
fail:<br>
- chain->status = result;<br>
+ result = x11_swapchain_result(chain, result);<br></blockquote><div><br></div></div></div><div>The "result =" does nothing here. I'm fine with leaving it though as it's also not hurting anything.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
wsi_queue_push(&chain->acquire<wbr>_queue, UINT32_MAX);<br>
<br>
return NULL;<br>
@@ -932,7 +986,7 @@ x11_image_init(VkDevice device_h, struct x11_swapchain *chain,<br>
result = wsi_create_native_image(&chain<wbr>->base, pCreateInfo,<br>
0, NULL, NULL, &image->base);<br>
}<br>
- if (result != VK_SUCCESS)<br>
+ if (result < 0)<br>
return result;<br>
<br>
image->pixmap = xcb_generate_id(chain->conn);<br>
<span class="m_-3001782454963191515m_-71200012016631171HOEnZb"><font color="#888888">--<br>
2.5.0.400.gff86faf<br>
<br>
</font></span></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>