[platforms/hwcomposer] Add scaling support
ClosedPublic

Authored by davidedmundson on Feb 7 2019, 12:35 PM.

Details

Summary

Despite plasma frameworks doing it's own scaling with fonts, it's been
requested to use kwin/wayland scaling.

Like DRM when kscreen is not used, scale value is loaded from a config
file.

Config format is
[HWComposerOutputs][0]
Scale=N

The 0 is to map similarly to DRM and support multi-screen, but with a screen index
rather than a UUID based on EDID.

Because we don't support multi screen this is always 0 for now.

Test Plan

Ran with the config value unset and with the config value at Scale=3.

Diff Detail

Repository
R108 KWin
Branch
hwcomposer_again
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8633
Build 8651: arc lint + arc unit
davidedmundson created this revision.Feb 7 2019, 12:35 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 7 2019, 12:35 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Feb 7 2019, 12:35 PM
zzag added a subscriber: zzag.Feb 7 2019, 12:39 PM
zzag added inline comments.
plugins/platforms/hwcomposer/hwcomposer_backend.cpp
534

Should it be setScale(scale)?

fix obvious typo

zzag added inline comments.Feb 7 2019, 12:47 PM
plugins/platforms/hwcomposer/hwcomposer_backend.cpp
532

Please make it const.

davidedmundson retitled this revision from [plugins/platforms/hwcomposer] Add scaling support to [platforms/hwcomposer] Add scaling support.Feb 7 2019, 2:00 PM
romangg requested changes to this revision.Feb 8 2019, 1:43 PM
romangg added a subscriber: romangg.
romangg added inline comments.
plugins/platforms/hwcomposer/hwcomposer_backend.cpp
532

We have for the Wayland and X11 nested backends a --scale option to define the scaling value when kwin gets started. Please use this instead of new env var.

This revision now requires changes to proceed.Feb 8 2019, 1:44 PM

I can do, but I'll explain my rationale for this first.

When running nested it tends to be the user typing "kwin_wayland --scale" directly into a terminal.
On deployment it's hidden behind the startplasmamobile script.

I don't want that script to end up forked across devices.

With this they can just dump a value in /etc/profile.d
I guess I could still use --scale here and then make startplasmamobile use an env
Or there's the option of using a config like DRM does.

I can do, but I'll explain my rationale for this first.

When running nested it tends to be the user typing "kwin_wayland --scale" directly into a terminal.
On deployment it's hidden behind the startplasmamobile script.

That makes sense. But I would still prefer only one way to manually set scale in KWin on startup.
In light of D18465 at one point I also want to generalize initial output description to being setup with a list of parameters and reading in config files in json format (and falling back to KScreen config files; that way we wouldn't have ugly mode switches on startup). Because of that having it the same way over all platforms supporting initial output description is advantageous.

I don't want that script to end up forked across devices.

With this they can just dump a value in /etc/profile.d
I guess I could still use --scale here and then make startplasmamobile use an env
Or there's the option of using a config like DRM does.

Reading out the env variable in startplasmamobile sounds fine until we have the initial config file read.

I also want to generalize initial output description to being setup with a list of parameters and reading in config files in json format (and falling back to KScreen config files; that way we wouldn't have ugly mode switches on startup

I /really/ don't want that. A JSON thing for nested testing, sure.

But having kscreen and kwin both in charge of outputs is a mixup of two design patterns which will inevitably lead to trouble. The issues with mode switching can and should be hidden by ksplash.

Reading out the env variable in startplasmamobile sounds fine

Will do.

Set scale based on --scale argument

romangg accepted this revision.Feb 11 2019, 2:24 PM
This revision is now accepted and ready to land.Feb 11 2019, 2:24 PM
zzag added inline comments.Feb 11 2019, 2:26 PM
plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp
134

const

bshah added a subscriber: bshah.Feb 12 2019, 3:32 PM

21:00 <bshah> I just realized --scale can't be used
21:00 <bshah> let me explain
21:01 <bshah> current kwinwrapper script is used for both normal drm and normal hwcomposer supported devices
21:01 <bshah> I can't just pass a random scale argument to call of kwin_wayland
21:02 <bshah> so it needs a) hwcomposer specific environment variable (prev revision). or b) read config file like drm backend does

21:01 <bshah> current kwinwrapper script is used for both normal drm and normal hwcomposer supported devices
21:01 <bshah> I can't just pass a random scale argument to call of kwin_wayland

kwinwrapper script should be able to know if the device supports drm or hwcomposer. So why can't it send --scale only for hwcomposer?

With DRM on PlaMo KScreen is used or do we use the separate code path to read in output config in DRM backend directly?

But anyway that's not a well defined interface. I want either only one of these methods or both methods to be supported (but not introduce a third one with env var). I'm in favor of using both, and reading out a config file for initial setup on hotplug or program start and before the potential managing client (KScreen) can send the real configuration. But @davidedmundson had some reservations against that if I understood him correctly.

bshah added a comment.Feb 12 2019, 4:47 PM

kwinwrapper script should be able to know if the device supports drm or hwcomposer. So why can't it send --scale only for hwcomposer?

kwinwrapper doesn't.. in fact it's kwin_wayland which does auto-detect of environment and chooses backend to use.

With DRM on PlaMo KScreen is used or do we use the separate code path to read in output config in DRM backend directly?

KScreen is used.

davidedmundson marked 3 inline comments as done.Feb 12 2019, 4:50 PM

I don't want to see a config and kscreen used concurrently. We'll end up making a complex synchronization framework with a tonne of code duplication which isn't needed to solve the fairly simple issues. (ksplash should be rendering identical content on all monitors, and kwin should wait for kscreen on hotplug)

I have absolutely no issue with backends loading from a handwritten static config for situations where kscreen is not used.

I can do that here and follow the DRM naming so that it could be moved to something more generic.

One thing we should keep in mind: at some point we want multi-screen support on Plasma Mobile / convergence. For that we will probably just use KScreen as well.

So we don't want to hard-code the output values via config files and so on. In DRM backend we have this readOutputsConfiguration() method and this is used by PlaMo in some way? But long-term it should go away. The DRM driver should provide the available modes and per default KWin picks the best suited one.

Instead of setting the internal scale value and so on to something fixed what we want in hwcomposer is a way to tell KWin the display characteristics and available modes (probably only one), if libhybris does not provide that. Then we can use KScreen at one point here as well.

davidedmundson edited the summary of this revision. (Show Details)
Like DRM when kscreen is not used, scale value is loaded from a config
file.

Config format is
[HWComposerOutputs][0]
Scale=N

The 0 is to map similarly to DRM and support multi-screen, but with a screen index
rather than a UUID based on EDID.

Because we don't support multi screen this is always 0 for now.
davidedmundson edited the test plan for this revision. (Show Details)Feb 21 2019, 3:14 PM
romangg accepted this revision.Feb 21 2019, 7:50 PM

I'm fine with that. If later on we can use KScreen the config entry will become simply irrelevant.

This revision was automatically updated to reflect the committed changes.