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.
On Wed, Feb 21, 2018 at 11:32 AM, Jason Ekstrand <ja...@jlekstrand.net>
wrote:
> From: Daniel Stone <dani...@collabora.com>
>
> Use a helper function for updating the swapchain status. This will be
> used later to handle VK_SUBOPTIMAL_KHR, where we need to make a
> non-error status stick to the swapchain until recreation. Instead of
> direct comparisons to VK_SUCCESS to check for error, test for negative
> numbers meaning an error status, and positive numbers indicating
> non-error statuses.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> src/vulkan/wsi/wsi_common_x11.c | 104 ++++++++++++++++++++++++++++++
> ----------
> 1 file changed, 79 insertions(+), 25 deletions(-)
>
> diff --git a/src/vulkan/wsi/wsi_common_x11.c
> b/src/vulkan/wsi/wsi_common_x11.c
> index 2cc7a67..e845728 100644
> --- a/src/vulkan/wsi/wsi_common_x11.c
> +++ b/src/vulkan/wsi/wsi_common_x11.c
> @@ -641,6 +641,36 @@ struct x11_swapchain {
> struct x11_image images[0];
> };
>
> +/**
> + * Update the swapchain status with the result of an operation, and return
> + * the combined status. The chain status will eventually be returned from
> + * AcquireNextImage and QueuePresent.
> + *
> + * We make sure to 'stick' more pessimistic statuses: an out-of-date error
> + * is permanent once seen, and every subsequent call will return this. If
> + * this has not been seen, success will be returned.
> + */
> +static VkResult
> +x11_swapchain_result(struct x11_swapchain *chain, VkResult result)
> +{
> + /* Prioritise returning existing errors for consistency. */
> + if (chain->status < 0)
> + return chain->status;
> +
> + /* If we have a new error, mark it as permanent on the chain and
> return. */
> + if (result < 0) {
> + chain->status = result;
> + return result;
> + }
> +
> + /* Return temporary errors, but don't persist them. */
> + if (result == VK_TIMEOUT || result == VK_NOT_READY)
> + return result;
> +
> + /* No changes, so return the last status. */
> + return chain->status;
> +}
> +
> static struct wsi_image *
> x11_get_wsi_image(struct wsi_swapchain *wsi_chain, uint32_t image_index)
> {
> @@ -648,6 +678,9 @@ x11_get_wsi_image(struct wsi_swapchain *wsi_chain,
> uint32_t image_index)
> return &chain->images[image_index].base;
> }
>
> +/**
> + * Process an X11 Present event. Does not update chain->status.
> + */
> static VkResult
> x11_handle_dri3_present_event(struct x11_swapchain *chain,
> xcb_present_generic_event_t *event)
> @@ -719,6 +752,8 @@ x11_acquire_next_image_poll_x11(struct x11_swapchain
> *chain,
> xcb_generic_event_t *event;
> struct pollfd pfds;
> uint64_t atimeout;
> + VkResult result = VK_SUCCESS;
> +
> while (1) {
> for (uint32_t i = 0; i < chain->base.image_count; i++) {
> if (!chain->images[i].busy) {
> @@ -726,7 +761,8 @@ x11_acquire_next_image_poll_x11(struct x11_swapchain
> *chain,
> xshmfence_await(chain->images[i].shm_fence);
> *image_index = i;
> chain->images[i].busy = true;
> - return VK_SUCCESS;
> + result = VK_SUCCESS;
>
Since our neat little helper returns the accumulated result, these can all
be
- return VK_WHATEVER;
+ return x11_swapchain_result(chain, VK_WHATEVER);
That makes things quite a bit cleaner.
> + goto out;
> }
> }
>
> @@ -734,24 +770,31 @@ x11_acquire_next_image_poll_x11(struct
> x11_swapchain *chain,
>
> if (timeout == UINT64_MAX) {
> event = xcb_wait_for_special_event(chain->conn,
> chain->special_event);
> - if (!event)
> - return VK_ERROR_OUT_OF_DATE_KHR;
> + if (!event) {
> + result = VK_ERROR_OUT_OF_DATE_KHR;
> + goto out;
> + }
> } else {
> event = xcb_poll_for_special_event(chain->conn,
> chain->special_event);
> if (!event) {
> int ret;
> - if (timeout == 0)
> - return VK_NOT_READY;
> + if (timeout == 0) {
> + result = VK_NOT_READY;
> + goto out;
> + }
>
> atimeout = wsi_get_absolute_timeout(timeout);
>
> pfds.fd = xcb_get_file_descriptor(chain->conn);
> pfds.events = POLLIN;
> ret = poll(&pfds, 1, timeout / 1000 / 1000);
> - if (ret == 0)
> - return VK_TIMEOUT;
> - if (ret == -1)
> - return VK_ERROR_OUT_OF_DATE_KHR;
> + if (ret == 0) {
> + result = VK_TIMEOUT;
> + goto out;
> + } else if (ret == -1) {
> + result = VK_ERROR_OUT_OF_DATE_KHR;
> + goto out;
> + }
>
> /* If a non-special event happens, the fd will still
> * poll. So recalculate the timeout now just in case.
> @@ -765,11 +808,18 @@ x11_acquire_next_image_poll_x11(struct
> x11_swapchain *chain,
> }
> }
>
> + /* Update the swapchain status here. We may catch non-fatal errors
> here,
> + * in which case we need to update the status and continue.
> + */
> VkResult result = x11_handle_dri3_present_event(chain, (void
> *)event);
>
This shadows the function-scope result variable. I don't think that's what
you intended.
> + result = x11_swapchain_result(chain, result);
> free(event);
> - if (result != VK_SUCCESS)
> - return result;
> + if (result < 0)
> + goto out;
> }
> +
> +out:
> + return x11_swapchain_result(chain, result);
> }
>
> static VkResult
> @@ -781,11 +831,9 @@ x11_acquire_next_image_from_queue(struct
> x11_swapchain *chain,
> uint32_t image_index;
> VkResult result = wsi_queue_pull(&chain->acquire_queue,
> &image_index, timeout);
> - if (result != VK_SUCCESS) {
> - return result;
> - } else if (chain->status != VK_SUCCESS) {
> - return chain->status;
> - }
> + /* On error, the thread has shut down, so safe to update chain->status
> */
> + if (result < 0)
> + return x11_swapchain_result(chain, result);
>
I think we also want
if (result == VK_TIMEOUT)
return result;
>
> assert(image_index < chain->base.image_count);
> xshmfence_await(chain->images[image_index].shm_fence);
> @@ -859,13 +907,16 @@ x11_queue_present(struct wsi_swapchain *anv_chain,
> const VkPresentRegionKHR *damage)
> {
> struct x11_swapchain *chain = (struct x11_swapchain *)anv_chain;
> + VkResult result;
>
> if (chain->threaded) {
> wsi_queue_push(&chain->present_queue, image_index);
> - return chain->status;
> + result = chain->status;
> } else {
> - return x11_present_to_x11(chain, image_index, 0);
> + result = x11_present_to_x11(chain, image_index, 0);
>
Fun fact: x11_present_to_x11 always returns VK_SUCCESS. I don't think this
function needs to be modified at all.
> }
> +
> + return x11_swapchain_result(chain, result);
> }
>
> static void *
> @@ -876,7 +927,7 @@ x11_manage_fifo_queues(void *state)
>
> assert(chain->base.present_mode == VK_PRESENT_MODE_FIFO_KHR);
>
> - while (chain->status == VK_SUCCESS) {
> + while (chain->status >= 0) {
> /* It should be safe to unconditionally block here. Later in the
> loop
> * we blocks until the previous present has landed on-screen. At
> that
> * point, we should have received IDLE_NOTIFY on all images
> presented
> @@ -885,15 +936,18 @@ x11_manage_fifo_queues(void *state)
> */
> uint32_t image_index;
> result = wsi_queue_pull(&chain->present_queue, &image_index,
> INT64_MAX);
> - if (result != VK_SUCCESS) {
> + if (result < 0) {
> goto fail;
> - } else if (chain->status != VK_SUCCESS) {
> + } else if (chain->status < 0) {
> + /* The status can change underneath us if the swapchain is
> destroyed
> + * from another thread.
> + */
> return NULL;
> }
>
> uint64_t target_msc = chain->last_present_msc + 1;
> result = x11_present_to_x11(chain, image_index, target_msc);
> - if (result != VK_SUCCESS)
> + if (result < 0)
> goto fail;
>
> while (chain->last_present_msc < target_msc) {
> @@ -904,13 +958,13 @@ x11_manage_fifo_queues(void *state)
>
> result = x11_handle_dri3_present_event(chain, (void *)event);
> free(event);
> - if (result != VK_SUCCESS)
> + if (result < 0)
> goto fail;
> }
> }
>
> fail:
> - chain->status = result;
> + result = x11_swapchain_result(chain, result);
>
The "result =" does nothing here. I'm fine with leaving it though as it's
also not hurting anything.
> wsi_queue_push(&chain->acquire_queue, UINT32_MAX);
>
> return NULL;
> @@ -932,7 +986,7 @@ x11_image_init(VkDevice device_h, struct x11_swapchain
> *chain,
> result = wsi_create_native_image(&chain->base, pCreateInfo,
> 0, NULL, NULL, &image->base);
> }
> - if (result != VK_SUCCESS)
> + if (result < 0)
> return result;
>
> image->pixmap = xcb_generate_id(chain->conn);
> --
> 2.5.0.400.gff86faf
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
