On Wed, Feb 21, 2018 at 11:53 AM, Jason Ekstrand <ja...@jlekstrand.net>
wrote:
> 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;
>
No, you were right in your original comment, "|| result == VK_TIMEOUT" is
better.
>
>> 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
