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