<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 11, 2018 at 10:39 PM, Keith Packard <<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:eric.engestrom@imgtec.com" target="_blank">eric.engestrom@imgtec.com</a>> 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 <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> 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 <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>> fixup for acquire fixup for RROutput type Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>> fixup </blockquote><div> </div><div>Lots of "fixup". Did you mean to actually comment on what that was? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a> | 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(+) </blockquote><div> </div><div>[...] </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +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 && </blockquote><div> </div><div>You're not checking vscan here. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + wsi->flags == xcb->mode_flags; +} </blockquote><div> </div><div>[...] </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +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_PLATFORM_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); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + /* 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; +} </blockquote><div> </div><div>[...] </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +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_PLATFORM_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; </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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; +} </blockquote><div> </div><div>Everything else looks good to me.</div><div> </div><div>--Jason </div></div> </div></div>