<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 21, 2018 at 7:37 AM, 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"><span>Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes:<br>
<br>
>> +      if (!ret)<br>
>> +         return VK_SUCCESS;<br>
>> +<br>
>> +      if (errno != ENOMEM) {<br>
><br>
> This strikes me as a bit odd. What does ENOMEM mean if not "out of<br>
> memory"?<br>
<br>
</span>ENOMEM means that the queue is full and that we should drain it and try<br>
again; that's what the wait_for_event call is below.<br>
<br>
The other-than-ENOMEM case is for some other failure, such as VT switch<br>
or lease revoke. For RegisterDisplayEvent, there aren't any return<br>
values other than VK_SUCCESS defined, and we're already assuming we can<br>
use VK_OUT_OF_HOST_MEMORY for any function which allocates memory.<br>
<br>
I think the correct value might be VK_ERROR_DEVICE_LOST or<br>
VK_ERROR_OUT_OF_DATE_KHR as something "bad" has clearly happened? The<br>
other place this is called is from QueuePresent, where either of those<br>
error codes are allowed. I could convert that message to<br>
VK_OUT_OF_HOST_MEMORY for RegisterDisplayEvent if you think that's a<br>
good idea.<br>
<br>
The sleep prevents an application from spinning at this failure,<br>
allowing the user to gracefully terminate the application.<br>
<span><br>
><br>
>> +         wsi_display_debug("queue vblank event %lu failed\n", <br>
>> fence->sequence);<br>
>> +         struct timespec delay = {<br>
>> +            .tv_sec = 0,<br>
>> +            .tv_nsec = 100000000ull,<br>
>> +         };<br>
>> +         nanosleep(&delay, NULL);<br>
>> +         return VK_ERROR_OUT_OF_HOST_MEMORY;<br>
><br>
> Given your previous explanation, I think this is ok but I think it deserves <br>
> a comment.<br>
<br>
</span>Wilco.<br>
<br>
I've added comments to this section to try and explain what's going on:<br>
<br>
      if (!ret)<br>
         return VK_SUCCESS;<br>
<br>
      if (errno != ENOMEM) {<br>
<br>
         /* Something unexpected happened. Pause for a moment so the<br>
          * application doesn't just spin and then return a failure indication<br>
          */<br>
<span><br>
         wsi_display_debug("queue vblank event %lu failed\n", fence->sequence);<br>
</span>         struct timespec delay = {<br>
            .tv_sec = 0,<br>
            .tv_nsec = 100000000ull,<br>
         };<br>
         nanosleep(&delay, NULL);<br>
         return VK_ERROR_OUT_OF_HOST_MEMORY;<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
      }<br>
<br>
      /* The kernel event queue is full. Wait for some events to be<br>
       * processed and try again<br>
       */<br>
<br>
      pthread_mutex_lock(&wsi->wait_<wbr>mutex);<br>
      ret = wsi_display_wait_for_event(wsi<wbr>, wsi_rel_to_abs_time(100000000u<wbr>ll));<br>
      pthread_mutex_unlock(&wsi->wai<wbr>t_mutex);<br>
<br>
      if (ret) {<br>
<span>         wsi_display_debug("vblank queue full, event wait failed\n");<br>
</span>         return VK_ERROR_OUT_OF_HOST_MEMORY;<br>
      }</blockquote><div><br></div><div>Looks good.  With that, patches 1-3 are</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>></div><div><br></div><div>I'll let Dave or Bas review your fence hackery in radv.<br></div></div></div></div>