Continuing on the new version of the patch...

On Tue, Feb 13, 2018 at 4:31 PM, Keith Packard <kei...@keithp.com> wrote:

> +enum wsi_image_state {
> +   wsi_image_idle,
> +   wsi_image_drawing,
> +   wsi_image_queued,
> +   wsi_image_flipping,
> +   wsi_image_displaying
> +};
>

With the exception of NIR (which is a bit odd), we usually use ALL_CAPS for
enum values.


> +
> +static const VkPresentModeKHR available_present_modes[] = {
> +   VK_PRESENT_MODE_FIFO_KHR,
>

Yes, this does seem like the only reasonable mode for now.  I suppose you
could check to see if the platform supports ASYNC_FLIP and advertise
IMMEDIATE in that case.  I know intel doesn't advertise it but I thought
amdgpu does.


> +};
> +
> +static VkResult
> +wsi_display_surface_get_present_modes(VkIcdSurfaceBase  *surface,
> +                                      uint32_t
> *present_mode_count,
> +                                      VkPresentModeKHR  *present_modes)
> +{
> +   if (present_modes == NULL) {
> +      *present_mode_count = ARRAY_SIZE(available_present_modes);
> +      return VK_SUCCESS;
> +   }
> +
> +   *present_mode_count = MIN2(*present_mode_count,
> ARRAY_SIZE(available_present_modes));
> +   typed_memcpy(present_modes, available_present_modes,
> *present_mode_count);
> +
> +   if (*present_mode_count < ARRAY_SIZE(available_present_modes))
> +      return VK_INCOMPLETE;
> +   return VK_SUCCESS;
>

vk_outarray, though I suppose you've probably already made that change.


> +}
> +
> +static VkResult
> +wsi_display_image_init(VkDevice                         device_h,
> +                       struct wsi_swapchain             *drv_chain,
> +                       const VkSwapchainCreateInfoKHR   *create_info,
> +                       const VkAllocationCallbacks      *allocator,
> +                       struct wsi_display_image         *image)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +   struct wsi_display           *wsi = chain->wsi;
> +   VkResult                     result;
> +   int                          ret;
> +   uint32_t                     image_handle;
> +
> +   if (chain->base.use_prime_blit)
> +      result = wsi_create_prime_image(&chain->base, create_info,
> &image->base);
>

I don't see use_prime_blit being set anywhere.  Perhaps that comes in a
later patch?  The line is obviously correct, so I'm fine with leaving it.


> +   else
> +      result = wsi_create_native_image(&chain->base, create_info,
> &image->base);
>

This will have to be updated for modifiers.  I'm reasonably happy for it to
be the no-op update for now though KHR_display supporting real modifiers
would be nice. :-)


> +   if (result != VK_SUCCESS)
> +      return result;
> +
> +   ret = drmPrimeFDToHandle(wsi->master_fd, image->base.fd,
> &image_handle);
>

This opens up some interesting questions.  GEM handles are not reference
counted by the kernel.  If the driver that we're running on is using
master_fd, then we shouldn't close our handle because that would close the
handle for the driver as well.  If it's not using master_fd, we should
close the handle to avoid leaking it.  The later is only a worry in the
prime case but given that I saw a line above about use_prime_blit, you have
me scared. :-)

One solution to this connundrum would be to simply never use the master FD
for the Vulkan driver itself and always open a render node.  If handed a
master FD, you could check if it matches your render node and set
use_prime_blit accordingly.  Then the driver and WSI would have separate
handle domains and this problem would be solved.  You could also just open
the master FD twice, once for WSI and once for the driver.


> +
> +   close(image->base.fd);
> +   image->base.fd = -1;
> +
> +   if (ret < 0)
> +      goto fail_handle;
> +
> +   image->chain = chain;
> +   image->state = wsi_image_idle;
> +   image->fb_id = 0;
> +
> +   /* XXX extract depth and bpp from image somehow */
>

