[Mesa-dev] [PATCH mesa 01/21] vulkan: Add KHR_display extension using DRM [v4]
Jason Ekstrand
jason at jlekstrand.net
Mon Apr 9 22:45:39 UTC 2018
On Wed, Mar 7, 2018 at 11:24 PM, Keith Packard <keithp at keithp.com> wrote:
>
> +/*
> + * Implement vkGetPhysicalDeviceDisplayPropertiesKHR (VK_KHR_display)
> + */
> +VkResult
> +wsi_display_get_physical_device_display_properties(VkPhysicalDevice
> physical_device,
> + struct wsi_device
> *wsi_device,
> + uint32_t
> *property_count,
> +
> VkDisplayPropertiesKHR *properties)
> +{
> + struct wsi_display *wsi = (struct wsi_display *)
> wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
> + drmModeResPtr mode_res;
> +
> + if (wsi->fd < 0)
> + goto bail;
> +
> + mode_res = drmModeGetResources(wsi->fd);
> +
> + if (!mode_res)
> + goto bail;
>
If you move the VK_OUTARRAY_MAKE up higher, both of the "goto bail"s can be
"return vk_outarray_status(&conn)". Not a big deal though; what you have
is probably fine.
> +
> + VK_OUTARRAY_MAKE(conn, properties, property_count);
> +
> + /* Get current information */
> +
> + for (int c = 0; c < mode_res->count_connectors; c++) {
> + struct wsi_display_connector *connector =
> + wsi_display_get_connector(wsi_device, mode_res->connectors[c]);
> +
> + if (!connector) {
> + drmModeFreeResources(mode_res);
> + return VK_ERROR_OUT_OF_HOST_MEMORY;
> + }
> +
> + if (connector->connected) {
> + vk_outarray_append(&conn, prop) {
> + wsi_display_fill_in_display_properties(wsi_device,
> + connector,
> + prop);
> + }
> + }
> + }
> +
> + drmModeFreeResources(mode_res);
> +
> + return vk_outarray_status(&conn);
> +
> +bail:
> + *property_count = 0;
> + return VK_SUCCESS;
> +}
> +
> +/*
> + * Implement vkGetPhysicalDeviceDisplayPlanePropertiesKHR (VK_KHR_display
> + */
> +VkResult
> +wsi_display_get_physical_device_display_plane_properties(VkPhysicalDevice
> physical_device,
> + struct
> wsi_device *wsi_device,
> + uint32_t
> *property_count,
> +
> VkDisplayPlanePropertiesKHR *properties)
> +{
> + struct wsi_display *wsi = (struct wsi_display *)
> wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
> + struct wsi_display_connector *connector;
> +
> + VK_OUTARRAY_MAKE(conn, properties, property_count);
> +
> + int stack_index = 0;
> +
> + LIST_FOR_EACH_ENTRY(connector, &wsi->connectors, list) {
> + vk_outarray_append(&conn, prop) {
> + if (connector && connector->active) {
> + prop->currentDisplay = wsi_display_connector_to_handl
> e(connector);
> + prop->currentStackIndex = stack_index++;
>
In your branch, this is 0. Why the change?
> + } else {
> + prop->currentDisplay = NULL;
>
This should probably be VK_NULL_HANDLE
> + prop->currentStackIndex = 0;
> + }
> + }
> + }
> + return vk_outarray_status(&conn);
> +}
[...]
> +static const VkPresentModeKHR available_present_modes[] = {
> + VK_PRESENT_MODE_FIFO_KHR,
> +};
> +
> +static VkResult
> +wsi_display_surface_get_present_modes(VkIcdSurfaceBase *surface,
> + uint32_t
> *present_mode_count,
> + VkPresentModeKHR *present_modes)
> +{
> + VK_OUTARRAY_MAKE(conn, present_modes, present_mode_count);
> +
> + for (unsigned int c = 0; c < ARRAY_SIZE(available_present_modes);
> c++) {
> + vk_outarray_append(&conn, present) {
> + *present = available_present_modes[c];
> + }
> + }
>
I don't think the array is helpful here. We can just do a
vk_outarray_append with FIFO and call it good. If anything, it's probably
worse since other present modes will probably be dependent on things such
as whether or not the driver exposes ASYNC.
> +
> + return vk_outarray_status(&conn);
> +}
> +
> +static void
> +wsi_display_destroy_buffer(struct wsi_display *wsi,
> + uint32_t buffer)
> +{
> + (void) drmIoctl(wsi->fd, DRM_IOCTL_MODE_DESTROY_DUMB,
> + &((struct drm_mode_destroy_dumb) { .handle = buffer
> }));
> +}
>
[...]
> +static VkResult
> +wsi_display_acquire_next_image(struct wsi_swapchain *drv_chain,
> + uint64_t timeout,
> + VkSemaphore semaphore,
> + uint32_t *image_index)
> +{
> + struct wsi_display_swapchain *chain = (struct wsi_display_swapchain
> *)drv_chain;
> + struct wsi_display *wsi = chain->wsi;
> + int ret = 0;
> + VkResult result = VK_SUCCESS;
> +
> + /* Bail early if the swapchain is broken */
> + if (chain->status != VK_SUCCESS)
> + return chain->status;
> +
> + if (timeout != 0 && timeout != UINT64_MAX)
> + timeout += wsi_get_current_monotonic();
>
This may overflow if the provide a large value that is not UINT64_MAX. We
should do something such as
uint64_t current = wsi_get_current_monotonic();
if (timeout + current < timeout)
timeout = UINT64_MAX;
else
timeout += current;
> +
> + pthread_mutex_lock(&wsi->wait_mutex);
> + for (;;) {
> + for (uint32_t i = 0; i < chain->base.image_count; i++) {
> + if (chain->images[i].state == WSI_IMAGE_IDLE) {
> + *image_index = i;
> + wsi_display_debug("image %d available\n", i);
> + chain->images[i].state = WSI_IMAGE_DRAWING;
> + result = VK_SUCCESS;
> + goto done;
> + }
> + wsi_display_debug("image %d state %d\n", i,
> chain->images[i].state);
> + }
> +
> + if (ret == ETIMEDOUT) {
> + result = VK_TIMEOUT;
> + goto done;
> + }
> +
> + ret = wsi_display_wait_for_event(wsi, timeout);
> +
> + if (ret && ret != ETIMEDOUT) {
> + result = VK_ERROR_OUT_OF_DATE_KHR;
> + goto done;
> + }
> + }
> +done:
> + pthread_mutex_unlock(&wsi->wait_mutex);
> +
> + if (result != VK_SUCCESS)
> + return result;
> +
> + return chain->status;
> +}
>
[...]
> +/*
> + * Pick a suitable CRTC to drive this connector. Prefer a CRTC which is
> + * currently driving this connector and not any others. Settle for a CRTC
> + * which is currently idle.
> + */
> +static uint32_t
> +wsi_display_select_crtc(struct wsi_display_connector *connector,
> + drmModeResPtr mode_res,
> + drmModeConnectorPtr drm_connector)
> +{
> + struct wsi_display *wsi = connector->wsi;
> + int c;
> + uint32_t crtc_id;
> +
> + /* See what CRTC is currently driving this connector */
> + if (drm_connector->encoder_id) {
> + drmModeEncoderPtr encoder = drmModeGetEncoder(wsi->fd,
> drm_connector->encoder_id);
> + if (encoder) {
> + crtc_id = encoder->crtc_id;
> + drmModeFreeEncoder(encoder);
> + if (crtc_id) {
> + if (wsi_display_crtc_solo(wsi, mode_res, drm_connector,
> crtc_id))
>
This could be crtc_id && wsi_display_crtc_solo(...)
> + return crtc_id;
> + }
> + }
> + }
> + crtc_id = 0;
> + for (c = 0; crtc_id == 0 && c < mode_res->count_crtcs; c++) {
> + drmModeCrtcPtr crtc = drmModeGetCrtc(wsi->fd, mode_res->crtcs[c]);
> + if (crtc && crtc->buffer_id == 0)
> + crtc_id = crtc->crtc_id;
> + drmModeFreeCrtc(crtc);
> + }
> + return crtc_id;
> +}
>
[...]
+/*
> + * Check to see if the kernel has no flip queued and if there's an image
> + * waiting to be displayed.
> + */
> +static VkResult
> +_wsi_display_queue_next(struct wsi_swapchain *drv_chain)
> +{
> + struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> + struct wsi_display *wsi = chain->wsi;
> + uint32_t i;
> + struct wsi_display_image *image = NULL;
> + VkIcdSurfaceDisplay *surface = chain->surface;
> + wsi_display_mode *display_mode =
> wsi_display_mode_from_handle(surface->displayMode);
> + wsi_display_connector *connector = display_mode->connector;
> + int ret;
> + VkResult result;
> +
> + if (wsi->fd < 0)
> + return VK_ERROR_INITIALIZATION_FAILED;
>
Is this even possible? I don't think it is. If it isn't, maybe an assert
would be better? If it is, it should be VK_ERROR_OUT_OF_DATE_KHR.
> + if (display_mode != connector->current_mode)
> + connector->active = false;
> +
> + for (;;) {
> + /* Check to see if there is an image to display, or if some image
> is already queued */
> +
> + for (i = 0; i < chain->base.image_count; i++) {
> + struct wsi_display_image *tmp_image = &chain->images[i];
> +
> + switch (tmp_image->state) {
> + case WSI_IMAGE_FLIPPING:
> + /* already flipping, don't send another to the kernel yet */
> + return VK_SUCCESS;
> + case WSI_IMAGE_QUEUED:
> + /* find the oldest queued */
> + if (!image || tmp_image->flip_sequence < image->flip_sequence)
> + image = tmp_image;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (!image)
> + return VK_SUCCESS;
> +
> + if (connector->active) {
> + ret = drmModePageFlip(wsi->fd, connector->crtc_id, image->fb_id,
> + DRM_MODE_PAGE_FLIP_EVENT, image);
> + if (ret == 0) {
> + image->state = WSI_IMAGE_FLIPPING;
> + return VK_SUCCESS;
> + }
> + wsi_display_debug("page flip err %d %s\n", ret, strerror(-ret));
> + } else {
> + ret = -EINVAL;
> + }
> +
> + if (ret) {
> + switch(-ret) {
> + case EINVAL:
> +
> + result = wsi_display_setup_connector(connector,
> display_mode);
> +
> + if (result != VK_SUCCESS) {
> + image->state = WSI_IMAGE_IDLE;
>
Would QUEUED be more appropreate here? wsi_display_setup_connector will
have returned either OUT_OF_MEMORY or OUT_OF_DATE so it probably doesn't
matter. I guess that begs the question, why are we even bothering to mess
around with the image's state?
> + return result;
> + }
> +
> + /* XXX allow setting of position */
> +
> + ret = drmModeSetCrtc(wsi->fd, connector->crtc_id,
> image->fb_id, 0, 0,
> + &connector->id, 1,
> &connector->current_drm_mode);
> +
> + if (ret == 0) {
> + image->state = WSI_IMAGE_DISPLAYING;
> +
> + /* Assume that the mode set is synchronous and that any
> + * previous image is now idle.
> + */
> + wsi_display_idle_old_displaying(image);
> + connector->active = true;
> + return VK_SUCCESS;
> + }
>
If ret != 0, we continue on and go around the loop again and we try inside
the "if (connector->active)" block. It seems like we can get into an
infinite loop if drmModeSetCrtc keeps returning -EINVAL. At some point,
don't we want ot throw OUT_OF_DATE and bail?
> + break;
> + case EACCES:
> +
> + /* Some other VT is currently active. Sit here waiting for
> + * our VT to become active again by polling once a second
> + */
> + usleep(1000 * 1000);
> + connector->active = false;
> + break;
> + default:
> + connector->active = false;
> + image->state = WSI_IMAGE_IDLE;
>
Again, why are we setting it to IDLE instead of leaving it in QUEUED? This
doesn't seem to do anything useful.
> + return VK_ERROR_OUT_OF_DATE_KHR;
> + }
> + }
> + }
> +}
>
[...]
> +VkResult
> +wsi_display_init_wsi(struct wsi_device *wsi_device,
> + const VkAllocationCallbacks *alloc,
> + int display_fd)
> +{
> + struct wsi_display *wsi;
> + VkResult result;
> +
> + wsi = vk_zalloc(alloc, sizeof(*wsi), 8,
> + VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
> + if (!wsi) {
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;
> + goto fail;
> + }
> +
> + wsi->fd = display_fd;
> +
> + pthread_mutex_init(&wsi->wait_mutex, NULL);
> + wsi->alloc = alloc;
> +
> + LIST_INITHEAD(&wsi->connectors);
> +
> + pthread_condattr_t condattr;
> + int ret;
> +
> + ret = pthread_mutex_init(&wsi->wait_mutex, NULL);
>
Why are you initializing this twice?
> + if (ret) {
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;
> + goto fail_mutex;
> + }
> +
> + ret = pthread_condattr_init(&condattr);
> + if (ret) {
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;
> + goto fail_condattr;
> + }
> +
> + ret = pthread_condattr_setclock(&condattr, CLOCK_MONOTONIC);
> + if (ret) {
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;
> + goto fail_setclock;
> + }
> +
> + ret = pthread_cond_init(&wsi->wait_cond, &condattr);
> + if (ret) {
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;
> + goto fail_cond;
> + }
> +
> + pthread_condattr_destroy(&condattr);
>
Given that you destroy the condattr here, maybe it would make sense to have
a simple init_ptread_cond_monotonic helper to keep the clean-up path
simpler? If someone added any initialization after this that needed a
cleanup, it would get tricky with the condattr destroy.
> +
> + 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_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;
> + wsi->base.create_swapchain = wsi_display_surface_create_swapchain;
> +
> + wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY] = &wsi->base;
> +
> + return VK_SUCCESS;
> +
> +fail_cond:
> +fail_setclock:
> + pthread_condattr_destroy(&condattr);
> +fail_condattr:
> + pthread_mutex_destroy(&wsi->wait_mutex);
> +fail_mutex:
> + vk_free(alloc, wsi);
> +fail:
> + return result;
> +}
>
[...]
And, there's the end. :-) Nothing too drastic. A couple of bugs and a
suggestion or two. On to patch 2. :-)
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180409/46380c75/attachment-0001.html>
More information about the mesa-dev
mailing list