EGLStream DRM Backend Initial Implementation
ClosedPublic

Authored by ekurzinger on Jan 27 2019, 9:44 PM.

Details

Summary

This is the initial implementation of a DRM backend based on the EGLDevice, EGLOutput, and EGLStream extensions, supporting NVIDIA graphics hardware using their proprietary driver. The new backend will be used if the environment variable KWIN_DRM_USE_EGL_STREAMS is set. On initialization, it will attempt to create an EGLDevice based on the DRM device currently in use and create EGLOutputs and EGLStreams for any attached displays. These are used to control presentation of the final composited frame. Additionally, it will register the wl_eglstream_controller Wayland interface so that native EGL windows created by clients can be attached to an EGLStream allowing buffer contents to be shared with the compositor as a GL texture.

At this time there are two known bugs in the NVIDIA driver's EGL implementation affecting desktop functionality. The first can result in tooltip windows drawn by plasmashell to contain incorrect contents. The second prevents KWayland from being able to query the format of EGLStream-backed buffers which interferes with the blur effect. Fixes for both of these are currently in development and should appear in an upcoming NVIDIA driver release.

Additionally, hardware cursors are currently not supported with this backend. Enabling them causes the desktop to intermittently hang for several seconds. This is also likely a bug in the NVIDIA DRM-KMS implementation but the root cause is still under investigation.

Test Plan

On a system with an NVIDIA graphics card running a recent release of their proprietary driver

  • Ensure the nvidia_drm kernel module is loaded with the option "modeset=1" ("# cat /sys/module/nvidia_drm/parameters/modeset" should print "Y")
  • Ensure EGL external platform support is installed https://github.com/NVIDIA/eglexternalplatform
  • Ensure KWin was build with the CMake option KWIN_BUILD_EGL_STREAM_BACKEND=ON (this is the default)
  • Start a plasma wayland session with the environment variable KWIN_DRM_USE_EGL_STREAMS set
  • Ensure output from KWin OpenGL initialization indicates the NVIDIA EGL driver is in use (as opposed to Mesa / llvmpipe).
  • Desktop should be fully functional and perform smoothly.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jan 28 2019, 11:40 AM
hein added a comment.Jan 28 2019, 2:39 PM

If I blacklist the kscreen2 kded module, I can get the session to start (otherwise it crashes during the splash). However, all surfaces (e.g. the panel or krunner) don't react to mouse clicks and seem frozen-ish.

BTW, while playing around with this I noticed that kwin_wayland doesn't work in windowed mode on X when run on nVidia EGL, due to the driver not implementing GL_OES_EGL_image.

ekurzinger added a comment.EditedJan 29 2019, 4:32 PM
In D18570#401368, @hein wrote:

If I blacklist the kscreen2 kded module, I can get the session to start (otherwise it crashes during the splash). However, all surfaces (e.g. the panel or krunner) don't react to mouse clicks and seem frozen-ish.

BTW, while playing around with this I noticed that kwin_wayland doesn't work in windowed mode on X when run on nVidia EGL, due to the driver not implementing GL_OES_EGL_image.

Yes, I had encountered that as well. It should be resolved by https://phabricator.kde.org/D18307

[edit]
Just commit the aforementioned change, so the freezing ought to be fixed now.

ekurzinger added inline comments.Jan 29 2019, 9:14 PM
plugins/platforms/drm/CMakeLists.txt
29

wayland-server is needed in order to register the wl_eglstream_controller interface. The NVIDIA driver uses this when an application creates a native EGL window to pass the corresponding EGLStream to the compositor

dl is needed because we dynamically load libnvidia-egl-wayland.so since that library would only be present on systems with the NVIDIA driver installed, so we would only want to require it if using the EGLStream backend.

plugins/platforms/drm/drm_backend.cpp
751

If Mesa is available, creating an EglGbmBackend should always succeed, I believe. On systems running the NVIDIA driver it will just fall back to using llvmpipe-based software rendering (of course, with abysmal performance).

We could attempt to create the EglStreamBackend first, which should fail on any non-NVIDIA systems, and then fall back to GBM (and then QPainter). Alternatively, I suppose we would need some way to detect if the NVIDIA driver is present. There are probably a few ways to do this but they might be a little hacky.

