Mir

Merge lp:~cemil-azizoglu/mir/use-options-to-probe into lp:mir

use-options-to-probe
Merge into development-branch

Proposed by Cemil Azizoglu on 2015-07-01

Status:	Rejected
Rejected by:	Cemil Azizoglu on 2015-07-15
Proposed branch:	lp:~cemil-azizoglu/mir/use-options-to-probe
Merge into:	lp:mir
Diff against target:	0 lines
To merge this branch:	bzr merge lp:~cemil-azizoglu/mir/use-options-to-probe
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel van Vugt			Needs Resubmitting on 2015-07-15
PS Jenkins bot (community)	continuous-integration		Approve on 2015-07-14
Chris Halse Rogers			Approve on 2015-07-06
Alexandros Frantzis (community)			Approve on 2015-07-03
Kevin DuBois (community)		2015-07-01	Needs Information on 2015-07-02
Review via email: mp+263447@code.launchpad.net

Commit message

Use command line options during platform probe.

Description of the change

Use command line options during platform probe.

Return best when a platform-specific option is passed in. This will help in differentiating between Mesa and X11 platforms when the server is launched from an xterm depending on the presence of the "--vt" option.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

FAILED: Continuous integration, rev:2720
http://jenkins.qa.ubuntu.com/job/mir-ci/4237/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3060/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/580
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3008/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/393
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/393/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3008/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4237/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4239/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4246/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-07-02:

IIUC, currently we have:

host platforms:
./mir_demo_server

guest platforms:
./mir_demo_server --host-server
./mir_demo_server --offscreen

With this branch, it becomes a bit more special-cased:

host platforms:
[android] ./mir_demo_server
[mesa] ./mir_demo_server --vt X

guest platforms:
[mesa] ./mir_demo_server
./mir_demo_server --host-server
./mir_demo_server --offscreen

Couldn't we have a mesa-specific option:
./mir_demo_server --x11-display :0.0
so that the host platforms are just the option-less versions?

review: Needs Information

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-07-03:

OK as an intermediate step, as long as it gets properly combined with the probing approach of mir-on-x11 (which I think was the plan anyway).

In the end this is how I think the logic should look like:

kms: if (use_vt_option || can_get_drm_master) return best; else return unsupported;
x11: if (xopendisplay_succeeds) return supported; else return unsupported;

> Couldn't we have a mesa-specific option:
> ./mir_demo_server --x11-display :0.0
> so that the host platforms are just the option-less versions?

I think the idea is to make the most reasonable choice at any time, which is in order:

1. Regardless of where we are, if --vt is used, use kms
2. If we are in a VT (= we can get drm master) use kms
3. If we are in an X11 session (= XOpenDisplay succeeds), use X11

There is also the choice of: Regardless of where we are (e.g. we could be in a VT), if DISPLAY is set use X11. However, that's difficult to implement without messing up the other rules. It is much easier to add --x11-display as you suggest, to force using X11, but I don't consider it critical.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-07-06:

Not what I was expecting, but this seems equally sensible :).

+1 for Alexandros' expectations for how this will evolve in the mir-on-x11 branch.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4271/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4274/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-07:

FAILED: Continuous integration, rev:2723
http://jenkins.qa.ubuntu.com/job/mir-ci/4276/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3097
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/618/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3045/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/425/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3045/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4276/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-07-08:

Well this is weird:

"3: Unknown package: debian/control contains versioned package python-mir-perf-framework but it is unknown to this script
3: The package list in this script needs to be updated
3/15 Test #3: package-abis ..............................***Failed 0.13 sec"

I don't get this on my local machine.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-08:

FAILED: Continuous integration, rev:2725
http://jenkins.qa.ubuntu.com/job/mir-ci/4282/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3110
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/632/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3058/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/431/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3058/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4282/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-08:

FAILED: Continuous integration, rev:2727
http://jenkins.qa.ubuntu.com/job/mir-ci/4283/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/633/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3059/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/432/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3059/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4283/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-08:

PASSED: Continuous integration, rev:2728
http://jenkins.qa.ubuntu.com/job/mir-ci/4285/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/635
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3061
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/434
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/434/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3061
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3061/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5868
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21738

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4285/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-07-09:

There are possibly better solutions...

Example 1: mplayer/mencoder with its countless built-in drivers and codecs each with their own options, makes it more obvious when a codec-specific option is specific. e.g...
mencoder -ovc lavc -lavcopts ...
This states clearly that I want to use video codec "lavc" and it has specific options of its own.

