[Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]

[Jason Ekstrand]

On Thu, Jun 21, 2018 at 7:37 AM, Keith Packard <keithp at keithp.com> wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> >> +      if (!ret)
> >> +         return VK_SUCCESS;
> >> +
> >> +      if (errno != ENOMEM) {
> >
> > This strikes me as a bit odd. What does ENOMEM mean if not "out of
> > memory"?
>
> ENOMEM means that the queue is full and that we should drain it and try
> again; that's what the wait_for_event call is below.
>
> The other-than-ENOMEM case is for some other failure, such as VT switch
> or lease revoke. For RegisterDisplayEvent, there aren't any return
> values other than VK_SUCCESS defined, and we're already assuming we can
> use VK_OUT_OF_HOST_MEMORY for any function which allocates memory.
>
> I think the correct value might be VK_ERROR_DEVICE_LOST or
> VK_ERROR_OUT_OF_DATE_KHR as something "bad" has clearly happened? The
> other place this is called is from QueuePresent, where either of those
> error codes are allowed. I could convert that message to
> VK_OUT_OF_HOST_MEMORY for RegisterDisplayEvent if you think that's a
> good idea.
>
> The sleep prevents an application from spinning at this failure,
> allowing the user to gracefully terminate the application.
>
> >
> >> +         wsi_display_debug("queue vblank event %lu failed\n",
> >> fence->sequence);
> >> +         struct timespec delay = {
> >> +            .tv_sec = 0,
> >> +            .tv_nsec = 100000000ull,
> >> +         };
> >> +         nanosleep(&delay, NULL);
> >> +         return VK_ERROR_OUT_OF_HOST_MEMORY;
> >
> > Given your previous explanation, I think this is ok but I think it
> deserves
> > a comment.
>
> Wilco.
>
> I've added comments to this section to try and explain what's going on:
>
>       if (!ret)
>          return VK_SUCCESS;
>
>       if (errno != ENOMEM) {
>
>          /* Something unexpected happened. Pause for a moment so the
>           * application doesn't just spin and then return a failure
> indication
>           */
>
>          wsi_display_debug("queue vblank event %lu failed\n",
> fence->sequence);
>          struct timespec delay = {
>             .tv_sec = 0,
>             .tv_nsec = 100000000ull,
>          };
>          nanosleep(&delay, NULL);
>          return VK_ERROR_OUT_OF_HOST_MEMORY;
>

I don't really like VK_ERROR_OUT_OF_HOST_MEMORY here but I don't know what
else to do at the moment.  The error codes for this extension are not
well-defined...  I think I'm fine with it for now.


>       }
>
>       /* The kernel event queue is full. Wait for some events to be
>        * processed and try again
>        */
>
>       pthread_mutex_lock(&wsi->wait_mutex);
>       ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(100000000u
> ll));
>       pthread_mutex_unlock(&wsi->wait_mutex);
>
>       if (ret) {
>          wsi_display_debug("vblank queue full, event wait failed\n");
>          return VK_ERROR_OUT_OF_HOST_MEMORY;
>       }


Looks good.  With that, patches 1-3 are

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

I'll let Dave or Bas review your fence hackery in radv.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180621/545c3be8/attachment.html>