And re: zzag I can totally make the EGLStream stuff compile-time optional if that would be preferred, I'll work on an amended version of the change and get it up shortly.

ekurzinger updated this revision to Diff 50516.Jan 29 2019, 9:39 PM

Added context to the diff and fixed style issues, (thanks the feedback)

ekurzinger marked 6 inline comments as done.Jan 29 2019, 9:40 PM
hein added a comment.Jan 30 2019, 10:26 AM

D18307 seems to have done the trick in more ways than one. I no longer need to blacklist the kscreen2 kded module and can successfully log in. This is written in Plasma/Wayland on nVidia drivers, congrats! There's still some wonkyness that may be from who knows where - the splash screen no longer renders - but so far, so good.

ekurzinger updated this revision to Diff 50573.Jan 30 2019, 6:14 PM
cfeck added a subscriber: cfeck.Jan 31 2019, 3:21 AM
cfeck added inline comments.
platformsupport/scenes/opengl/abstract_egl_backend.cpp
130 ↗(On Diff #50573)

Does it really need to be declared GLboolean? You are using the logical && operator anyway.

plugins/platforms/drm/drm_backend.cpp
642

A was searching for the definition of this function, because it is named wrongly (should be setSoftwareCursor), but it seems to be a mistake in the base class. If it's private API, it might be worth to rename it.

705

For implicitely shared Qt classes, it might be dangerous to use a reference, because that won't increase the shared-ref counter. Please just omit the &.

710

If for (const &output : m_outputs) also works here (and at other places), you could improve readability.

plugins/platforms/drm/egl_stream_backend.cpp
189

ext is only a byte array. To add it to a string, Qt needs to know in which encoding the byte array is. Please use ... + QLatin1String(ext).

325

Space before :

439

Not sure here, but I think these should also not be reference variables.

476

Space

romangg added inline comments.Jan 31 2019, 7:02 AM
plugins/platforms/drm/drm_backend.cpp
642

Not in this diff though. Same holds for other suggestions below in this file, which are not directly related to this diff.

fredrik added inline comments.
plugins/platforms/drm/CMakeLists.txt
29

You can probably avoid linking to dl by using QLibrary:

https://doc.qt.io/qt-5/qlibrary.html

ekurzinger added inline comments.Jan 31 2019, 7:44 PM
plugins/platforms/drm/drm_backend.cpp
642

yeah, while I agree with these suggestions I think it would be best to limit this commit to code related to the new backend as opposed to general cleanup. I'll address the style issues you noticed in egl_stream_backend.cpp, though (thanks for catching those).

plugins/platforms/drm/egl_stream_backend.cpp
189

I believe QString::operator+ does this implicitly

ekurzinger updated this revision to Diff 50619.Jan 31 2019, 9:03 PM
ekurzinger edited the summary of this revision. (Show Details)
ekurzinger edited the test plan for this revision. (Show Details)

Replaced dlopen / dlsym with QLibrary::resolve to avoid having to link against dl
Fixed a few more style issues
Added KWIN_BUILD_EGL_STREAM_BACKEND CMake option to control whether the new backend is built (defaults to ON).
Changed the name of the environment variable to KWIN_DRM_USE_EGL_STREAMS (instead of EGLDEVICE) to be a bit more consistent.

ekurzinger marked 3 inline comments as done.Jan 31 2019, 9:06 PM
ekurzinger added inline comments.
plugins/platforms/drm/egl_stream_backend.cpp
439

They're const so I don't think it really makes much of a difference. If anything references save a couple of bytes and avoid a struct copy.

ekurzinger updated this revision to Diff 51224.Feb 8 2019, 10:53 PM

Use a KWayland wrapper around wayland-eglstream-controller instead of binding the interface from KWin. This avoids having to link against Wayland libraries. Depends on revision https://phabricator.kde.org/D18824 implementing the required KWayland class.

romangg requested changes to this revision.Feb 9 2019, 8:22 PM
romangg added inline comments.
platformsupport/scenes/opengl/abstract_egl_backend.cpp
130 ↗(On Diff #51224)

Put this function in EglStreamBackend only. On another note, do we need it at all? It's called only once.

plugins/platforms/drm/drm_backend.cpp
762

else keyword and curly braces are unnecessary.

803–807

In case of HAVE_EGL_STREAMS not if m_useEglStreams is false.

plugins/platforms/drm/drm_output.h
27

Remove. Forward declaration is enough.

plugins/platforms/drm/egl_stream_backend.h
88

Instead of taking the wl_resource pointer as key, use the KWayland::Server::SurfaceInterface pointer.

This revision now requires changes to proceed.Feb 9 2019, 8:22 PM

First off: a driver not supporting Atomic Mode Setting is nothing modern. Does Nvidia has a plan to improve in this regard?

I'm really not happy, that the DrmOutput::present is not run and we have to export these internals from DrmOutput to the EglStreamBackend, which replaces the flip with an EGL function call. Why does the Nvidia driver not integrate with the libdrm API in regards to page flipping? It should be possible to at least offer a wrapper around it or not?

Why are there multiple texture copies necessary in EglStreamTexture and does this not have a performance impact?

ognarb added a subscriber: ognarb.Feb 11 2019, 6:00 PM
davidedmundson added inline comments.Feb 11 2019, 6:12 PM
plugins/platforms/drm/drm_backend.cpp
323

I know this isn't related to your patch, and I don't want to take it off-topic, but when I run on my nvidia machine I get a null value from drmResources.

I get the same failure with a simple self-contained libDRM test.

I have a valid FD from logind.
/dev/card0 exists, drmGetVersion works fine.

I'm running Nvidia driver version 415.27

Any ideas?

plugins/platforms/drm/drm_backend.cpp
323

Hmm, interesting. Could you confirm that the nvidia_drm module is loaded with the option "modeset=1"? We currently don't enable the DRM driver by default (although that will probably change eventually).

# cat /sys/module/nvidia_drm/parameters/modeset

should print "Y"

That was it, thank you!

Session mostly worked great.

My blur's are missing, and I get a crash when the internal QPA creates a new GL surface over internal QOpenGLFramebufferObjects.
You'll see when you press alt+tab and it's set to show a dialog (https://phabricator.kde.org/P313)

I'll try and help on them

Oh yeah, I've seen similar backtraces to that as well, actually. I think it's because QT is trying to share between an OpenGL context and an OpenGLES context. I don't think it's related to the compositor, though. It also occurs on GNOME and the weston eglstream port. I'm not sure if it's a QT bug or NVIDIA driver bug but I'll look into it a bit more as well.

ekurzinger updated this revision to Diff 51710.Feb 14 2019, 8:11 PM
ekurzinger edited the test plan for this revision. (Show Details)
  • Fixed style issues in drm_backend.cpp / drm_output.h
  • Switched surface -> texture map to use SurfaceInterface pointers instead of wl_resource pointers
  • Moved eglQueryWaylandBufferWL to egl_stream_backend.cpp
  • Added support for receiving stream attributes from EglStreamControllerInterface (currently unused by the driver, but good for future proofness).
ekurzinger marked 4 inline comments as done.Feb 14 2019, 8:12 PM

First off: a driver not supporting Atomic Mode Setting is nothing modern. Does Nvidia has a plan to improve in this regard?

I'm really not happy, that the DrmOutput::present is not run and we have to export these internals from DrmOutput to the EglStreamBackend, which replaces the flip with an EGL function call. Why does the Nvidia driver not integrate with the libdrm API in regards to page flipping? It should be possible to at least offer a wrapper around it or not?

Well, our DRM-KMS driver technically does support atomic modesetting, however currently we only allow the presentation of GPU-accessible buffers through the EGLDevice / EGLOutput extensions. In fact, these use atomic modesetting capabilities in their implementation. As far as I know this will remain the case until work on the Unix device memory allocation library is complete and while this is still on the table it's a ways off yet. The new backend really just uses DrmOutputs to track whether a page flip is pending. If we don't want to expose these internals, an alternative might be to track this separately from DrmOutput::m_pageFlipPending and maybe install a different page flip handler when using EGL streams that would just clear this flag (and re-start the compositor). I'll see if I can get something like that thrown together, it might be a bit cleaner, actually.

Why are there multiple texture copies necessary in EglStreamTexture and does this not have a performance impact?

When a texture is bound as the consumer of an EGL stream, it needs to be a GL_TEXTURE_EXTERNAL_OES. However, most of the core compositing code assumes window contents are in a regular GL_TEXTURE_2D. These two types of textures require different types of samplers in shaders, ect., so I think modifying KWin to composite directly from the stream-bound texture would make the change quite a bit more invasive. From some measurements I've taken, the overhead from the additional blit is in the neighbourhood of 0.15 ms (although I suppose this will depend on hardware), so I don't think it involves hugely significant performance hit. It's a vidmem -> vidmem transfer so at least it can take advantage of hardware acceleration. Having said that, I'm not too happy about it either. It's definitely a compromise.

hein added a comment.Feb 17 2019, 4:39 AM

The patch currently doesn't apply cleanly against master anymore, mind rebasing on the next update?

ekurzinger updated this revision to Diff 52113.Feb 19 2019, 8:25 PM

Updated diff against HEAD

First off: a driver not supporting Atomic Mode Setting is nothing modern. Does Nvidia has a plan to improve in this regard?

I'm really not happy, that the DrmOutput::present is not run and we have to export these internals from DrmOutput to the EglStreamBackend, which replaces the flip with an EGL function call. Why does the Nvidia driver not integrate with the libdrm API in regards to page flipping? It should be possible to at least offer a wrapper around it or not?

Well, our DRM-KMS driver technically does support atomic modesetting, however currently we only allow the presentation of GPU-accessible buffers through the EGLDevice / EGLOutput extensions. In fact, these use atomic modesetting capabilities in their implementation. As far as I know this will remain the case until work on the Unix device memory allocation library is complete and while this is still on the table it's a ways off yet. The new backend really just uses DrmOutputs to track whether a page flip is pending. If we don't want to expose these internals, an alternative might be to track this separately from DrmOutput::m_pageFlipPending and maybe install a different page flip handler when using EGL streams that would just clear this flag (and re-start the compositor). I'll see if I can get something like that thrown together, it might be a bit cleaner, actually.

In this case it seems better to me to not write the EGLStreams support directly into the DRM Backend at all, but create a new EGLStream Backend on same level as DRM Backend. This would separate the code better. When at some point a generic Unix device memory allocation library comes along we can then write this one into the DRM backend.

What is actually needed from the DRM Backend? If currently the DrmOutputs are only used for tracking page flips, this could be done in some minimal EglStreamBackend and EglStreamOutput classes inheriting Platform and AbstractOutput. Or maybe EGLStreams provides a signal for that as well through EGL extension such that the DRM device does not even have to be opened directly by KWin as done in DrmBackend::openDrm?

There is some other functionality in DrmBackend and DrmOutput, that it would need to replicate, or better directly move it to a higher abstraction level, since it does belong here and not in the DRM backend code (for example DrmBackend::m_dpmsFilter of type DpmsInputEventFilter class, large parts of DrmOutput::transform and DrmOutput::automaticRotation).

Other aspect which is important to me long-term: is multi-GPU support of different vendors possible with the current patches or a separate backend? Current patches don't seem to allow that easily to add later on. Depending on DRM access in a separate backend it might be simpler to do later.

I'm talking about for example using the APU outputs on mainboard simultaneously with a discrete GPU.

In this case it seems better to me to not write the EGLStreams support directly into the DRM Backend at all, but create a new EGLStream Backend on same level as DRM Backend. Th

That sounds pointless.
There's less of a difference between EGL streams and GBM than there is between GBM and QPainter.
It would create a huge architectural problem given we create a platform before the scene so you'd at best have to duplicate the qpainter side.


In terms of this patch, I think it's pretty much good to go without any changes - and if there are any, I think that can be our responsibility rather than making the nvidia guy do it after they've done the brunt research work.

romangg added a comment.EditedFeb 20 2019, 12:49 AM

In this case it seems better to me to not write the EGLStreams support directly into the DRM Backend at all, but create a new EGLStream Backend on same level as DRM Backend. Th

That sounds pointless.
There's less of a difference between EGL streams and GBM than there is between GBM and QPainter.

GBM and QPainter both use the DrmBackend/DrmOutput methods as they are meant to be used including all atomic mode setting classes while EGLStream does everything internally bypassing the Drm methods. I'm not sure how you come to the conclusion EGLStreams and GBM would be more similar.

It would create a huge architectural problem given we create a platform before the scene so you'd at best have to duplicate the qpainter side.

That's a good counter-argument though to having a separate backend. But everything in the DrmQPainterBackend besides the DrmDumbBuffer and DrmOutput pointers is independent of Drm code. But still the complexity grows, and therefore it might be better to not go down this road. Or maybe revisit it when multi-GPU is actually closer on the roadmap.

Besides having it as a separate backend would be somewhat clunky looking at the backend auto selection code. There is currently an env var used for activating the EGLStream backend, which could be used in the auto backend selection as well, but in the end we maybe want to get rid of that variable. Is there already a plan for that by the way?

Also I now notice again, that EglStreamerBackend class does also use the DrmOutput at one point to access its DrmCrtc and DrmPlane objects for getting the id of these kernel objects and then uses them in an EGL call. While this is not really much usage it makes it more reasonable again to have EGLStreams in the Drm backend.

In terms of this patch, I think it's pretty much good to go without any changes - and if there are any, I think that can be our responsibility rather than making the nvidia guy do it after they've done the brunt research work.

I don't agree. We have no more responsibility to this patch than Nvidia. And I was not talking in my review comments about superficial stuff, but some base assumptions in this initial implementation. Getting these right is important. Your argument above was good in regards to not having a separate backend. But that does not change the problem I have with this solution here.

I don't want the Drm backend get diluted by allowing hooks to be inserted, which break up well defined functional chains and bypass most important and defining functionality of the Drm backend. I'm open for discussing this more and also in helping to find a more uniform solution. And if there are no solutions in reasonable time to be found I'm open for making an exception under the current circumstances since at least we don't add lots of alternative code for the bypassed present function. That's in the driver.

And I was not talking in my review comments about superficial stuff,

We're just talking about the use of DrmOutput::present, right?
We could make a fake DrmBuffer instance round that doesn't do anything - and add some property in DrmBuffer to indicate whether we perform the relevant DRM calls or not. Would you prefer that?

@erik
One difference missing is that when we change modes we delay it till the DrmOutput::presentLegacy and then call drmModeSetCrtc with the pending buffer handle.
Right now as far as I can see any type of mode switching is missing.

I found the source of one of my issues.

src/server/buffer_interface.cpp:152
if (eglQueryWaylandBufferWL(eglDisplay, buffer, EGL_TEXTURE_FORMAT, &format)) {
return false

The size attributes using the same method were extracted correctly and from what I can tell this function should still be valid on EGLSreams.

Hardcoding a hack fixed my panel blur and also resolved some update issues it was having.

Is that in the KWayland master branch? In the file I'm looking at buffer_interface.cpp:152 is in the middle of a switch statement in the constructor.

Sorry I had extra debug that offset the line numbers

We don't enter the if statement on line 146 when using master + https://phabricator.kde.org/D18824

And I was not talking in my review comments about superficial stuff,

We're just talking about the use of DrmOutput::present, right?
We could make a fake DrmBuffer instance round that doesn't do anything - and add some property in DrmBuffer to indicate whether we perform the relevant DRM calls or not. Would you prefer that?

@erik
One difference missing is that when we change modes we delay it till the DrmOutput::presentLegacy and then call drmModeSetCrtc with the pending buffer handle.
Right now as far as I can see any type of mode switching is missing.

Yeah, testing a bit more this definitely looks to be a concern that needs to be addressed. For instance setting a transformation for
an output results in pretty major corruption. I think your suggestion sounds like a good idea and I've been working on trying to get
something like it working. I think this will also address Roman's (valid) concerns about the awkward way we're currently bypassing
DrmOutput::present and is probably the best way forward. I'll try to get an amended version of the patch up soon.

I found the source of one of my issues.

src/server/buffer_interface.cpp:152
if (eglQueryWaylandBufferWL(eglDisplay, buffer, EGL_TEXTURE_FORMAT, &format)) {
return false

The size attributes using the same method were extracted correctly and from what I can tell this function should still be valid on EGLSreams.

Hardcoding a hack fixed my panel blur and also resolved some update issues it was having.

So, it looks like that attribute was added to the spec a bit later but support for querying it never got implemented in our driver :(
Fortunately, it looks like adding this support won't be too difficult on our end. Unfortunately, doing so appears to break GNOME's
EGLStream support which seems to depend on this query failing to determine if a buffer is an EGLStream-backed one. Fortunately
again, though, this should be fixable on their end with a very minor change so I've reached our to them for their thoughts.

mati865 added a subscriber: mati865.Mar 6 2019, 7:31 PM
abalaji added a subscriber: abalaji.Mar 7 2019, 3:54 PM
eszlari added a subscriber: eszlari.Mar 7 2019, 8:42 PM
ekurzinger updated this revision to Diff 53757.Mar 12 2019, 8:22 PM
ekurzinger edited the summary of this revision. (Show Details)

Sorry for the delay, but the new backend now integrates into the atomic modesetting path of DrmOutput::present eliminating the need to expose the internal pageFlipPending flag and is a bit cleaner in general, I think. It leverages the existing code for modesets (which are now actually working correctly), however page flips are still performed through EGL. This necessitated adding an early return from DrmOutput::presentAtomically as well as not registering the flip event when using the EGLStream backend. I tried to make this as minimally-invasive as I could, and since there's already a fair amount of coupling between DrmOutput and DrmBackend hopefully a little bit more isn't the end of the world. Modesets use an empty dumb buffer, which is the same strategy that Mutter and Weston's EGLStream backends employ.

I thought this might fix hardware cursors, but they still appear to cause some strange issues so I left them disabled for now. I suspect this might be a bug in the NVIDIA driver. Additionally, I've changed the EGLOutput stream to operate in mailbox mode which eliminates some potential problematic interactions with DPMS.

Anyway, let me know what you think.

romangg accepted this revision.Mar 12 2019, 8:47 PM

Without going through it in detail the overall structure looks really nice now. I'm really happy with the integration in the new version, in particular that our normal AMS code path is used for mode switching. Wait for another +1 by someone who can test it on a live system. But from my side it's good to go.

plugins/platforms/drm/egl_stream_backend.cpp
473

Check if the condition is not fulfilled and if this is the case return early.

This revision is now accepted and ready to land.Mar 12 2019, 8:47 PM
ekurzinger marked an inline comment as done.

Code still looks good. +1 from me

I can't personally use my Nvidia machine till the 27th March.
I would appreciate it if some other reader here could.

What's our plan for the EGL_TEXTURE_FORMAT issue? Simply to implicitly depend on the new driver?

Last time, I also had some issues with Qt clients, particularly system settings. This was with the old driver and with the Qt 5.12 branch. Could you verify if it behaves for you?

What's our plan for the EGL_TEXTURE_FORMAT issue? Simply to implicitly depend on the new driver?

I'm hoping this bug, along with the tool-tip issue, might be considered minor enough to not warrant holding up the show until we can get fixes out (which should be soon). The desktop is still essentially usable with them present, right? And the backend already depends on a fairly recent version of the driver with support for all of the required EGL stream features (some of which are fairly new).

Last time, I also had some issues with Qt clients, particularly system settings. This was with the old driver and with the Qt 5.12 branch. Could you verify if it behaves for you?

I had noticed these as well, I believe the problem was that the previous version of the backend didn't check if a buffer's size ect. had changed in EglStreamTexture::updateTexture, but apparently in some cases it is necessary to do so. Anyway, these should be addressed now. System Settings does appear to be rendering correctly with this version of the patch.

The desktop is still essentially usable with them present, right?

If we change kwayland to default to RGBA if the test fails, it's usable.

My blur's are missing, and I get a crash when the internal QPA creates a new GL surface over internal QOpenGLFramebufferObjects.
You'll see when you press alt+tab and it's set to show a dialog (https://phabricator.kde.org/P313)

Ah, so it looks like the Alt + Tab issue is due to the fact that AbstractPlatformContext passes EGL_SURFACE_TYPE == EGL_WINDOW_BIT to eglChooseConfig, however EGL_PLATFORM_DEVICE_EXT doesn't actually support creating windows. I'm not 100% clear on the role the QPA code is playing, but does it actually need that capability, though? If I change the surface type to EGL_PBUFFER_BIT I no longer get the crash and the window switcher sidebar appears to render correctly.

davidedmundson accepted this revision.Apr 10 2019, 11:15 PM

There might be some more issues to fix as we go along, but from a technical POV this is fine - lets get this in and we can iterate in-tree.

Awesome, just waiting on a final sign-off for https://phabricator.kde.org/D18824 and I'll get both checked in.

This revision was automatically updated to reflect the committed changes.