Example 2: In Mesa, graphics drivers have custom options of their own which you pass according to naming convention of environment variables: http://mesa3d.org/envvars.html

In both of these examples it's much clearer that an option is platform or driver specific. Similarly, I think we should make driver-specific options more explicit. Like...
<some_mir_server> --driver android --android-opts ...
or
<some_mir_server> --driver mesa-kms --mesa-kms-opts vt=1

Most importantly that means there is only _one_ option you need to look at to force the choice of graphics driver (--driver). Or if that's not provided then auto-detect. As an added bonus, my proposal also means that each driver does not need to be coupled to the ugly options interface, but instead just gets an option string of its own interpretation. It would also be a bonus if we went back onto the path of supporting graphics drivers written in C without requiring C++ from everyone (which limits driver compatibility).

But now I've proposed that what we have already needs to be changed too... More pragmatically I think there should only be one single server option to force the choice of graphics driver. It should not be implied by context. Once that is in place, it would syntactically not be possible to have the ambiguous:
env DISPLAY=:0 <some_mir_server> --vt 1
because it would be replaced by:
env DISPLAY=:0 <some_mir_server> --driver kms --kms-opts vt=1
which unambiguously selects KMS over X11.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-07-09:

Another bonus of my proposal is that we could then avoid loading all the drivers twice on start-up. You don't need to know driver-specific options in advance, unless the user explicitly asked about them:

some_mir_server --driver kms --kms-opts help

This is a pretty common pattern you will find in a lot of software and it would solve much of our problems nicely.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-07-09:

> Similarly, I think we should make driver-specific options more explicit.

Seems like a good idea. I would go for:

--driver mesa-kms|android|...
--driver-opts mesa-kms:mesa-kms-opt=1,android:android-opt=2,all-platforms-opt=3

Where driver-opts can be used even without an explicit driver setting. The reason is to allow invocations to work across platforms without change. So a script that wants to run everywhere, but wants to use VT 1 on mesa-kms can just use:

unity-system-compositor --driver-opts mesa-kms:vt=1

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-07-09:

> Couldn't we have a mesa-specific option:
> ./mir_demo_server --x11-display :0.0
> so that the host platforms are just the option-less versions?

With this branch we can reliably differentiate between mesa and x11. So I don't think this is needed. It doesn't bring any value. Besides what would it mean when you do "./mir_demo_server --x11-display :0.0" from a vt? It wouldn't make sense.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-07-09:

> > Similarly, I think we should make driver-specific options more explicit.
>
> Seems like a good idea. I would go for:
>
> --driver mesa-kms|android|...
> --driver-opts mesa-kms:mesa-kms-opt=1,android:android-opt=2,all-platforms-
> opt=3
>
> Where driver-opts can be used even without an explicit driver setting. The
> reason is to allow invocations to work across platforms without change. So a
> script that wants to run everywhere, but wants to use VT 1 on mesa-kms can
> just use:
>
> unity-system-compositor --driver-opts mesa-kms:vt=1

alf, duflu, I'm not sure what problem we are trying to solve here? My code is based on what we have which uses implicit (autodetection) method of choosing a driver. You seem to be questioning the underlying plumbing.

That said, I don't see any reason to change what we have. We are reliably differentiating between drivers. And with this branch and mir-on-x, we continue to do that.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-07-10:

Indeed I am questioning existing logic outside of this proposal too. If the existing logic needs changing then building on it further is just growing the problem.

What I propose requires less code, and also would fix bug 1454105.

It's not a massive issue if this branch lands. However that would make the problem larger and make it difficult to remove all this stuff, given we think we might want to.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-07-10:

> Indeed I am questioning existing logic outside of this proposal too. If the
> existing logic needs changing then building on it further is just growing the
> problem.
>
> What I propose requires less code, and also would fix bug 1454105.
>
> It's not a massive issue if this branch lands. However that would make the
> problem larger and make it difficult to remove all this stuff, given we think
> we might want to.

Choosing a driver explicitly is already supported - you just have to provide the '--platform-graphics-lib' option. In addition to that, we have the autodetection code. If we want to remove that, we can discuss it during 2/3rds.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-14:

PASSED: Continuous integration, rev:2729
http://jenkins.qa.ubuntu.com/job/mir-ci/4311/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3150
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/673
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3098
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/460
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/460/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3098
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3098/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5903
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21822