<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 15, 2018 at 9:46 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 class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> It seems a little odd to me to default to opening the master node and then<br>
> fall back to the render node if it doesn't work. I suppose that's probably<br>
> ok so long as we ensure that vkGetPhysicalDeviceDisplayProp<wbr>ertiesKHR<br>
> returns no displays if we're on the render node.<br>
><br>
> We could always go back to the DRM fd extension idea but I'm not coming up<br>
> with something nice and clean in the 60 seconds I've thought about it.<br>
<br>
</span>As I said in the last comments about this section, Dave Airlie and I<br>
added this code only recently so that we could test this extension<br>
without also needing the kernel and X leasing changes.<br>
<br>
I think we should decide how to enable this functionality "for real",<br>
and I have two easy options:<br>
<br>
1) Use my KEITHP_kms_display extension (presumably renamed MESA), which<br>
exposes a way to pass the DRM fd from the application into the<br>
driver. This makes it possible for the application to get the FD<br>
through any mechanism at all (including RandR or the new Wayland<br>
extension) and leave that out of the Vulkan code entirely.<br>
<br>
2) Add a new extension which passes a new data structure that directs<br>
the driver to open either the Render or Primary nodes.<br>
<br>
When this is done, we can switch from the current code which tries to<br>
open the Primary node whenever the KHR_display extension is requested.<span class=""><br></span></blockquote><div><br></div><div>I think I like option 1. If the client knows the difference between render and primary for 2, then they are most likely already opening the master node themselves or at least have access to the FD.<br><br></div><div>For an application that just wants to draw some stuff on the screen and is ok with letting Vulkan fully handle KMS for it, the current code may be a fine solution.<br><br></div><div>Sorry, I'm just not feeling all that opinionated about this at the moment. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Would it make anything easier if we just storred the DRM struct here? "No"<br>
> is a perfectly valid answer.<br>
<br>
</span>Nope -- once we add the acquire_xlib extension, we get modes through<br>
either X or DRM, depending on whether we're pre-lease or post-lease.<span class=""><br></span></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"><span class="">
> Any particular reason why the list of modes is global and not in the<br>
> connector? It seems like it would be a tiny bit more efficient and<br>
> convenient to put the list in the connector.<br>
<br>
</span>I think you're right. I have some vague memory of a lifetime issue with<br>
connectors, but can't think of what it might be, even after reviewing<br>
the relevant parts of the Vulkan spec. I've gone ahead and changed it;<br>
seems to work fine.<br>
<span class=""><br>
>> + LIST_FOR_EACH_ENTRY(display_<wbr>mode, &wsi->display_modes, list)<br>
>> + if (display_mode->connector == connector)<br>
>> + display_mode->valid = false;<br>
>><br>
><br>
> Please use braces for loops containing more than one line.<br>
<br>
</span>Well, that was easy to fix -- the condition is now gone :-)<span class=""><br></span></blockquote><div><br></div><div>Even better!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Since we're allocating these in a physical device query, we need to use the<br>
> INSTANCE scope. the OBJECT scope is intended for vkCreate functions to<br>
> allocated data that will live no longer than the associated vkDestroy<br>
> function.<br>
<br>
</span>Thanks! The whole Vulkan memory model remains a mystery to me.<br>
<br>
I've changed allocation of wsi_display_mode and wsi_display_connector to<br>
use SCOPE_INSTANCE. VkIceSurfaceDisplay, wsi_display_fence and<br>
wsi_display_swapchain remain using SCOPE_OBJECT.<br></blockquote><div><br></div><div>That sounds correct to me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I've also changed *all* instances of vk_alloc to use<br>
vk_zalloc. These are all small data structures allocated only during<br>
application startup, so I think the benefit of known memory contents is<br>
worth the cost of memset.<br>
<span class=""><br>
> Hooray for obviously false fixed constants!<br>
><br>
> I know the answer to this will be "EDIDs lie, never trust them!" but can we<br>
> get the real value somehow? As someone who has a 13" laptop with a<br>
> 3200x1800 display, I know that number isn't always right. :-)<br>
<br>
</span>Yes, we could dig the real value out of the EDID, but we'd have to parse<br>
the entire EDID to manage that. I don't want to stick an EDID parser<br>
directly in Mesa, so I'm kinda waiting for someone to create a separate<br>
EDID parsing library that the X server, Mesa and others can share. Until<br>
then, I'd prefer to just lie here.<br></blockquote><div><br></div><div>Clearly, we need systemd-edidd. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> double-;;<br>
<br>
Thx. I remember seeing this while reviewing patches and forgot all about<br>
it...<br>
<span class=""><br>
> From the Vulkan spec:<br>
><br>
> Note:<br>
> For devices which have no natural value to return here, implementations<br>
</span>> *should* return the maximum resolution supported.<br>
<span class="">><br>
> We should walk the list and pick the biggest one.<br>
<br>
</span>I did this intentionally -- most monitors have a preferred resolution,<br>
which is their native pixel size. And, we want to tell applications to<br>
use that size, even if the monitor offers a larger (presumabl scaled)<br>
resolution in their mode list.<span class=""><br></span></blockquote><div><br></div><div>Yes, *if* it has a preferred resolution, we should return that one. If it doesn't, we should walk the list and return the largest instead of just defaulting to 1024x768. At least that's what the spec seems to say to me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> See question about MM_PER_PIXEL above<br>
<br>
</span>Yeah, see response about not boiling the EDID ocean above ;-)<br>
<span class=""><br>
> I know i915 can do better at least in some cases. Is there a practical way<br>
> to expose this? If not, I'm fine with just exposing IDENTITY.<br>
<br>
</span>I'm not seeing this exposed through the common DRM mode interfaces<br>
yet. We should probably consider adding this to the kernel and then<br>
adding it here.<span class=""><br></span></blockquote><div><br></div><div>That's fine.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> This error is not allowed for this function. We should just write 0 to<br>
> property_count and return VK_SUCCESS. Maybe add some asserts for debug<br>
> builds if you really think this shouldn't ever happen.<br>
<br>
</span>I bet it will happen if you VT switch away and then try this function.<br>
<br>
I've added this at the end of the function:<br>
<br>
bail:<br>
*property_count = 0;<br>
return VK_SUCCESS;<span class=""><br></span></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"><span class="">
> This could be made a lot easier with vk_outarray:<br>
</span>...<br>
<span class="">> Again, vk_outarray is your friend.<br>
</span>...<br>
<span class="">> This isn't correct. The loop below could turn up no displays. Again,<br>
> vk_outarray is your friend.<br>
</span>...<br>
<span class="">> Yeah, I sound like a broken record now. vk_outarray. I think this one is<br>
> actually correct, so you could leave it alone if you wanted.<br>
<br>
</span>I've replaced all of the array returns with vk_outarray; I'm afraid I<br>
found a bunch of open-coded versions of this and didn't stumble on<br>
vk_outarray until too late. Thanks!<span class=""><br></span></blockquote><div><br></div><div>You're welcome. I'm so glad Chad made that little helper. 50-75% of the open-coded versions were wrong.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> + /* XXX use actual values */<br>
>><br>
><br>
> That would be good. I don't know enough about KMS to know where you'd get<br>
> those.<br>
<br>
</span>I think the real values will only be useful once we add multi-plane<br>
support to this code. At this point, with only a single plane, I'm not<br>
sure it's interesting to be able to pan it around? So, these values are<br>
'safe', and sufficient to display a single plane covering the full<br>
screen. When we want more, we can go actually hook up multi-plane<br>
support, which will require more work in the window system extensions to<br>
allow applications to request the desired number of planes.<span class=""><br></span></blockquote><div><br></div><div>Sounds good to me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> +static VkResult<br>
>> +wsi_display_surface_get_<wbr>support(VkIcdSurfaceBase *surface,<br>
>> + struct wsi_device *wsi_device,<br>
>> + const VkAllocationCallbacks *allocator,<br>
>> + uint32_t queueFamilyIndex,<br>
>> + int local_fd,<br>
>> + VkBool32* pSupported)<br>
>> +{<br>
>> + *pSupported = VK_TRUE;<br>
>><br>
><br>
> As I commented above, I think we want this to be conditional on whether or<br>
> not you actually got a master FD.<br>
<br>
</span>With just KHR_display, you can't get here without already having needed<br>
a master FD -- you need a VkSurfaceKHR, and you create one of those<br>
using vkCreateDisplayPlaneSurfaceKHR<wbr>, which requires a plane and a mode;<br>
a mode comes from vkGetDisplayModePropertiesKHR or<br>
vkCreateDisplayModeKHR, both of which requires a VkDisplayKHR. A<br>
VkDisplayKHR comes from vkGetPhysicalDeviceDisplayProp<wbr>ertiesKHR, which<br>
fails if you don't have a master FD.<br></blockquote><div><br></div><div>Right. That makes sense.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Once we have AcquireXlibDisplay, then you will be able to create display<br>
surfaces before you have a master FD as you can create modes using X<br>
resources. And, you can't actually tell if you will be able to get a<br>
master FD until you try...<span class=""><br></span></blockquote><div><br></div><div>Yeah<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> + caps->supportedCompositeAlpha = (VK_COMPOSITE_ALPHA_INHERIT_<wbr>BIT_KHR |<br>
>> + VK_COMPOSITE_ALPHA_OPAQUE_BIT_<wbr>KHR);<br>
>><br>
><br>
> I don't think INHERIT is really appropreate here. I don't think it's<br>
> practical to set the transparency using KMS without doing it as part of the<br>
> modeset. Since it really has to be insde the WSI code, we just want<br>
> OPAQUE_BIT for now.<br>
<br>
</span>Yeah, another area which really needs to wait until we figure out what<br>
we want for multi-plane support. If you only have one plane, you don't<br>
exactly have any alpha compositing.<span class=""><br></span></blockquote><div><br></div><div>Yup. Let's just drop INHERIT and only advertise OPAQUE.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> I'm done reviewing for the day. I'll try to resume tomorrow. If you'd<br>
> like me to continue on the new patches, I can do that.<br>
<br>
</span>Thanks so much for your review so far; getting rid of the open-coded<br>
property queries is really valuable, along with using vk_zalloc<br>
everywhere.<br>
<br>
The "new" patches are exactly the same code changes, just split into<br>
core/anv/radv bits to make Dave Airlie happy. Feel free to either<br>
continue with the old patches or to switch over. I've pushed out a<br>
series of tiny patches which address each of the review comments above<br>
separately if you want to check those over. I'll merge them into the<br>
main patch series at some point.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
-keith<br>
</font></span></blockquote></div><br></div></div>