You have the format in create_info.  We could add some generic VkFormat
introspection or we can just handle the 2-4 cases we care about directly.


> +   ret = drmModeAddFB(wsi->master_fd, create_info->imageExtent.width,
> create_info->imageExtent.height,
> +                      24, 32, image->base.row_pitch, image_handle,
> &image->fb_id);
>

What happens if we close the handle immediately after calling
drmModeAddFB?  Does the FB become invalid?  If so, then we probably want to
stash the handle so we can clean it up when we destroy the swapchain.
Unless, of course, we're willing to make the assumption (as stated above)
that the driver and WSI are always using the same FD.


> +
> +   if (ret)
> +      goto fail_fb;
> +
> +   return VK_SUCCESS;
> +
> +fail_fb:
> +   /* fall through */
> +
> +fail_handle:
> +   wsi_destroy_image(&chain->base, &image->base);
> +
> +   return VK_ERROR_OUT_OF_HOST_MEMORY;
> +}
> +
> +static void
> +wsi_display_image_finish(struct wsi_swapchain           *drv_chain,
> +                         const VkAllocationCallbacks    *allocator,
> +                         struct wsi_display_image       *image)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +
> +   wsi_destroy_image(&chain->base, &image->base);
> +}
> +
> +static VkResult
> +wsi_display_swapchain_destroy(struct wsi_swapchain
> *drv_chain,
> +                              const VkAllocationCallbacks
>  *allocator)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +
> +   for (uint32_t i = 0; i < chain->base.image_count; i++)
> +      wsi_display_image_finish(drv_chain, allocator, &chain->images[i]);
> +   vk_free(allocator, chain);
> +   return VK_SUCCESS;
> +}
> +
> +static struct wsi_image *
> +wsi_display_get_wsi_image(struct wsi_swapchain  *drv_chain,
> +                          uint32_t              image_index)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +
> +   return &chain->images[image_index].base;
> +}
> +
> +static void
> +wsi_display_idle_old_displaying(struct wsi_display_image *active_image)
> +{
> +   struct wsi_display_swapchain *chain = active_image->chain;
> +
> +   wsi_display_debug("idle everyone but %ld\n", active_image -
> &(chain->images[0]));
> +   for (uint32_t i = 0; i < chain->base.image_count; i++)
> +      if (chain->images[i].state == wsi_image_displaying &&
> &chain->images[i] != active_image) {
> +         wsi_display_debug("idle %d\n", i);
> +         chain->images[i].state = wsi_image_idle;
> +      }
> +}
> +
> +static VkResult
> +_wsi_display_queue_next(struct wsi_swapchain     *drv_chain);
> +
> +static void
> +wsi_display_page_flip_handler2(int              fd,
> +                               unsigned int     frame,
> +                               unsigned int     sec,
> +                               unsigned int     usec,
> +                               uint32_t         crtc_id,
> +                               void             *data)
> +{
> +   struct wsi_display_image     *image = data;
> +
> +   wsi_display_debug("image %ld displayed at %d\n", image -
> &(image->chain->images[0]), frame);
> +   image->state = wsi_image_displaying;
> +   wsi_display_idle_old_displaying(image);
> +   (void) _wsi_display_queue_next(&(image->chain->base));
> +}
> +
> +static void wsi_display_page_flip_handler(int fd, unsigned int frame,
> +                                          unsigned int sec, unsigned int
> usec, void *data)
> +{
> +   wsi_display_page_flip_handler2(fd, frame, sec, usec, 0, data);
> +}
> +
> +static drmEventContext event_context = {
> +   .version = DRM_EVENT_CONTEXT_VERSION,
> +   .page_flip_handler = wsi_display_page_flip_handler,
> +#if DRM_EVENT_CONTEXT_VERSION >= 3
> +   .page_flip_handler2 = wsi_display_page_flip_handler2,
> +#endif
> +};
> +
> +static void *
> +wsi_display_wait_thread(void *data)
> +{
> +   struct wsi_display   *wsi = data;
> +   struct pollfd pollfd = {
> +      .fd = wsi->master_fd,
> +      .events = POLLIN
> +   };
> +   int ret;
> +
> +   pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
> +   for (;;) {
> +      ret = poll(&pollfd, 1, -1);
> +      if (ret > 0) {
> +         pthread_mutex_lock(&wsi->wait_mutex);
> +         (void) drmHandleEvent(wsi->master_fd, &event_context);
> +         pthread_mutex_unlock(&wsi->wait_mutex);
> +         pthread_cond_broadcast(&wsi->wait_cond);
> +      }
> +   }
> +   return NULL;
> +}
> +
> +static int
> +wsi_display_start_wait_thread(struct wsi_display        *wsi)
> +{
> +   if (!wsi->wait_thread) {
> +      int ret = pthread_create(&wsi->wait_thread, NULL,
> wsi_display_wait_thread, wsi);
> +      if (ret)
> +         return ret;
> +   }
> +   return 0;
> +}
> +
> +/* call with wait_mutex held */
>

