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

["Keith Packard" <keithp@keithp.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3382 bytes --]

Emil Velikov <emil.l.velikov@gmail.com> writes:

> A few top level comments:

Thanks much for looking over this code. I realize it's a large amount of
change and hence more work to review.

>  - using card/primary node and (missing) authentication
> Current code opens the primary node when VK_KHR_display is available.
> Thus running a platform=drm,x11,wayland build will fail, as the client
> is not authenticated with the X/WL server.

this is really mostly there to make it easy to test the KHR_display
backend without needing any further code. I added this only recently for
this purpose. I'm not sure how we should be selecting for opening the
primary device; perhaps another extension that adds a block in the pNext
chain to look for?

I don't know if you saw the first version of this series, but I had
specified a new extension which provided the device FD from the
application, letting you pass in whatever you could open (including a
leased FD from X or a primary opened from the application). That seems
like it could use some design ideas so we can make this do what we
want.

One option would be to remove the primary device code from the drivers
once the leasing code is added so that the only way to use KHR_display
is by leasing resources from the window system?

The radv driver goes a bit further and checks with the kernel to see if
rendering will work. For anv, the ioctls necessary for drawing are
permitting on a non-authenticated node. Both drivers work fine for
direct and X as-is, but I'm not very happy with having a random app with
the primary device open as that seems like a mistake waiting to happen.

>  - reuse "drm" as a shorthand for the "display"
> We've been using drm for egl/gbm, va/drm, etc, one less platform plus
> no equivalent in either of egl/va/...

Yeah, anything that supports drm can support display. I'll review the
build process and try to simplify it further.

>  - remove conditional compilation based on library version/features.
> Eg. checks like the following should be avoided.
> #if DRM_EVENT_CONTEXT_VERSION

Should I just have the build depend on a recent enough version of the library?

>  - use drmModeAddFB2 (or the withModifiers version even) over drmModeAddFB
> Might help with the XXX just above it ;-)

It won't help with that -- I just need more information from the
driver. However, that information could come back as a format instead of
depth/bpp, which would mean using drmModeAddFB2 instead of
drmModeAddFB. Thanks for reminding me of this XXX.  wsi_create_*_image
gets a VkSwapchainCreateInfoKHR which has a VkFormat inside. Is that
sufficient to compute a fourcc format for AddFB2? I think I could
probably guess depth/bpp from it and be right most of the time?

>  - the spec says we're at VK_KHR_display rev 21, while the code advertises rev1
>     Extension('VK_KHR_display',                           1,
> 'VK_USE_PLATFORM_DISPLAY_KHR'),

Thanks. The version of the spec I've got (1.0.49) says it's already at
rev 23!

>  - there are plenty of unnecessary of headers #include(d)

As is often the case. I can go trim things down.

>  - we could simplify the ifdef spaghetti by making
> wsi_*_{init,finish}_wsi empty stubs
> Definitely something that can be done, independently of your work.

That would be lovely.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel