Implement basic vulkan renderer #1214
Conversation
|
Nice PR!
I assume screencopy works but dmabuf-export doesn't.
Can you try I don't think we want to implement wl_drm -- but we definitely want dmabuf.
+1
Have you designed this with a maybe future software renderer in mind?
I think we can switch to a per-renderer code in the backends, instead of a per-backend code in the renderer. What do you think?
No, you need to destroy the previous one and create a new one.
We should probably use the same device as the DRM backend? |
| bool wlr_output_damage_make_current(struct wlr_output_damage *output_damage, | ||
| bool *needs_swap, pixman_region32_t *damage); | ||
| bool wlr_output_damage_begin(struct wlr_output_damage *output_damage, | ||
| struct wlr_renderer *renderer, bool *needs_swap, pixman_region32_t *damage); |
There was a problem hiding this comment.
The renderer can be obtained from the output.
There was a problem hiding this comment.
oh we don't even need the renderer anymore, good catch.
|
For the abstraction of backend specific details we can probably abstract it in a simliar way to how we did wlr_renderer's various texture things. We can have a list of named APIs (e.g. EGL) which are supported both by backends and renderers and then they review each others lists when considering which of their own features to enable (or the minimum features required for one or the other to function correctly, possibly leading to an error). |
types/wlr_output_damage.c
Outdated
| @@ -101,14 +101,20 @@ void wlr_output_damage_destroy(struct wlr_output_damage *output_damage) { | |||
| free(output_damage); | |||
| } | |||
|
|
|||
| bool wlr_output_damage_make_current(struct wlr_output_damage *output_damage, | |||
| bool *needs_swap, pixman_region32_t *damage) { | |||
| bool wlr_output_damage_begin(struct wlr_output_damage *output_damage, | |||
There was a problem hiding this comment.
This name is strange since it doesn't begin anything. Maybe rename to wlr_output_damage_get?
There was a problem hiding this comment.
Yes, that's better. The function started rendering for some time but that was a bad idea.
On the drm backend it should work since it's not related to the renderer. Couldn't figure out how to get the sample to work yet.
forgot about the weston sample programs, that should make implementing and testing dmabuf possible.
I tried to keep things as general as possible, so implementing a software renderer should work with the current interface (or slight changes, not sure if one can implement a software renderer on xcb windows without knowing the visualid).
Yes. I haven't found a way yet to identify VkPhysicalDevice with a gbm_device so not sure how to do that. It may be possible over pci ids.
Changing this is possible but i don't think there is a very elegant way. The problem is that backends and renderers are too different. How should we e.g. add vulkan-specific code to the drm backend? the drm backend would have to implement a whole vulkan swapchain which doesn't seem fitting there to me. We first create a backend and then create a renderer on top of that and not the other way around. Backends would need to know much of the internals of the renderer and have access to its state (mainly for vulkan: e.g. what format does the vulkan render pass need or when does rendering to my render buffers finishes) while - as implemented at the moment - the renderer interfaces with the backends over very thin and well-defined interfaces. I can't come up with a good abstraction over wl/x11 backends and drm backend so we would need too different apis there at least. But then there are also differences for wl and x11 backends, namely that the gl renderer has to construct an additional object (wl_egl_window) for the wl backend and not for x11, the vulkan renderer doesn't have to do that. So in the end we would probably end up with per-renderer code in the backend and additionally (and least to a certain degree) backend-dependent code in the renderer. |
|
I would rather have per-backend code in the renderer than per-renderer code in the backend. |
There was a problem hiding this comment.
I'm against this as-is currently. Linear textures are very slow and limited compared to tiled textures, and the extension needed to have the ability to import and export DMA-BUFs with specific modifiers (and hence tilings) is missing, which basically means all imports and exports are undefined and working on someone's system under specific conditions.
Please, complain about this on #dri-devel, I require this extension too (VK_EXT_image_drm_format_modifier) to get VAAPI importing and exporting for my libavutil vulkan hwaccel, I've been complaining for the last 7 months but chadv and whoever else were dealing with it are no closer to getting this merged.
Once this extension is in this will need a non-trivial rewrite, hence I'm against merging something unoptimal and barely working.
|
why does this need a non-trivial rewrite for using the extension? that's one of the reasons dmabuf importing wasn't implemented at all. The "all textures are currently created with linear tiling" thing i mentioned is only for textures for shm buffers, i.e. create_texture_from_pixels and using optimal tiling there only requires some extra uploading loading since we then can't write to the images by mapping anymore. |
|
After some discussions on IRC, I've decided that it would be best to change the way the DRM buffer handling works before considering this for merging. I'm also inclined to agree with @atomnuker about waiting for the proper modifier extension to happen. At least until everything is done "properly" and is shown to work well on a wide range of hardware/drivers, I would only merge this is something listed as experimental that would need to be explicitly enabled (i.e. remove it from any auto-detection logic). I should have some time to make some DRM changes in the near future, so we'll just leave this PR on hold until then. However, I do suspect that this PR may require quite a large reworking. |
|
I'm okay with putting this on hold for now, merging this as experimental works as well but doesn't make much sense when there really is a rework needed. Can't do anything about the modifier extension at the moment but with drm buffer handling you mean the way the gbm render surfaces are implemented at the moment, right? Do you mean explicit fencing (which seems to be required to do vulkan on drm without khr_display correctly) with the drm changes or do you also plan to move the entire custom gbm_bo handling into the drm backend? What else isn't done "properly" and can be done before you consider accepting this? |
Right.
Maybe implicit fencing will work here? I'm not 100% sure.
The modifier thing is what I was mainly thinking of. I haven't poured over the PR very deeply yet, but I want to make sure everything is done how it's supposed to be done. |
Explicit fencing is required, yes. |
|
I would rather not let this pull request grow stale, full of conflicts, etc. This is 4,000 lines of code that is going to quickly rot if we leave it here until we finish refactoring elsewhere. Having this in the tree wouldn't make the other backends worse, would it? |
|
This only took 3 days |
|
hm so what if i disable it by default in meson, remove it as fallback when the gles2 renderer fails initializing (in renderer_autocreate) and maybe add a warning in meson/when it's used at runtime that it's only experimental and not ready as default renderer, is that ok for everybody? This pr obviously touches some parts other than the new renderer, like moving around the code for gles2, adding the whole wlr_render_surface thing and changing/fixing the way we set the cursor on wayland (due to blocking eglSwapBuffers) so it would be a good idea to at least make sure it does not break anything on any system for the gles2 renderer (could only test it on two systems). Other than that a large part of this pr are the interface changes that allow non-gles2 renderers anyways and yes, having this merged soon would relieve me of some work making sure it stays up to date. I (and whoever else may be interested) could then start sending smaller prs that add the missing features. |
|
+1. Make sure the |
|
Regarding #1174, it fixes that for the cursor surface only by tracking frame callbacks. |
|
I guess this comment got left behind, but I want to rethink this:
More people will write custom renderers than custom backends, let's optimize for that use-case. |
|
@nyorain It seems that you changed the meaning of |
Yeah, that's how it's handled at the moment. If every backend had renderer specific code, writing custom renderers would not be possible without touching the backends. At the moment it's impossible to create a completely new backend (i.e. using new primitives; unable to use an existent render surface implementation) without touching the renderer but that shouldn't happen too often and we can't get around this. |
|
@ammen99 thanks for the notice, this should be fixed now. Since the vulkan renderer obviously doesn't need a wlr_egl object and wlroots isn't that egl fixed anymore it's slightly different than before. Your custom renderer create func could look like this: struct wlr_renderer *create_custom_gles_renderer(struct wlr_backend *backend) {
EGLenum platform;
void *remote_display;
const EGLint *config_attribs;
EGLint visualid;
if (!wlr_backend_egl_params(backend, &platform, &remote_display,
&config_attribs, &visualid)) {
wlr_log(WLR_ERROR, "Backend does not support egl");
return NULL;
}
// create changed_config_attribs
// process the given config attribs: change them/add new ones
// for adding new ones you have to copy them first into a new buffer
...
return wlr_gles2_renderer_create_for_egl(backend, platform, remote_display,
changed_config_attribs, visualid);
}As before, the compositor can still have the final word about the egl config attribs used (but should query the backends requirements/default attribs). By default the backends egl params are used without any processing (wlr_gles2_renderer_create). |
|
Can you enumerate the touchpoints between the renderers and the backends? |
|
It boils down to
Renderers are still created in the backend as before via a given renderer create callback. Instead of egl specific arguments this callback gets passed the backend, the create_func or renderer itself can then retrieve properties from the backend, e.g. these egl arguments via For every output the backend creates, it will ask the renderer to create a |
|
I don't understand how the vulkan renderer gets away without having an implementation of |
|
|
- fix invisible cursors on wayland backend (both renderers) - split wlr_vulkan into wlr_vk_instance and wlr_vk_device. This will allow to have multiple devices for the same instance later on - query format properties - change how extensions are handled for devices/instances to allow creating it with custom extensions - correctly query present queue support (add to backend interface) - correctly allocate descriptor pools. No longer limited to a fixed number of 500 descriptors Aside from general fixes, those fixes work towards allowing a more functional vk_display backend.
|
PR dead? |
|
Nope, we're just waiting for progress on #1355 before working again on this one. The renderer redesign should make a lot of things simpler. |
|
also waiting for mesa to implement VK_EXT_image_drm_format_modifier. There was at least an implementation for anv (intel) before the extension was even released but that hasn't made it upstream yet afaik. |
|
@nyorain @emersion can you share some insight with respect to I've crawled the web for this a few times since the holidays and I found the initial intel branch / patchset but after this became part of the official vulkan spec I don't see much more with respect needing implementation. Surely this isn't all that is needed to implement the extension. I'm willing to do the MESA work with respect to |
|
@ascent12 I was tracking that PR for awhile, I was going to comment on the PR asking the status but I now see details on the issue that were added yesterday, thank you. |
|
Tracking stuff related to mesa from the outside is a little difficult but I came across this today when looking into this again https://github.com/st3r4g/swvkc There is a link to this PR https://gitlab.freedesktop.org/mesa/mesa/merge_requests/515 This seems like a good reference for someone just starting out with contributing so I'll try pick up a Radeon RX 5500 when it comes out ( hopefully early December ) and I will implement and test a |
|
On the wlroots side it seems that switching to GBM is the next step for vulkan support. and there has been code pulled out from the renderer redesign for gbm here. This looks like a massive undertaking. There also seems to be some bits related to Though I'm not quite sure if this is the same lagacy drm that @ascent12 references in the above PR. But also this MR seems to imply that it is optional now ? Sorry if this is noisy btw, I'm trying to gather and expose context here. |
|
If this get's ever finished, would it be technically possible to use the new unofficial https://github.com/Yours3lf/rpi-vk-driver Vulkan driver to run a snappy VM on an ancient RPi? That be super awesome. |
|
In theory, yes. In practice, this renderer is not too useful unless Btw, when |
|
It probably does not make sense to continue investing in this until the renderer redesign settles down. There are still a lot of moving parts and most of this code would have to be rewritten or overhauled. |
When that's done, if you're interested to work on this, you could try adding a Vulkan renderer to glider. Glider is the test compositor I've been using, and it's close to what renderer v6 will look like. The renderer basically needs to import GBM buffers and render to them. I've written about renderer v6 in my last status update: https://emersion.fr/blog/2020/status-update-19/ Example of |
|
I think we're pretty close to being able to have a Vulkan renderer now that the |
|
Alright, I'll look into it but this will take a while. I don't think it's useful anyways before a mesa driver supports |
|
|
|
I noticed the turnip driver casually enabled |
|
Superseded by #2771. |
Finally had some time to put this together.
Current state
all examples (and rootston) work, screenshotting as well
the vulkan renderer. Don't know how to test dmabuf and to get mesa egl/vulkan
clients to work the vulkan renderer would have to implement
wl_drm at the moment; i'm not sure how to do that correctly.
Since it was was mentioned that this might not be needed in the future
at all, leaving such buffers for another pr.
The dmabuf implementation would also profit from the mentioned (and
hopefully soon releasing) vulkan dmabuf modifiers extension.
See @fooishbar's comments ([1], [2])
or changes to wlr interfaces. Those are usually explicitly marked in the code.
You can try it out by starting programs with
WLR_RENDERER=vulkan ....Note that besides the vulkan dependency it currently requires glslangValidator (arch package glslang) to compile the shaders from glsl into spirv c headers. This dependency is only required at build time and only to recompile the shaders; we could also include the
compiled shader headers in the repo, removing this dependency.
Interface changes
As expected, this has to touch wlr interfaces (only unstable ones, thanks for waiting with stabilizing them). The main changes are in renderer, output and backend interfaces.
This also introduces a new abstraction wlr_render_surface, generally is the design inspired and close to the ideas from renderer redesign v5. This allowed me to remove all direct egl and gl usages from backends. egl initialization is still done by the backend and the render_surface creation interface is explicit (using wl/xcb/gbm primitives) so that we don't have any
wlr_backend_is_xchains in the renderers. But otherwise renderers must be aware of all backends they support and do so explicitly. Therefore, this moves some codefrom the backends (mainly drm,
drm/renderer.c) to the vulkan/gles2 renderers.In the renderer redesign issue, @SirCmpwn mentioned that this approach makes it hard to write new backends or renderers since for a new backend the render interface must be
extended and support implemented for existent renderers (if possible) and for a new renderer one has to explicitly implement support for the supported backends.
The thing with this is: there is no way without that difficulty.
When creating a new renderer, it has to know how to render on the backends
its supports. This cannot be done in a general way since e.g. vulkan has explicit methods for creating surfaces and egl on wayland requires creation of a wayland-egl-window and rendering on the drm/gbm backend requires a lot of completely different backend AND renderer dependent primitives.
And when creating a new backend, it is either possible to use the primitives
the renderer interface already uses (e.g. gbm) or the render interface
just has to be extended to be able to pass the new primitives from backend
to renderer.
We could (what i did first, see some older commits) create
a more general
create_render_surfaceinterface, where avoid*handle is passed to the renderer. But this requires that rendererand backend both know what the handle should be (and casting
it into structures containing all information from the backend, see
e.g. the gbm case where a single handle isn't enough information) and to
if (backend_is_x(backend)) ...chains in the renderer which seemed more limited and generally not like a good idea.This digs somewhat into the current drm backend implementation to make this work, so it should be tested on as many setups as possible and definitely receive the mandatory @ascent12 review. I realize that this pr is rather large and may require some changes before everyone is happy merging it so please give me the chance to get it into that state with as much reviews and comments as possible. Most of the later commit messages are also rather detailed and outline design decisions as problems came up.
I'm not sure how many of you are experienced with vulkan, but some reviews of the vulkan usage would be appreciated as well (although i heavily relied on the debug layers while developing). Otherwise, see the vulkan spec for information.
If you (categorically/at the moment) reject such a pr or a vulkan renderer altogether, please also let me know up front. Although i spent quite some time on this i did so without communicating/asking and mainly because it was a really interesting task. So i'd understand if you chose not to merge this. If you do, i will gladly maintain this renderer in the future. Will also be as often as possible on the irc the next time, just ping me for questions and discussion.
This also fixes some non-vulkan issues though i found on the way so you at least probably
want the wlr_output_damage_begin fix and the improved wayland output cursor setting.
Notes
A list of notes/decisions i wrote down while writing the renderer as well as ideas for future improvements and optimizations.
interface for this. It is only needed in one place at the moment iirc:
in the drm backend when setting the cursor (there it is currently
first rendered and then read from the dummy render surface). Since it's called every time
the cursor is changed when using the drm
backend, this should be a rather important optimization (and
i guess we can implement it more efficiently in gles2 as well using a dummy framebuffer
we read the texture from instead of rendering the texture first?)
that can be improved with explicit fencing (at least for atomic modesetting)
and releasing on pageflip again (see drm backend comment)
but since this requires more changes to the drm backend/wlr interface and is mainly a
optimization thing, it is left for later
vk_khr_incremental_present is currently not used (optimization)implemented, buffer age for swapchain surfaces fixed by pre-acquiring an imageall textures use currently linear tiling and are created on hoststaging logic for optimal shm textures now implementedvisible memory. This has a rather large performance impact and
can be improved with some uploading logic
can textures ever be resized? if so this is currently not implementednope, not allowedon the vulkan renderer
A better approach would be to choose them by what they support but
also make this configurable. Multi gpu support is currently not
implemented in any way and due to the way we interface with the
drm backend, the vulkan renderer might not work at all when using
the drm backend with multiple gpus.
in vulkan gbm render surfaces, we currently create gbm_bo's and import them into vulkan.no, wouldn't be possible to specify gbm_use_scanout this way; can be improved with modifiers extensionwe could also create vulkan images, export them as dmabufs and import them as gbm_bo's. Might have an impact on performance/portability (?)
since the currently packaged mesa version does not implement vk_khr_displayStarted a VkDisplayKHR backend implementation here on top of this pr, roughly working with mesa 18.2. To not further blow this pr up, will submit it as a new one afterwards.this does not implement such a backend. That is a somewhat independent
topic anyways. Would like to implement it though in the future (the next
mesa version implements it iirc).
the vulkan renderer currently implements a custom swapchain for gbm render surfaces.nope, gbm_surface does not work with vulkanIt might be possible to use gbm_surface there as well but since the
documentation (in the gbm implementation, not the header) explicitly mentions
eglSwapBuffers and when it must be called, i'm not sure and haven't tried it.
want to support, let me know. The same can be achieved (with some work and additional
extension loading) with vulkan 1.0.