It might be worth a line in the comment to say that this function does not
guarntee that any particular type of event (or indeed any event at all) has
been processed.  It's just a yield.


> +static int
> +wsi_display_wait_for_event(struct wsi_display           *wsi,
> +                           uint64_t                     timeout_ns)
> +{
> +   int ret;
> +
> +   ret = wsi_display_start_wait_thread(wsi);
> +
> +   if (ret)
> +      return ret;
> +
> +   struct timespec abs_timeout = {
> +      .tv_sec = timeout_ns / ((uint64_t) 1000 * (uint64_t) 1000 *
> (uint64_t) 1000),
> +      .tv_nsec = timeout_ns % ((uint64_t) 1000 * (uint64_t) 1000 *
> (uint64_t) 1000)
> +   };
> +
> +   ret = pthread_cond_timedwait(&wsi->wait_cond, &wsi->wait_mutex,
> &abs_timeout);
>
+
> +   wsi_display_debug("%9ld done waiting for event %d\n", pthread_self(),
> ret);
> +   return ret;
> +}
> +
> +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;
> +
> +   if (timeout != 0 && timeout != UINT64_MAX)
> +      timeout += wsi_get_current_monotonic();
> +
> +   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;
>

I don't see anywhere where you track any sort of per-swapchain error
status.  We need something so that, once you get VK_ERROR_OUT_OF_DATE, most
of the other functions will start bailing immediately with OUT_OF_DATE
instead of assuming the client catches it the first time it's returned.  In
particular, if wsi_display_queue_next fails inside an event handler, we
need to know.  In the X11 WSI patches, we just have a VkResult called
"status" in the swapchain.


