<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Mar 7, 2018 at 11:24 PM, Keith Packard <span dir="ltr"><<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+/*<br>
+ * Implement vkGetPhysicalDeviceDisplayProp<wbr>ertiesKHR (VK_KHR_display)<br>
+ */<br>
+VkResult<br>
+wsi_display_get_physical_devi<wbr>ce_display_properties(VkPhysic<wbr>alDevice             physical_device,<br>
+                                                   struct wsi_device            *wsi_device,<br>
+                                                   uint32_t                     *property_count,<br>
+                                                   VkDisplayPropertiesKHR       *properties)<br>
+{<br>
+   struct wsi_display           *wsi = (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLA<wbr>TFORM_DISPLAY];<br>
+   drmModeResPtr                mode_res;<br>
+<br>
+   if (wsi->fd < 0)<br>
+      goto bail;<br>
+<br>
+   mode_res = drmModeGetResources(wsi->fd);<br>
+<br>
+   if (!mode_res)<br>
+      goto bail;<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   VK_OUTARRAY_MAKE(conn, properties, property_count);<br>
+<br>
+   /* Get current information */<br>
+<br>
+   for (int c = 0; c < mode_res->count_connectors; c++) {<br>
+      struct wsi_display_connector *connector =<br>
+         wsi_display_get_connector(wsi<wbr>_device, mode_res->connectors[c]);<br>
+<br>
+      if (!connector) {<br>
+         drmModeFreeResources(mode_res<wbr>);<br>
+         return VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      }<br>
+<br>
+      if (connector->connected) {<br>
+         vk_outarray_append(&conn, prop) {<br>
+            wsi_display_fill_in_display_pr<wbr>operties(wsi_device,<br>
+                                                   connector,<br>
+                                                   prop);<br>
+         }<br>
+      }<br>
+   }<br>
+<br>
+   drmModeFreeResources(mode_res<wbr>);<br>
+<br>
+   return vk_outarray_status(&conn);<br>
+<br>
+bail:<br>
+   *property_count = 0;<br>
+   return VK_SUCCESS;<br>
+}<br>
+<br>
+/*<br>
+ * Implement vkGetPhysicalDeviceDisplayPlan<wbr>ePropertiesKHR (VK_KHR_display<br>
+ */<br>
+VkResult<br>
+wsi_display_get_physical_devi<wbr>ce_display_plane_properties(Vk<wbr>PhysicalDevice               physical_device,<br>
+                                                         struct wsi_device              *wsi_device,<br>
+                                                         uint32_t                       *property_count,<br>
+                                                         VkDisplayPlanePropertiesKHR    *properties)<br>
+{<br>
+   struct wsi_display           *wsi = (struct wsi_display *) wsi_device->wsi[VK_ICD_WSI_PLA<wbr>TFORM_DISPLAY];<br>
+   struct wsi_display_connector *connector;<br>
+<br>
+   VK_OUTARRAY_MAKE(conn, properties, property_count);<br>
+<br>
+   int stack_index = 0;<br>
+<br>
+   LIST_FOR_EACH_ENTRY(connector<wbr>, &wsi->connectors, list) {<br>
+      vk_outarray_append(&conn, prop) {<br>
+         if (connector && connector->active) {<br>
+            prop->currentDisplay = wsi_display_connector_to_handl<wbr>e(connector);<br>
+            prop->currentStackIndex = stack_index++;<br></blockquote><div><br></div><div>In your branch, this is 0.  Why the change?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+         } else {<br>
+            prop->currentDisplay = NULL;<br></blockquote><div><br></div><div>This should probably be VK_NULL_HANDLE<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            prop->currentStackIndex = 0;<br>
+         }<br>
+      }<br>
+   }<br>
+   return vk_outarray_status(&conn);<br>
+} </blockquote><div><br></div><div>[...] <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+static const VkPresentModeKHR available_present_modes[] = {<br>
+   VK_PRESENT_MODE_FIFO_KHR,<br>
+};<br>
+<br>
+static VkResult<br>
+wsi_display_surface_get_prese<wbr>nt_modes(VkIcdSurfaceBase  *surface,<br>
+                                      uint32_t          *present_mode_count,<br>
+                                      VkPresentModeKHR  *present_modes)<br>
+{<br>
+   VK_OUTARRAY_MAKE(conn, present_modes, present_mode_count);<br>
+<br>
+   for (unsigned int c = 0; c < ARRAY_SIZE(available_present_m<wbr>odes); c++) {<br>
+      vk_outarray_append(&conn, present) {<br>
+         *present = available_present_modes[c];<br>
+      }<br>
+   }<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   return vk_outarray_status(&conn);<br>
+}<br>
+<br>
+static void<br>
+wsi_display_destroy_buffer(st<wbr>ruct wsi_display *wsi,<br>
+                           uint32_t buffer)<br>
+{<br>
+   (void) drmIoctl(wsi->fd, DRM_IOCTL_MODE_DESTROY_DUMB,<br>
+                   &((struct drm_mode_destroy_dumb) { .handle = buffer }));<br>
+}<br></blockquote><div><br>[...]<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+static VkResult<br>
+wsi_display_acquire_next_imag<wbr>e(struct wsi_swapchain     *drv_chain,<br>
+                               uint64_t                 timeout,<br>
+                               VkSemaphore              semaphore,<br>
+                               uint32_t                 *image_index)<br>
+{<br>
+   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)drv_chain;<br>
+   struct wsi_display           *wsi = chain->wsi;<br>
+   int                          ret = 0;<br>
+   VkResult                     result = VK_SUCCESS;<br>
+<br>
+   /* Bail early if the swapchain is broken */<br>
+   if (chain->status != VK_SUCCESS)<br>
+      return chain->status;<br>
+<br>
+   if (timeout != 0 && timeout != UINT64_MAX)<br>
+      timeout += wsi_get_current_monotonic();<br></blockquote><div><br></div><div>This may overflow if the provide a large value that is not UINT64_MAX.  We should do something such as<br><br></div><div>uint64_t current = wsi_get_current_monotonic();<br></div><div>if (timeout + current < timeout)<br></div><div>   timeout = UINT64_MAX;<br></div><div>else<br></div><div>   timeout += current;</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   pthread_mutex_lock(&wsi->wait<wbr>_mutex);<br>
+   for (;;) {<br>
+      for (uint32_t i = 0; i < chain->base.image_count; i++) {<br>
+         if (chain->images[i].state == WSI_IMAGE_IDLE) {<br>
+            *image_index = i;<br>
+            wsi_display_debug("image %d available\n", i);<br>
+            chain->images[i].state = WSI_IMAGE_DRAWING;<br>
+            result = VK_SUCCESS;<br>
+            goto done;<br>
+         }<br>
+         wsi_display_debug("image %d state %d\n", i, chain->images[i].state);<br>
+      }<br>
+<br>
+      if (ret == ETIMEDOUT) {<br>
+         result = VK_TIMEOUT;<br>
+         goto done;<br>
+      }<br>
+<br>
+      ret = wsi_display_wait_for_event(wsi<wbr>, timeout);<br>
+<br>
+      if (ret && ret != ETIMEDOUT) {<br>
+         result = VK_ERROR_OUT_OF_DATE_KHR;<br>
+         goto done;<br>
+      }<br>
+   }<br>
+done:<br>
+   pthread_mutex_unlock(&wsi->wa<wbr>it_mutex);<br>
+<br>
+   if (result != VK_SUCCESS)<br>
+      return result;<br>
+<br>
+   return chain->status;<br>
+}<br></blockquote><div><br>[...]<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+/*<br>
+ * Pick a suitable CRTC to drive this connector. Prefer a CRTC which is<br>
+ * currently driving this connector and not any others. Settle for a CRTC<br>
+ * which is currently idle.<br>
+ */<br>
+static uint32_t<br>
+wsi_display_select_crtc(struc<wbr>t wsi_display_connector    *connector,<br>
+                        drmModeResPtr                   mode_res,<br>
+                        drmModeConnectorPtr             drm_connector)<br>
+{<br>
+   struct wsi_display   *wsi = connector->wsi;<br>
+   int                  c;<br>
+   uint32_t             crtc_id;<br>
+<br>
+   /* See what CRTC is currently driving this connector */<br>
+   if (drm_connector->encoder_id) {<br>
+      drmModeEncoderPtr encoder = drmModeGetEncoder(wsi->fd, drm_connector->encoder_id);<br>
+      if (encoder) {<br>
+         crtc_id = encoder->crtc_id;<br>
+         drmModeFreeEncoder(encoder);<br>
+         if (crtc_id) {<br>
+            if (wsi_display_crtc_solo(wsi, mode_res, drm_connector, crtc_id))<br></blockquote><div><br></div><div>This could be crtc_id && wsi_display_crtc_solo(...)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+               return crtc_id;<br>
+         }<br>
+      }<br>
+   }<br>
+   crtc_id = 0;<br>
+   for (c = 0; crtc_id == 0 && c < mode_res->count_crtcs; c++) {<br>
+      drmModeCrtcPtr crtc = drmModeGetCrtc(wsi->fd, mode_res->crtcs[c]);<br>
+      if (crtc && crtc->buffer_id == 0)<br>
+         crtc_id = crtc->crtc_id;<br>
+      drmModeFreeCrtc(crtc);<br>
+   }<br>
+   return crtc_id;<br>
+}<br></blockquote><div><br>[...]<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+/*<br>
+ * Check to see if the kernel has no flip queued and if there's an image<br>
+ * waiting to be displayed.<br>
+ */<br>
+static VkResult<br>
+_wsi_display_queue_next(struc<wbr>t wsi_swapchain     *drv_chain)<br>
+{<br>
+   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *) drv_chain;<br>
+   struct wsi_display           *wsi = chain->wsi;<br>
+   uint32_t                     i;<br>
+   struct wsi_display_image     *image = NULL;<br>
+   VkIcdSurfaceDisplay          *surface = chain->surface;<br>
+   wsi_display_mode             *display_mode = wsi_display_mode_from_handle(s<wbr>urface->displayMode);<br>
+   wsi_display_connector        *connector = display_mode->connector;<br>
+   int                          ret;<br>
+   VkResult                     result;<br>
+<br>
+   if (wsi->fd < 0)<br>
+      return VK_ERROR_INITIALIZATION_FAILED<wbr>;<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+   if (display_mode != connector->current_mode)<br>
+      connector->active = false;<br>
+<br>
+   for (;;) {<br>
+      /* Check to see if there is an image to display, or if some image is already queued */<br>
+<br>
+      for (i = 0; i < chain->base.image_count; i++) {<br>
+         struct wsi_display_image  *tmp_image = &chain->images[i];<br>
+<br>
+         switch (tmp_image->state) {<br>
+         case WSI_IMAGE_FLIPPING:<br>
+            /* already flipping, don't send another to the kernel yet */<br>
+            return VK_SUCCESS;<br>
+         case WSI_IMAGE_QUEUED:<br>
+            /* find the oldest queued */<br>
+            if (!image || tmp_image->flip_sequence < image->flip_sequence)<br>
+               image = tmp_image;<br>
+            break;<br>
+         default:<br>
+            break;<br>
+         }<br>
+      }<br>
+<br>
+      if (!image)<br>
+         return VK_SUCCESS;<br>
+<br>
+      if (connector->active) {<br>
+         ret = drmModePageFlip(wsi->fd, connector->crtc_id, image->fb_id,<br>
+                               DRM_MODE_PAGE_FLIP_EVENT, image);<br>
+         if (ret == 0) {<br>
+            image->state = WSI_IMAGE_FLIPPING;<br>
+            return VK_SUCCESS;<br>
+         }<br>
+         wsi_display_debug("page flip err %d %s\n", ret, strerror(-ret));<br>
+      } else {<br>
+         ret = -EINVAL;<br>
+      }<br>
+<br>
+      if (ret) {<br>
+         switch(-ret) {<br>
+         case EINVAL:<br>
+<br>
+            result = wsi_display_setup_connector(co<wbr>nnector, display_mode);<br>
+<br>
+            if (result != VK_SUCCESS) {<br>
+               image->state = WSI_IMAGE_IDLE;<br></blockquote><div><br></div><div>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?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+               return result;<br>
+            }<br>
+<br>
+            /* XXX allow setting of position */<br>
+<br>
+            ret = drmModeSetCrtc(wsi->fd, connector->crtc_id, image->fb_id, 0, 0,<br>
+                                 &connector->id, 1, &connector->current_drm_mode);<br>
+<br>
+            if (ret == 0) {<br>
+               image->state = WSI_IMAGE_DISPLAYING;<br>
+<br>
+               /* Assume that the mode set is synchronous and that any<br>
+                * previous image is now idle.<br>
+                */<br>
+               wsi_display_idle_old_displayi<wbr>ng(image);<br>
+               connector->active = true;<br>
+               return VK_SUCCESS;<br>
+            }<br></blockquote><div><br></div><div>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?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            break;<br>
+         case EACCES:<br>
+<br>
+            /* Some other VT is currently active. Sit here waiting for<br>
+             * our VT to become active again by polling once a second<br>
+             */<br>
+            usleep(1000 * 1000);<br>
+            connector->active = false;<br>
+            break;<br>
+         default:<br>
+            connector->active = false;<br>
+            image->state = WSI_IMAGE_IDLE;<br></blockquote><div><br></div><div>Again, why are we setting it to IDLE instead of leaving it in QUEUED?  This doesn't seem to do anything useful.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            return VK_ERROR_OUT_OF_DATE_KHR;<br>
+         }<br>
+      }<br>
+   }<br>
+}<br></blockquote><div><br>[...]<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+VkResult<br>
+wsi_display_init_wsi(struct wsi_device *wsi_device,<br>
+                     const VkAllocationCallbacks *alloc,<br>
+                     int display_fd)<br>
+{<br>
+   struct wsi_display *wsi;<br>
+   VkResult result;<br>
+<br>
+   wsi = vk_zalloc(alloc, sizeof(*wsi), 8,<br>
+                   VK_SYSTEM_ALLOCATION_SCOPE_IN<wbr>STANCE);<br>
+   if (!wsi) {<br>
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      goto fail;<br>
+   }<br>
+<br>
+   wsi->fd = display_fd;<br>
+<br>
+   pthread_mutex_init(&wsi->wait<wbr>_mutex, NULL);<br>
+   wsi->alloc = alloc;<br>
+<br>
+   LIST_INITHEAD(&wsi->connector<wbr>s);<br>
+<br>
+   pthread_condattr_t condattr;<br>
+   int ret;<br>
+<br>
+   ret = pthread_mutex_init(&wsi->wait_<wbr>mutex, NULL);<br></blockquote><div><br></div><div>Why are you initializing this twice?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+   if (ret) {<br>
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      goto fail_mutex;<br>
+   }<br>
+<br>
+   ret = pthread_condattr_init(&condatt<wbr>r);<br>
+   if (ret) {<br>
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      goto fail_condattr;<br>
+   }<br>
+<br>
+   ret = pthread_condattr_setclock(&con<wbr>dattr, CLOCK_MONOTONIC);<br>
+   if (ret) {<br>
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      goto fail_setclock;<br>
+   }<br>
+<br>
+   ret = pthread_cond_init(&wsi->wait_c<wbr>ond, &condattr);<br>
+   if (ret) {<br>
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
+      goto fail_cond;<br>
+   }<br>
+<br>
+   pthread_condattr_destroy(&con<wbr>dattr);<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   wsi->base.get_support = wsi_display_surface_get_suppor<wbr>t;<br>
+   wsi->base.get_capabilities = wsi_display_surface_get_capabi<wbr>lities;<br>
+   wsi->base.get_capabilities2 = wsi_display_surface_get_capabi<wbr>lities2;<br>
+   wsi->base.get_formats = wsi_display_surface_get_format<wbr>s;<br>
+   wsi->base.get_formats2 = wsi_display_surface_get_format<wbr>s2;<br>
+   wsi->base.get_present_modes = wsi_display_surface_get_presen<wbr>t_modes;<br>
+   wsi->base.create_swapchain = wsi_display_surface_create_swa<wbr>pchain;<br>
+<br>
+   wsi_device->wsi[VK_ICD_WSI_PL<wbr>ATFORM_DISPLAY] = &wsi->base;<br>
+<br>
+   return VK_SUCCESS;<br>
+<br>
+fail_cond:<br>
+fail_setclock:<br>
+   pthread_condattr_destroy(&con<wbr>dattr);<br>
+fail_condattr:<br>
+   pthread_mutex_destroy(&wsi->w<wbr>ait_mutex);<br>
+fail_mutex:<br>
+   vk_free(alloc, wsi);<br>
+fail:<br>
+   return result;<br>
+}<br></blockquote><div><br>[...]<br><br></div><div>And, there's the end. :-)  Nothing too drastic.  A couple of bugs and a suggestion or two.  On to patch 2. :-)<br><br></div><div>--Jason<br></div></div></div></div>