[Mesa-dev] [PATCH mesa 7/9] vulkan: Add EXT_acquire_xlib_display [v3]
Jason Ekstrand
jason at jlekstrand.net
Wed Jun 13 22:43:36 UTC 2018
On Mon, Jun 11, 2018 at 10:39 PM, Keith Packard <keithp at keithp.com> wrote:
> This extension adds the ability to borrow an X RandR output for
> temporary use directly by a Vulkan application. For DRM, we use the
> Linux resource leasing mechanism.
>
> v2:
> Clean up xlib_lease detection
>
> * Use separate temporary '_xlib_lease' variable to hold the
> option value to avoid changin the type of a variable.
>
> * Use boolean expressions instead of additional if statements
> to compute resulting with_xlib_lease value.
>
> * Simplify addition of VK_USE_PLATFORM_XLIB_XRANDR_KHR to
> vulkan_wsi_args
>
> Suggested-by: Eric Engestrom <eric.engestrom at imgtec.com>
>
> Move mode list from wsi_display to wsi_display_connector
>
> Fix scope for wsi_display_mode and wsi_display_connector allocs
>
> Suggested-by: Jason Ekstrand <jason at jlekstrand.net>
>
> v3:
> Adopt Jason Ekstrand's coding conventions
>
> Declare variables at first use, eliminate extra whitespace
> between types and names. Wrap lines to 80 columns.
>
> Explicitly forbid multiple DRM leases. Making the code support
> this looks tricky and will require additional thought.
>
> Use xcb_randr_output_t throughout the internals of the
> implementation. Convert at the public API
> (wsi_get_randr_output_display).
>
> Clean up check for usable active_crtc (possible when only the
> desired output is connected to the crtc).
>
> Suggested-by: Jason Ekstrand <jason.ekstrand at intel.com>
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
>
> fixup for acquire
>
> fixup for RROutput type
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
>
> fixup
>
Lots of "fixup". Did you mean to actually comment on what that was?
> ---
> configure.ac | 32 ++
> meson.build | 11 +
> meson_options.txt | 7 +
> src/vulkan/Makefile.am | 5 +
> src/vulkan/wsi/meson.build | 5 +
> src/vulkan/wsi/wsi_common_display.c | 493 ++++++++++++++++++++++++++++
> src/vulkan/wsi/wsi_common_display.h | 17 +
> 7 files changed, 570 insertions(+)
>
[...]
> +static bool
> +wsi_display_mode_matches_x(struct wsi_display_mode *wsi,
> + xcb_randr_mode_info_t *xcb)
> +{
> + return wsi->clock == (xcb->dot_clock + 500) / 1000 &&
> + wsi->hdisplay == xcb->width &&
> + wsi->hsync_start == xcb->hsync_start &&
> + wsi->hsync_end == xcb->hsync_end &&
> + wsi->htotal == xcb->htotal &&
> + wsi->hskew == xcb->hskew &&
> + wsi->vdisplay == xcb->height &&
> + wsi->vsync_start == xcb->vsync_start &&
> + wsi->vsync_end == xcb->vsync_end &&
> + wsi->vtotal == xcb->vtotal &&
>
You're not checking vscan here.
> + wsi->flags == xcb->mode_flags;
> +}
>
[...]
> +static struct wsi_display_connector *
> +wsi_display_get_output(struct wsi_device *wsi_device,
> + xcb_connection_t *connection,
> + xcb_randr_output_t output)
> +{
> + struct wsi_display *wsi =
> + (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLA
> TFORM_DISPLAY];
> + struct wsi_display_connector *connector;
> + uint32_t connector_id;
> +
> + xcb_window_t root = wsi_display_output_to_root(connection, output);
> + if (!root)
> + return NULL;
> +
> + xcb_randr_get_screen_resources_cookie_t src =
> + xcb_randr_get_screen_resources(connection, root);
> + xcb_randr_get_output_info_cookie_t oic =
> + xcb_randr_get_output_info(connection, output, XCB_CURRENT_TIME);
> + xcb_randr_get_screen_resources_reply_t *srr =
> + xcb_randr_get_screen_resources_reply(connection, src, NULL);
> + xcb_randr_get_output_info_reply_t *oir =
> + xcb_randr_get_output_info_reply(connection, oic, NULL);
>
Why are you fetching these here and not lower down? The only uses of them
inside the "if (!connector)" is to free them. Seems to be a bit of a waste.
> +
> + /* See if we already have a connector for this output */
> + connector = wsi_display_find_output(wsi_device, output);
> +
> + if (!connector) {
> + xcb_atom_t connector_id_atom = 0;
> +
> + /*
> + * Go get the kernel connector ID for this X output
> + */
> + connector_id = wsi_display_output_to_connector_id(connection,
> +
> &connector_id_atom,
> + output);
> +
> + /* Any X server with lease support will have this atom */
> + if (!connector_id) {
> + free(oir);
> + free(srr);
> + return NULL;
> + }
> +
> + /* See if we already have a connector for this id */
> + connector = wsi_display_find_connector(wsi_device, connector_id);
> +
> + if (connector == NULL) {
> + connector = wsi_display_alloc_connector(wsi, connector_id);
> + if (!connector) {
> + free(oir);
> + free(srr);
> + return NULL;
> + }
> + list_addtail(&connector->list, &wsi->connectors);
> + }
> + connector->output = output;
> + }
> +
> + if (oir && srr) {
> + /* Get X modes and add them */
> +
> + connector->connected =
> + oir->connection != XCB_RANDR_CONNECTION_DISCONNECTED;
> +
> + wsi_display_invalidate_connector_modes(wsi_device, connector);
> +
> + xcb_randr_mode_t *x_modes = xcb_randr_get_output_info_modes(oir);
> + for (int m = 0; m < oir->num_modes; m++) {
> + xcb_randr_mode_info_iterator_t i =
> + xcb_randr_get_screen_resources_modes_iterator(srr);
> + while (i.rem) {
> + xcb_randr_mode_info_t *mi = i.data;
> + if (mi->id == x_modes[m]) {
> + VkResult result = wsi_display_register_x_mode(
> + wsi_device, connector, mi, m < oir->num_preferred);
> + if (result != VK_SUCCESS) {
> + free(oir);
> + free(srr);
> + return NULL;
> + }
> + break;
> + }
> + xcb_randr_mode_info_next(&i);
> + }
> + }
> + }
> +
> + free(oir);
> + free(srr);
> + return connector;
> +}
>
[...]
> +VkResult
> +wsi_acquire_xlib_display(VkPhysicalDevice physical_device,
> + struct wsi_device *wsi_device,
> + Display *dpy,
> + VkDisplayKHR display)
> +{
> + struct wsi_display *wsi =
> + (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLA
> TFORM_DISPLAY];
> + xcb_connection_t *connection = XGetXCBConnection(dpy);
> + struct wsi_display_connector *connector =
> + wsi_display_connector_from_handle(display);
> + xcb_window_t root;
> +
> + /* XXX no support for multiple leases yet */
> + if (wsi->fd >= 0)
> + return VK_ERROR_OUT_OF_DATE_KHR;
>
This function is supposed to return either VK_SUCCESS or
VK_ERROR_INITIALIZATION_FAILED. The errors here and below should probably
all be VK_ERROR_INITIALIZATION_FAILED.
> +
> + if (!connector->output) {
> + connector->output = wsi_display_connector_id_to_output(connection,
> +
> connector->id);
> +
> + /* Check and see if we found the output */
> + if (!connector->output)
> + return VK_ERROR_OUT_OF_DATE_KHR;
> + }
> +
> + root = wsi_display_output_to_root(connection, connector->output);
> + if (!root)
> + return VK_ERROR_OUT_OF_DATE_KHR;
> +
> + xcb_randr_crtc_t crtc = wsi_display_find_crtc_for_output(connection,
> + root,
> +
> connector->output);
> +
> + if (!crtc)
> + return VK_ERROR_OUT_OF_DATE_KHR;
> +
> + xcb_randr_lease_t lease = xcb_generate_id(connection);
> + xcb_randr_create_lease_cookie_t cl_c =
> + xcb_randr_create_lease(connection, root, lease, 1, 1,
> + &crtc, &connector->output);
> + xcb_randr_create_lease_reply_t *cl_r =
> + xcb_randr_create_lease_reply(connection, cl_c, NULL);
> + if (!cl_r)
> + return VK_ERROR_OUT_OF_DATE_KHR;
> +
> + int fd = -1;
> + if (cl_r->nfd > 0) {
> + int *rcl_f = xcb_randr_create_lease_reply_fds(connection, cl_r);
> +
> + fd = rcl_f[0];
> + }
> + free (cl_r);
> + if (fd < 0)
> + return VK_ERROR_OUT_OF_DATE_KHR;
> +
> + wsi->fd = fd;
> +
> + return VK_SUCCESS;
> +}
>
Everything else looks good to me.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180613/88cdacf7/attachment-0001.html>
More information about the mesa-dev
mailing list