[Mesa-dev] [PATCH] vulkan: Add VK_GOOGLE_display_timing extension (x11+display, anv+radv) [v6]

[Michel Dänzer]

On 2018-11-15 7:06 p.m., Keith Packard wrote:
> This adds support for the VK_GOOGLE_display timing extension, which
> provides two things:
> 
>  1) Detailed information about when frames are displayed, including
>     slack time between GPU execution and display frame.
> 
>  2) Absolute time control over swapchain queue processing. This allows
>     the application to request frames be displayed at specific
>     absolute times, using the same timebase as that provided in vblank
>     events.
> 
> Support for this extension has been implemented for the x11 and
> display backends; adding support to other backends should be
> reasonable straightforward for one familiar with those systems and
> should not require any additional device-specific code.
> 
> v2:
> 	Adjust GOOGLE_display_timing earliest value.  The
> 	earliestPresentTime for an image cannot be before the previous
> 	image was displayed, or even a frame later (in FIFO mode).
> 
> 	Make GOOGLE_display_timing use render completed time.  Switch
> 	from VK_PIPELINE_TOP_OF_PIPE_BIT to
> 	VK_PIPELINE_STAGE_ALL_COMMANDS_BIT so that the time reported
> 	to applications as the end of rendering reflects the latest
> 	possible value to ensure that applications don't underestimate
> 	the amount of work done in the frame.
> 
> 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 at intel.com>
> 
> v4:
> 	Adapt to changes in MESA_query_timestamp extension
> 
> v5:
> 	Squash core bits and anv/radv wrappers into a single patch
> 
>         Suggested-by: Jason Ekstrand <jason.ekstrand at intel.com>
> 
> v6:
> 	Switch from MESA_query_timestamp to EXT_calibrated_timestamps
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> 
> [...]
> 
> @@ -979,9 +1187,49 @@ wsi_common_queue_present(const struct wsi_device *wsi,
>            */
>           struct wsi_image *image =
>              swapchain->get_wsi_image(swapchain, pPresentInfo->pImageIndices[i]);
> -         submit_info.commandBufferCount = 1;
> -         submit_info.pCommandBuffers =
> -            &image->prime.blit_cmd_buffers[queue_family_index];
> +         submit_buffers[submit_info.commandBufferCount++] = 
> +            image->prime.blit_cmd_buffers[queue_family_index];
> +      }
> +
> +      /* Set up GOOGLE_display_timing bits */
> +      if (present_times_info &&
> +          present_times_info->pTimes != NULL &&
> +          i < present_times_info->swapchainCount)
> +      {
> +         const VkPresentTimeGOOGLE *present_time =
> +            &present_times_info->pTimes[i];
> +
> +         struct wsi_image *image =
> +            swapchain->get_wsi_image(swapchain, pPresentInfo->pImageIndices[i]);
> +
> +         timing = wsi_next_timing(swapchain, pPresentInfo->pImageIndices[i]);
> +         timing->timing.presentID = present_time->presentID;
> +         timing->timing.desiredPresentTime = present_time->desiredPresentTime;
> +         timing->target_msc = 0;
> +         image->timing = timing;
> +
> +         if (present_time->desiredPresentTime != 0)
> +         {
> +            int64_t delta_nsec = (int64_t) (present_time->desiredPresentTime -
> +                                            swapchain->frame_ust);
> +
> +            /* Set the target msc only if it's no more than two seconds from
> +             * now, and not stale
> +             */
> +            if (0 <= delta_nsec && delta_nsec <= 2000000000ul) {
> +               VkRefreshCycleDurationGOOGLE refresh_timing;
> +
> +               swapchain->get_refresh_cycle_duration(swapchain,
> +                                                     &refresh_timing);
> +
> +               int64_t refresh = (int64_t) refresh_timing.refreshDuration;
> +               int64_t frames = (delta_nsec + refresh/2) / refresh;

desiredPresentTime has "no sooner than" semantics, so I think this should be

               int64_t frames = (delta_nsec + refresh-1) / refresh;


> +               timing->target_msc = swapchain->frame_msc + frames;
> +            }
> +         }

Note that MSC based timing won't work well with variable refresh rate.
In the long term, support for PresentOptionUST should be implemented and
used.


> @@ -1691,6 +1760,66 @@ wsi_display_queue_present(struct wsi_swapchain *drv_chain,
>  
>     pthread_mutex_lock(&wsi->wait_mutex);
>  
> +   if (image->base.timing && image->base.timing->target_msc != 0) {
> +      VkIcdSurfaceDisplay *surface = chain->surface;
> +      wsi_display_mode *display_mode =
> +         wsi_display_mode_from_handle(surface->displayMode);
> +      wsi_display_connector *connector = display_mode->connector;
> +
> +      wsi_display_debug("delta frame %ld\n",
> +                        image->base.timing->target_msc - connector->last_frame);
> +      if (image->base.timing->target_msc > connector->last_frame) {
> +         uint64_t frame_queued;
> +         VkDisplayKHR display = wsi_display_connector_to_handle(connector);
> +
> +         wsi_display_debug_code(uint64_t current_frame, current_nsec;
> +                                drmCrtcGetSequence(wsi->fd, connector->crtc_id,
> +                                                   &current_frame,
> +                                                   &current_nsec);
> +                                wsi_display_debug("from current: %ld\n",
> +                                                  image->base.timing->target_msc
> +                                                  - current_frame));
> +
> +         image->fence = wsi_display_fence_alloc(chain->base.device,
> +                                                chain->base.wsi,
> +                                                display, &chain->base.alloc);
> +
> +         if (!image->fence) {
> +            result = VK_ERROR_OUT_OF_HOST_MEMORY;
> +            goto bail_unlock;
> +         }
> +
> +         result = wsi_register_vblank_event(image->fence,
> +                                            chain->base.wsi,
> +                                            display,
> +                                            0,
> +                                            image->base.timing->target_msc - 1,
> +                                            &frame_queued);
> +
> +         if (result != VK_SUCCESS)
> +            goto bail_unlock;
> +
> +         /* Check and make sure we are queued for the right frame, otherwise
> +          * just go queue an image
> +          */
> +         if (frame_queued <= image->base.timing->target_msc - 1) {
> +            image->state = WSI_IMAGE_WAITING;
> +
> +            /*
> +             * Don't set the image member until we're going to wait for the
> +             * event to arrive before flipping to the image. That way, if the
> +             * register_vblank_event call happens to process the event, it
> +             * won't actually do anything
> +             */
> +            image->fence->image = image;
> +            wsi_display_start_wait_thread(wsi);
> +            result = VK_SUCCESS;
> +            goto bail_unlock;
> +         }
> +
> +      }
> +   }

What is this code for? At least with X11 Present, shouldn't it be
sufficient to simply pass the target MSC value to x11_present_to_x11?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer