[Mesa-dev] [PATCH 1/7] vulkan: Add KHR_display extension to anv and radv using DRM

[Jason Ekstrand]

On Thu, Feb 15, 2018 at 9:46 AM, Keith Packard <keithp at keithp.com> wrote:

> Jason Ekstrand <jason at jlekstrand.net> 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.
>

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.

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.

Sorry, I'm just not feeling all that opinionated about this at the moment.
:-)


> > 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.
>

Sounds good.


> > 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 :-)
>

Even better!


> > 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.
>

That sounds correct to me.


> 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.
>

Clearly, we need systemd-edidd. :-)


> > 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.
>

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.


> > 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.
>

That's fine.


> > 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;
>

Sounds good.


> > 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!
>

You're welcome.  I'm so glad Chad made that little helper.  50-75% of the
open-coded versions were wrong.


> >> +   /* 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.
>

Sounds good to me.


> >> +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.
>

Right.  That makes sense.


> 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...
>

Yeah


> >> +   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.
>

Yup.  Let's just drop INHERIT and only advertise OPAQUE.


> >  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180223/360d21ff/attachment-0001.html>