[platforms/hwcomposer] Add scaling support
AcceptedPublic

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

Details

Reviewers
romangg
Group Reviewers
KWin
Summary

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

Scale value is based off an environment variable as plasma-mobile
doesn't support kscreen and typically hwcomposer usage will be in a
static deployment.

Test Plan

Ran without the env, nothing broke
Added to kwinwrapper, everything was the right size

Diff Detail

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

Should it be setScale(scale)?

fix obvious typo

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

Please make it const.

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

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.Fri, Feb 8, 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.Mon, Feb 11, 2:24 PM
This revision is now accepted and ready to land.Mon, Feb 11, 2:24 PM
zzag added inline comments.Mon, Feb 11, 2:26 PM
plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp
134

const

bshah added a subscriber: bshah.Tue, Feb 12, 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.Tue, Feb 12, 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.Tue, Feb 12, 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.