> +         goto done;
> +      }
> +   }
> +done:
> +   pthread_mutex_unlock(&wsi->wait_mutex);
> +   return result;
> +}
> +
> +/*
> + * Check whether there are any other connectors driven by this crtc
> + */
> +static bool
> +wsi_display_crtc_solo(struct wsi_display        *wsi,
> +                      drmModeResPtr             mode_res,
> +                      drmModeConnectorPtr       connector,
> +                      uint32_t                  crtc_id)
> +{
> +   int                  c, e;
> +
> +   /* See if any other connectors share the same encoder */
> +   for (c = 0; c < mode_res->count_connectors; c++) {
> +      if (mode_res->connectors[c] == connector->connector_id)
> +         continue;
> +
> +      drmModeConnectorPtr       other_connector =
> drmModeGetConnector(wsi->master_fd, mode_res->connectors[c]);
> +      if (other_connector) {
> +         bool                      match = (other_connector->encoder_id
> == connector->encoder_id);
>

Lots of whitespace in here.  We don't usually try to line stuff up in code
unless there's a good reason.


> +         drmModeFreeConnector(other_connector);
> +         if (match)
> +            return false;
> +      }
> +   }
> +
> +   /* See if any other encoders share the same crtc */
> +   for (e = 0; e < mode_res->count_encoders; e++) {
> +      if (mode_res->encoders[e] == connector->encoder_id)
> +         continue;
> +
> +      drmModeEncoderPtr         other_encoder =
> drmModeGetEncoder(wsi->master_fd, mode_res->encoders[e]);
> +      if (other_encoder) {
> +         bool                      match = (other_encoder->crtc_id ==
> crtc_id);
>

Same here.


> +         drmModeFreeEncoder(other_encoder);
> +         if (match)
> +            return false;
> +      }
> +   }
> +   return true;
> +}
> +
> +/*
> + * 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->master_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))
> +               return crtc_id;
> +         }
> +      }
> +   }
> +   crtc_id = 0;
> +   for (c = 0; crtc_id == 0 && c < mode_res->count_crtcs; c++) {
> +      drmModeCrtcPtr crtc = drmModeGetCrtc(wsi->master_fd,
> mode_res->crtcs[c]);
> +      if (crtc && crtc->buffer_id == 0)
> +         crtc_id = crtc->crtc_id;
> +      drmModeFreeCrtc(crtc);
>

I take it that 0 is not a valid crtc_id?


> +   }
> +   return crtc_id;
> +}
> +
> +static VkResult
> +wsi_display_setup_connector(wsi_display_connector       *connector,
> +                            wsi_display_mode            *display_mode)
> +{
> +   struct wsi_display   *wsi = connector->wsi;
> +   drmModeModeInfoPtr   drm_mode;
> +   drmModeConnectorPtr  drm_connector;
> +   drmModeResPtr        mode_res;
> +   VkResult             result;
> +   int                  m;
> +
> +   if (connector->current_mode == display_mode && connector->crtc_id)
> +      return VK_SUCCESS;
> +
> +   mode_res = drmModeGetResources(wsi->master_fd);
> +   if (!mode_res) {
> +      result = VK_ERROR_INITIALIZATION_FAILED;
>

This looks more like an OUT_OF_DATE to me.  Unless, of course, it's
actually an OUT_OF_HOST_MEMORY.


> +      goto bail;
> +   }
> +
> +   drm_connector = drmModeGetConnectorCurrent(wsi->master_fd,
> connector->id);
> +   if (!drm_connector) {
> +      result = VK_ERROR_INITIALIZATION_FAILED;
>

Same here.


> +      goto bail_mode_res;
> +   }
> +
> +   /* Pick a CRTC if we don't have one */
> +   if (!connector->crtc_id) {
> +      connector->crtc_id = wsi_display_select_crtc(connector, mode_res,
> drm_connector);
> +      if (!connector->crtc_id) {
> +         result = VK_ERROR_OUT_OF_DATE_KHR;
> +         goto bail_connector;
> +      }
> +   }
> +
> +   if (connector->current_mode != display_mode) {
> +
>

Extra new line?


> +      /* Find the drm mode cooresponding to the requested VkDisplayMode */
> +      drm_mode = NULL;
> +      for (m = 0; m < drm_connector->count_modes; m++) {
> +         drm_mode = &drm_connector->modes[m];
> +         if (wsi_display_mode_matches_drm(display_mode, drm_mode))
> +            break;
> +         drm_mode = NULL;
> +      }
> +
> +      if (!drm_mode) {
> +         result = VK_ERROR_OUT_OF_DATE_KHR;
> +         goto bail_connector;
> +      }
> +
> +      connector->current_mode = display_mode;
> +      connector->current_drm_mode = *drm_mode;
> +   }
> +
> +   result = VK_SUCCESS;
> +
> +bail_connector:
> +   drmModeFreeConnector(drm_connector);
> +bail_mode_res:
> +   drmModeFreeResources(mode_res);
> +bail:
> +   return result;
> +
> +}
> +
> +/*
> + * 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->master_fd < 0)
> +      return VK_ERROR_INITIALIZATION_FAILED;
>

Is this possible?  If so, it should be OUT_OF_DATE


> +
> +   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->master_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 one half uses braces, please use braces on the other half.


> +
> +      if (ret) {
> +         switch(-ret) {
> +         case EINVAL:
> +
> +            result = wsi_display_setup_connector(connector,
> display_mode);
> +
> +            if (result != VK_SUCCESS) {
> +               image->state = wsi_image_idle;
> +               return result;
>

As I stated earlier, I think we want some sort of chain->status so that we
can easily know that things have gone out-of-date and we don't end up
retrying just because the client called QueuePresent again.


> +            }
> +
> +            /* XXX allow setting of position */
> +
> +            ret = drmModeSetCrtc(wsi->master_fd, connector->crtc_id,
> image->fb_id, 0, 0,
> +                                 &connector->id, 1,
> &connector->current_drm_mode);
> +
> +            if (ret == 0) {
> +               image->state = wsi_image_displaying;
> +               wsi_display_idle_old_displaying(image);
>

Can we really idle them immediately or do we need to wait until this image
has begun to scan out before returning a new image from AcquireNextImage?
I suppose we do use implicit fencing for all WSI images today so this is
probably ok.  Still, might be worth a TODO comment or something.


> +               connector->active = true;
> +               return VK_SUCCESS;
> +            }
> +            break;
> +         case EACCES:
> +            usleep(1000 * 1000);
>

