<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 19, 2018 at 9:44 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"><span>Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes:<br>
<br>
<br>
>> +      if (!prop)<br>
>> +         continue;<br>
>> +      if (prop->flags & DRM_MODE_PROP_ENUM) {<br>
>> +         if (!strcmp(prop->name, "DPMS"))<br>
>> +            connector->dpms_property = drm_connector->props[p];<br>
>><br>
><br>
> break?<br>
<br>
</span>Not break; I need to free the property. However, an early exit from the<br>
loop seems reasonable. How about:<br>
<br>
   for (int p = 0; connector->dpms_property == 0 && p < drm_connector->count_props; p++) {<br>
<br>
This skips the whole sequence if the property has already been found, or<br>
stops as soon as it has.<span><br></span></blockquote><div><br></div><div>That seems good to me.  Unless, of course, DPMS is something we expect to change over time somehow.  Then again, we don't handle that at all right now so meh.  Let's go with what you wrote above for now.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
>> +static bool<br>
>> +wsi_display_fence_wait(struct wsi_fence *fence_wsi,<br>
>> +                       bool absolute,<br>
>> +                       uint64_t timeout)<br>
>><br>
><br>
> Would it make more sense for this function to return a VkResult?  Then you<br>
> could tell the difference between success, timeout, and some other<br>
> failure.  I guess the only other thing to return would be<br>
> VK_ERROR_DEVICE_LOST which seems pretty harsh but, then again,<br>
> pthread_timed_wait just failed so that's also really bad.<br>
<br>
</span>That's a good idea. The boolean return is pretty ambiguous. I copied<br>
that from the radv internal fence API, which could also benefit from<br>
this change. I've changed the API and adjusted the anv and radv code to<br>
match. It reads a lot better now.<span><br></span></blockquote><div><br></div><div>Cool.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
>> +   if (!absolute)<br>
>> +      timeout = wsi_rel_to_abs_time(timeout);<br>
>><br>
><br>
> Are relative times really useful?  I suspect it doesn't save you more than<br>
> a couple of lines and it makes the interface weird.<br>
<br>
</span>No. Relative timeouts aren't actually used anywhere either. I've removed them.<br></blockquote><div><br></div><div>Sounds good.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I did catch a mistake in the anv driver looking at this -- the !waitAll<br>
code wasn't bothering to check the fences if the time had already<br>
passed, so an application polling would never catch the fences being<br>
ready. I've changed the while (current_time < timeout) {} to a do {}<br>
while (current_time < timeout) loop.<span><br></span></blockquote><div><br></div><div>Yeah, that was going to be a problem for someone if they ever decided to busy-loop in the app. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
>> +static void<br>
>> +wsi_display_fence_destroy(str<wbr>uct wsi_fence *fence_wsi)<br>
>> +{<br>
>> +   struct wsi_display_fence *fence = (struct wsi_display_fence *)<br>
>> fence_wsi;<br>
>> +<br>
>><br>
><br>
> An assert(!fence->destroyed) in here might be useful to guard against<br>
> double-frees.<br>
<br>
</span>Sure. I was under the impression that application bugs weren't supposed<br>
to be rigorously checked in the implementation though? When should I be<br>
checking API usage issues?<span><br></span></blockquote><div><br></div><div>They shouldn't be and that's why I'm a fan of making them asserts which get compiled out instead of actual checks.  Also, I find this pseudo reference counting to be somewhat confusing and adding asserts informs the reader of the assumptions made.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
>> +      if (!ret)<br>
>> +         return VK_SUCCESS;<br>
>> +<br>
>> +      if (errno != ENOMEM) {<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>
><br>
> Why are we sleeping for 0.1s before we return?  That seems fishy.<br>
<br>
</span>Yeah, the kernel API is not great. There's a finite queue which can be<br>
consumed with both flip events and vblank wait events. If that fills,<br>
we'll get an error back. The only way to empty it is to have some events<br>
get delivered, and those will only get delivered after a vblank happens.<br>
<br>
It's an application bug that triggers this -- requesting too many vblank<br>
events. Throttling the application so it doesn't just spin makes it<br>
possible to stop it.<br>
<span><br>
>> +      pthread_mutex_lock(&wsi->wait_<wbr>mutex);<br>
>> +      ret = wsi_display_wait_for_event(wsi<wbr>, wsi_rel_to_abs_time(<br>
>> 100000000ull));<br>
>><br>
><br>
> What's with the magic number?<br>
<br>
</span>0.1s -- a value which is longer than any display time, but short enough<br>
to catch things like DPMS off or VT switch without unduly delaying the<br>
application.<br>
<span><br>
>> +VkResult<br>
>> +wsi_register_device_event(VkD<wbr>evice device,<br>
>> +                          struct wsi_device *wsi_device,<br>
>> +                          const VkDeviceEventInfoEXT *device_event_info,<br>
>> +                          const VkAllocationCallbacks *allocator,<br>
>> +                          struct wsi_fence **fence_p)<br>
>> +{<br>
>> +   return VK_ERROR_FEATURE_NOT_PRESENT;<br>
>><br>
><br>
> I don't think we're allowed to just not implemnet this.  At the very least,<br>
> we should accept the event and never trigger it.  Better would be to<br>
> actually wire up hotplug detection.  I have no idea how insane that would<br>
> be to do. :-P<br>
<br>
</span>It's not a big deal to implement, I just didn't need it. I suppose the<br>
test suite will be unhappy with this? Let me know if you want to insist<br>
on having it implemented.<span><br></span></blockquote><div><br></div><div>What test suite?  Honestly, I know of no code anywhere that actually uses this API for anything other than VR headsets.</div><div><br></div><div>I guess it's kind-of a question of how much effort we want to put into this.  One option would be to add VK_KHR_display support to vkcube and make it automatically show up on all your displays using hotplug events.</div><div><br></div><div>If we're going to not care, returning VK_ERROR_FEATURE_NOT_PRESENT is probably the best thing to do since at least the app has feedback.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> Both RegisterDeviceEvent and RegisterDisplayEvent say they can only return<br>
> VK_SUCCESS.  We should submit a MR against the extensions to also allow<br>
> OUT_OF_HOST_MEMORY at the very least.<br>
<br>
</span>There's already weasel words in the section on memory allocation that<br>
says the command must generate VK_ERROR_OUT_OF_HOST_MEMORY when<br>
allocation fails. But, it would be nice for these APIs to be documented<br>
as possibly returning that value.<span><br></span></blockquote><div><br></div><div>Yeah.  It's probably a 2-line change.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> Any particular reason to put these all the way down here?  I think my<br>
> preference would be to move wsi_display_fence_event_handle<wbr>r to right after<br>
> wsi_display_fence_check_free and give it a predeclaration (instead of these<br>
> two) and then move the sequence and vblank handlers to right above<br>
> event_context since they're just little wrappers around<br>
> wsi_display_fence_check_free.  Sorry if that's a bit petty but it was hard<br>
> to find wsi_display_fence_check_free all the way down here and it's really<br>
> needed in order to understand the pseudo reference counting you're doing<br>
> with fences.<br>
<br>
</span>Sure; that's easy enough and reduces us to a single forward function<br>
declaration instead of two.<br>
<br>
I've implemented all of the indicated changes above; I'll send out a<br>
replacement patch series shortly.<span class="m_8362526152467808220HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>Awesome.  I think we're really close on this one.</div><div><br></div><div>--Jason<br></div></div></div></div>