WIP [wayland] make automatic backend selection truly automatic
Needs ReviewPublic

Authored by alexeymin on Jul 12 2019, 12:39 PM.

Details

Reviewers
romangg
Group Reviewers
KWin
Summary

Choose backend automatically, based on actually available
plugins at runtime, independently of kwin compile options.
Don't limit command line options too.

Test Plan

Compile kwin without some of backends. Then
Compile kwin with that backend separately, manually
copy the appropriate plugin into plugins directory.
Verify that kwin_wayland --help lists that option,
and running kwin_wayland without arguments, with proper
environment automatically selects proper backend plugin.

Or, kompile kwin as usual with all backends, install it,
then manually delete some plugins.

Diff Detail

Repository
R108 KWin
Branch
alexeymin/rework-wayland-backends-plugin-system
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18847
Build 18865: arc lint + arc unit
alexeymin created this revision.Jul 12 2019, 12:39 PM
Restricted Application added a project: KWin. · View Herald TranscriptJul 12 2019, 12:39 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
alexeymin requested review of this revision.Jul 12 2019, 12:39 PM
alexeymin edited the test plan for this revision. (Show Details)Jul 12 2019, 12:57 PM

Is there a real world situation where KWin is getting compiled twice with different settings? When adding the ifdefs this was based on feedback from distributions

zzag edited reviewers, added: KWin; removed: zzag.Jul 13 2019, 10:47 AM
alexeymin retitled this revision from wayland: make automatic backend selection truly automatic to WIP [wayland] make automatic backend selection truly automatic.Jul 16 2019, 8:18 PM

@graesslin I'm not sure about compiling twice with different settings, but there is most definitely a use-case for this patch: mobile distros.

For example postmarketOS (where Alexey and I are from) we tend to run KWin on either the fbdev or DRM backend. However, since last month we also have our first libhybris devices, which use the hwcomposer plugin. The problem with libhybris is that it's unstable, can change a lot, and there are multiple versions to be used depending on the device and Android it comes from.

We don't know all of this at compile time, and we'll never do. How it currently works means we do have to compile it multiple times with different settings for all the variations of libhybris we have, and package them properly so they can't be installed along-side each other as they'll all provide the same files, etc. I think you get my point.

This patch would mean we can just build a single KWin package, build all the different libhybris variants there are, install the correct one with the device, and the proper hwcomposer backend would be used.

I'm not convinced that removing the ifdef is the solution to the problem. What you basically want is being able to do an out-of-tree build of a backend. Ideally you would want to install all the variants next to each other and be able to pick the correct one.

So what about the following idea: The plugins provide the command line arguments themselves (e.g. a static method that's queried) and specify the checks themselves. Thus we would externalize the resolution to the plugin.

alexeymin updated this revision to Diff 69837.Sat, Nov 16, 12:32 PM
  • Link kwin_wayland with Wayland::Client
  • Platform: add canLoad() function and implement in all backends
  • Add autoLoadPriority to wayland backends plugins JSON metadata
  • main_wayland: sort loaded plugins by autoLoadPriority
  • main_wayland: automatic backend selection based on their priority
alexeymin updated this revision to Diff 69838.Sat, Nov 16, 12:42 PM
  • remove unwanted change in translated string

not convinced that removing the ifdef is the solution

It is, because now availability and presence of command-line options is decided at KWin compile time, and it should be decided at run time based on the actual list of available plugins. Just removing #ifdefs already solves the problem that I cannot pass --hwcomposer argument to kwin_wayland if KWin was built without libhybris support and hwcomposer plugin is installed later and it can be loaded perfectly (and works fine). That's why we'll have to remove #ifdefs anyway.

I wanted to go with the minimalistic and non-invasive approach, with as little bloodshed as possible. But if you wish to see some rewrites, how about this:

  • Each plugin has a method to help in autodetection process (canLoad()) that quickly checks if the environment conditions are correct and allow loading of this plugin. Previously there was a function automaticBackendSelection(), that did that by just checking qEnvironmentVariableIsSet and so; now each plugin can do more complicated tests than just env vars checking.
  • Each plugin has a concept of "priority" for automatic loading, because we need some order among them. This was previously also in automaticBackendSelection(), the order of if statements. The priority is saved in JSON metadata, the same way as the "input" property is now stored and tested in Application::initPlatform() : https://cgit.kde.org/kwin.git/tree/main.cpp?h=Plasma/5.17#n442

It is not trivial to move CLI args providing and parsing into plugins, because some common arguments like --width and --height are only used for windowed backends, for example ( https://cgit.kde.org/kwin.git/tree/main_wayland.cpp?h=Plasma/5.17#n420 ) and some plugins have specific CLI options, like --x11-display for x11-windowed backend or --fb-device for framebuffer backend, --wayland-display for nested wayland. But maybe it's still possible by extending base Platform class even more...

anthonyfieroni added inline comments.
main_wayland.cpp
398–400

Just use toInt

405–407

ditto