?


> +            connector->active = false;
> +            break;
> +         default:
> +            connector->active = false;
> +            image->state = wsi_image_idle;
> +            return VK_ERROR_OUT_OF_DATE_KHR;
> +         }
> +      }
> +   }
> +}
> +
> +static VkResult
> +wsi_display_queue_present(struct wsi_swapchain          *drv_chain,
> +                          uint32_t                      image_index,
> +                          const VkPresentRegionKHR      *damage)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +   struct wsi_display           *wsi = chain->wsi;
> +   struct wsi_display_image     *image = &chain->images[image_index];
> +   VkResult                     result;
> +
> +   assert(image->state == wsi_image_drawing);
> +   wsi_display_debug("present %d\n", image_index);
> +
> +   pthread_mutex_lock(&wsi->wait_mutex);
> +
>

assert(image->state == wsi_image_idle);


> +   image->flip_sequence = ++chain->flip_sequence;
> +   image->state = wsi_image_queued;
> +
> +   result = _wsi_display_queue_next(drv_chain);
> +
> +   pthread_mutex_unlock(&wsi->wait_mutex);
> +
> +   return result;
> +}
> +
> +static VkResult
> +wsi_display_surface_create_swapchain(VkIcdSurfaceBase
>  *icd_surface,
> +                                     VkDevice
>  device,
> +                                     struct wsi_device
> *wsi_device,
> +                                     int
> local_fd,
> +                                     const VkSwapchainCreateInfoKHR
>  *create_info,
> +                                     const VkAllocationCallbacks
> *allocator,
> +                                     struct wsi_swapchain
>  **swapchain_out)
> +{
> +   struct wsi_display *wsi = (struct wsi_display *)
> wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
> +   VkResult result;
> +
> +   assert(create_info->sType == VK_STRUCTURE_TYPE_SWAPCHAIN_
> CREATE_INFO_KHR);
> +
> +   struct wsi_display_swapchain *chain;
> +   const unsigned num_images = create_info->minImageCount;
> +   size_t size = sizeof(*chain) + num_images * sizeof(chain->images[0]);
> +
> +   chain = vk_alloc(allocator, size, 8,
> +                    VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> +
> +   if (chain == NULL)
> +      return VK_ERROR_OUT_OF_HOST_MEMORY;
> +
> +   result = wsi_swapchain_init(wsi_device, &chain->base, device,
> +                               create_info, allocator);
> +
> +   chain->base.destroy = wsi_display_swapchain_destroy;
> +   chain->base.get_wsi_image = wsi_display_get_wsi_image;
> +   chain->base.acquire_next_image = wsi_display_acquire_next_image;
> +   chain->base.queue_present = wsi_display_queue_present;
> +   chain->base.present_mode = create_info->presentMode;
> +   chain->base.image_count = num_images;
> +
> +   chain->wsi = wsi;
> +
> +   chain->surface = (VkIcdSurfaceDisplay *) icd_surface;
> +
> +   for (uint32_t image = 0; image < chain->base.image_count; image++) {
> +      result = wsi_display_image_init(device, &chain->base, create_info,
> allocator,
> +                                      &chain->images[image]);
> +      if (result != VK_SUCCESS)
> +         goto fail_init_images;
> +   }
> +
> +   *swapchain_out = &chain->base;
> +
> +   return VK_SUCCESS;
> +
> +fail_init_images:
>

You need to clean up if you are only able to create some of the images.


> +   return result;
> +}
> +
> +VkResult
> +wsi_display_init_wsi(struct wsi_device *wsi_device,
> +                     const VkAllocationCallbacks *alloc,
> +                     VkPhysicalDevice physical_device,
>

You can get the physical device from the wsi_device.  No need to pass it in
separately.


> +                     int device_fd)
> +{
> +   struct wsi_display *wsi;
> +   VkResult result;
> +
> +   wsi = vk_alloc(alloc, sizeof(*wsi), 8,
> +                   VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
> +   if (!wsi) {
> +      result = VK_ERROR_OUT_OF_HOST_MEMORY;
> +      goto fail;
> +   }
> +   memset(wsi, '\0', sizeof (*wsi));
> +
> +   wsi->master_fd = -1;
> +   if (drmGetNodeTypeFromFd(device_fd) == DRM_NODE_PRIMARY)
> +      wsi->master_fd = device_fd;
> +   wsi->render_fd = device_fd;
> +
> +   pthread_mutex_init(&wsi->wait_mutex, NULL);
> +   wsi->physical_device = physical_device;
>

I don't see this being used anywhere.  Is it needed?


> +   wsi->alloc = alloc;
> +
> +   LIST_INITHEAD(&wsi->display_modes);
> +   LIST_INITHEAD(&wsi->connectors);
> +
> +   pthread_condattr_t condattr;
> +   int ret;
> +
> +   ret = pthread_mutex_init(&wsi->wait_mutex, NULL);
> +   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);
> +
> +   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;
> +}
> +
> +void
> +wsi_display_finish_wsi(struct wsi_device *wsi_device,
> +                       const VkAllocationCallbacks *alloc)
> +{
> +   struct wsi_display *wsi = (struct wsi_display *)
> wsi_device->wsi[VK_ICD_WSI_PLATFORM_DISPLAY];
> +
> +   if (wsi) {
> +
> +      struct wsi_display_connector *connector, *connector_storage;
> +      LIST_FOR_EACH_ENTRY_SAFE(connector, connector_storage,
> &wsi->connectors, list) {
> +         vk_free(wsi->alloc, connector);
> +      }
> +
> +      struct wsi_display_mode *mode, *mode_storage;
> +      LIST_FOR_EACH_ENTRY_SAFE(mode, mode_storage, &wsi->display_modes,
> list) {
> +         vk_free(wsi->alloc, mode);
> +      }
> +
> +      pthread_mutex_lock(&wsi->wait_mutex);
> +      if (wsi->wait_thread) {
> +         pthread_cancel(wsi->wait_thread);
> +         pthread_join(wsi->wait_thread, NULL);
> +      }
> +      pthread_mutex_unlock(&wsi->wait_mutex);
> +      pthread_mutex_destroy(&wsi->wait_mutex);
> +      pthread_cond_destroy(&wsi->wait_cond);
> +
> +      vk_free(alloc, wsi);
> +   }
> +}
> diff --git a/src/vulkan/wsi/wsi_common_display.h
> b/src/vulkan/wsi/wsi_common_display.h
> new file mode 100644
> index 00000000000..b414a226293
> --- /dev/null
> +++ b/src/vulkan/wsi/wsi_common_display.h
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright © 2017 Keith Packard
> + *
> + * Permission to use, copy, modify, distribute, and sell this software
> and its
> + * documentation for any purpose is hereby granted without fee, provided
> that
> + * the above copyright notice appear in all copies and that both that
> copyright
> + * notice and this permission notice appear in supporting documentation,
> and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no
> representations
> + * about the suitability of this software for any purpose.  It is
> provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT
> OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
> USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
> PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#ifndef WSI_COMMON_DISPLAY_H
> +#define WSI_COMMON_DISPLAY_H
> +
> +#include "wsi_common.h"
> +#include <xf86drm.h>
> +#include <xf86drmMode.h>
> +
> +#define typed_memcpy(dest, src, count) ({ \
> +   STATIC_ASSERT(sizeof(*src) == sizeof(*dest)); \
> +   memcpy((dest), (src), (count) * sizeof(*(src))); \
> +})
> +
> +VkResult
> +wsi_display_get_physical_device_display_properties(VkPhysicalDevice
>        physical_device,
> +                                                   struct wsi_device
>       *wsi_device,
> +                                                   uint32_t
>        *property_count,
> +
>  VkDisplayPropertiesKHR       *properties);
> +VkResult
> +wsi_display_get_physical_device_display_plane_properties(VkPhysicalDevice
>              physical_device,
> +                                                         struct
> wsi_device              *wsi_device,
> +                                                         uint32_t
>                *property_count,
> +
>  VkDisplayPlanePropertiesKHR    *properties);
> +
> +VkResult
> +wsi_display_get_display_plane_supported_displays(VkPhysicalDevice
>        physical_device,
> +                                                 struct wsi_device
>       *wsi_device,
> +                                                 uint32_t
>        plane_index,
> +                                                 uint32_t
>        *display_count,
> +                                                 VkDisplayKHR
>        *displays);
> +VkResult
> +wsi_display_get_display_mode_properties(VkPhysicalDevice
>  physical_device,
> +                                        struct wsi_device
> *wsi_device,
> +                                        VkDisplayKHR
>  display,
> +                                        uint32_t
>  *property_count,
> +                                        VkDisplayModePropertiesKHR
>  *properties);
> +
> +VkResult
> +wsi_get_display_plane_capabilities(VkPhysicalDevice
>  physical_device,
> +                                   struct wsi_device
> *wsi_device,
> +                                    VkDisplayModeKHR
> mode_khr,
> +                                    uint32_t
> plane_index,
> +                                    VkDisplayPlaneCapabilitiesKHR
>  *capabilities);
> +
> +VkResult
> +wsi_create_display_surface(VkInstance instance,
> +                           const VkAllocationCallbacks *pAllocator,
> +                           const VkDisplaySurfaceCreateInfoKHR
> *pCreateInfo,
> +                           VkSurfaceKHR *pSurface);
> +
> +#endif
> diff --git a/src/vulkan/wsi/wsi_common_private.h
> b/src/vulkan/wsi/wsi_common_private.h
> index 503b2a015dc..d38d2efa116 100644
> --- a/src/vulkan/wsi/wsi_common_private.h
> +++ b/src/vulkan/wsi/wsi_common_private.h
> @@ -135,6 +135,16 @@ void wsi_wl_finish_wsi(struct wsi_device *wsi_device,
>                         const VkAllocationCallbacks *alloc);
>
>
> +VkResult
> +wsi_display_init_wsi(struct wsi_device *wsi_device,
> +                     const VkAllocationCallbacks *alloc,
> +                     VkPhysicalDevice physical_device,
> +                     int device_fd);
> +
> +void
> +wsi_display_finish_wsi(struct wsi_device *wsi_device,
> +                       const VkAllocationCallbacks *alloc);
> +
>  #define WSI_DEFINE_NONDISP_HANDLE_CASTS(__wsi_type, __VkType)
>   \
>
>   \
>     static inline struct __wsi_type *
>  \
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to