Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "--display-config static=<filename>" option #551

Merged
merged 34 commits into from Aug 24, 2018
Merged

Conversation

AlanGriffiths
Copy link
Contributor

@AlanGriffiths AlanGriffiths commented Aug 17, 2018

Add a "--display-config static=<filename>" option. (Fixes #495)

This reads the indicated file and tries to position the display outputs accordingly.
If the file does not exist, or if actual outputs are not specified defaults are applied.
The complete resulting layout is listed in the Mir "info" log.

Note: Specifying a non-existent file is an easy way to get a template for editing.

(As with other configuration options this can also be specified with environment variables or a config file.)

@AlanGriffiths
Copy link
Contributor Author

Example config:

output_id=0/1: position=0,0; size=1280x800 # VGA modes: 1920x1080@60.0, 1680x1050@60.0, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.8, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/2: position=0,800; size=1280x800 # HDMI-A modes: 1920x1080@60.0, 1680x1050@59.9, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.9, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/3 # DisplayPort (disconnected)
output_id=0/4 # HDMI-A (disconnected)
output_id=0/5 # DisplayPort (disconnected)

@Saviq
Copy link
Collaborator

Saviq commented Aug 17, 2018

I know I originally said we don't need card_id, output_id, but if its sole purpose is to guide the user, maybe we should be more descriptive after all?

Also, we're printing modes, but there's no way to use them? I suppose size is distinct from mode in the sense that you may want to scale? But wouldn't we want to use scale for that?

@AlanGriffiths
Copy link
Contributor Author

@Saviq: I know I originally said we don't need card_id, output_id, but if its sole purpose is to guide the user, maybe we should be more descriptive after all?

I can change that to have card_id. (Or we can also do what xrandr does and only use the card number when it's not 0.)

It's hard to do anything helpful as we can't tell the use which physical output they corresponds to.

Also, we're printing modes, but there's no way to use them? I suppose size is distinct from mode in the sense that you may want to scale? But wouldn't we want to use scale for that?

Would it be clearer to be setting mode? E.g.

output_id=0/2: position=0,800; mode=1280x1024@75.0 # HDMI-A modes: ...

We can simply add setting scale:

output_id=0/2: position=0,800; mode=1280x1024@75.0; scale=2.0 # HDMI-A modes: ...

@Saviq
Copy link
Collaborator

Saviq commented Aug 17, 2018

+1 on setting mode and optionally scale. I'd say position and mode are required, but refresh rate could be omitted?

To help the user a bit, could we add some kind of ID of the connected display in the comment? Or is it not helpful, especially when your display wall (as usual) is comprised of multiple displays of the same make/model?

I think if we're not explicit about card/output in this we can just drop output_id= altogether. Maybe add a comment at the top describing the format?

@AlanGriffiths
Copy link
Contributor Author

A new example:

output_id=0/1: position=0,0; mode=1280x800@59.8; scale=1 # VGA modes: 1920x1080@60.0, 1680x1050@60.0, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.8, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/2: position=0,800; mode=1280x800@59.9; scale=1 # HDMI-A modes: 1920x1080@60.0, 1680x1050@59.9, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.9, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/3 # DisplayPort (disconnected)
output_id=0/4 # HDMI-A (disconnected)
output_id=0/5 # DisplayPort (disconnected)

@RAOF
Copy link
Contributor

RAOF commented Aug 20, 2018

If we're going to have a configuration file, maybe we should use an actual configuration format? Any of JSON, TOML, or YAML would seem to be be fine choices.

For JSON, jsoncpp is already installed on all our systems as a dependency of lots of things (dev package libjsoncpp-dev, website) or there's the header-only json library website.

For TOML there's nothing obvious in the archives, but the toml library is header-only and looks nice enough.

For YAML there's yaml-cpp (dev package libyaml-cpp-dev, website).

We seem to be using a lot of YAML in Canonical, maybe that's a reasonable choice?

@Saviq
Copy link
Collaborator

Saviq commented Aug 20, 2018

What's the format of "the config file" that can replace command-line options and env vars?

@AlanGriffiths
Copy link
Contributor Author

@Saviq What's the format of "the config file" that can replace command-line options and env vars?

https://www.boost.org/doc/libs/1_68_0/doc/html/program_options/overview.html#id-1.3.31.5.10

@AlanGriffiths
Copy link
Contributor Author

AlanGriffiths commented Aug 20, 2018

I've just remembered #293 - we should have orientation as an option.

Also,experimenting with this: the Mir compositor isn't respecting scale. So that shouldn't be an option (yet).

I'll make those changes.

I don't have strong opinions about config file formats. Once we reach a consensus I'll implement it.

@Saviq
Copy link
Collaborator

Saviq commented Aug 20, 2018

I just wonder if it's desirable to split the config files then? Can the boost options be dynamic (for changing number of outputs)?

@AlanGriffiths
Copy link
Contributor Author

The reason I split them was to facilitate having multiple display configs (e.g. docked and non-docked). But I guess that can also be done in a single file. I'll have to experiment a bit with the boost stuff to see exactly what's possible. (I've not actually used sections or repeated options.)

@AlanGriffiths
Copy link
Contributor Author

@Saviq trying to answer your question about Boost.Options:

Boost.Options needs to know the key names and option types before parsing the file. It isn't dynamic in that sense.

OTOH Boost.Options doesn't need to be dynamic to support multiple outputs: it is entirely possible to parse multiple values for the same key. (We'd need to rework some mirserver code to expose that feature, but the parsing engine works OK with it.) That is we could have this in the ~/.config/miral-kiosk.config file:

[display-config]
output_id=0/1: position=0,0; mode=1280x800@59.8; scale=1 # VGA modes: 1920x1080@60.0, 1680x1050@60.0, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.8, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/2: position=0,800; mode=1280x800@59.9; scale=1 # HDMI-A modes: 1920x1080@60.0, 1680x1050@59.9, 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.9, 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0, 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2, 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
output_id=0/3 # DisplayPort (disconnected)
output_id=0/4 # HDMI-A (disconnected)
output_id=0/5 # DisplayPort (disconnected)

We could even have several display configuration section, provided we know the section names in advance. For example, [display-config] and [display-config-alt].

However, in the supported format everything is key/value pairs. There no hierarchical structure (as you'd see in a yaml file). (The section name is just a shorthand for display-config.output_id.)

@Saviq
Copy link
Collaborator

Saviq commented Aug 20, 2018

It looks like the INI format is too limiting, I'll write up a richer example of what I think we should ultimately support for this use case and let's decide how far we want to go in the first iteration.

@AlanGriffiths
Copy link
Contributor Author

OK, I'll leave this up for discussion.

@Saviq
Copy link
Collaborator

Saviq commented Aug 20, 2018

@stephaneverdy, @jhodapp, @lool, @RAOF, @gerboland, @wmww

This is me going to the extreme in the outlook of supporting robust kiosk/signage stories. RFC!

Features

Here are the high-level features I think we should support with this long-term:

  • static display configuration based on physical outputs (as no EDID seems to be good enough, or worth it in the end)
    • need to ensure there's no risk of things getting reordered
  • pinning client surfaces to a display/geometry (think two displays driven from a single unit, one of them touch-enabled)
  • pinning input devices to surfaces/displays
  • ?

Config file format

A config format that I think could tackle this (remember this is taking it to the extreme, we'd be implementing this on an as-needed basis):

inputs:
# a list of input devices matched by regular expressions of any of the below keys
# can match zero or more devices
  - input-id: usb-Logitech_USB_Keyboard-event-kbd
                                   # (/dev/input/by-id/*)
    input-path: pci-0000:00:14.0-usb-0:1.3.2:1.0-event-kbd
                                   # (/dev/input/by-path/*)
    input-vendor: 046d             # USB vendor ID
    input-prod: c31d               # USB product ID
    # TODO: other means of matching input devices?
    label: logitech-keyboard       # for use in display/surface pinning below

  - input-type: kbd
    label: all-keyboards
    
  - input-type: mouse
    label: all-mice
    
  - input-type: touch              # TODO: what're the input types?
    # the below would be defaults, overridden by pinning to surfaces / displays
    displays: [my-display, other-display]
                                   # spanning the bounding rectangle of the listed displays
                                   # TODO: including those not listed, if they happen to fall
                                   # into the bounding rect (?)
    geometry: [0, 0, 3840, 2160]   # [x, y, w, h]
    # TODO: or, if touch devices report a default resolution
    position: [0, 0]               # [x, y]
    size: [3840, 2160]             # [w, h]
    # TODO: what other input properties we should allow here?

layouts:
# keys here are layout labels (used for atomically switching between them)
# when enabling displays, surfaces should be matched in reverse recency order

  my-layout:                       # the first layout is the default

    displays:
    # a list of cards matched by regular expressions of card-{path,serial,id},
    # failure to match a single card should be an error
    # omitted cards and outputs are disabled

    - card-path: ^pci-0000:00.02.0$  # preferred, most stable (/dev/dri/by-path/*-card)
      # remaining keys in the form <socket-type>-<output-id>
      hdmi-0:
        position: [0, 0]             # required, [x, y] 
        label: my-display            # used in surface / input pinning
        mode: 1920x1080@60           # defaults to the preferred mode
        resolution: [1920, 1080]     # forces a resolution, takes precedence over mode
        refresh-rate: 60.0           # forces a refresh rate, takes precedence over mode
        scale: 1.0                   # a float
        orientation: normal          # {normal, left, right, inverted}
        reflection: normal           # {normal, x, y, xy}
        inputs: [all-mice]           # inputs to pin to this display
      hdmi-1: ~                      # tilde (NULL in YAML) or omitting disables the output

    - card-serial: ^1234ABCD$        # TODO: alternative, if possible to get (?)
      displayport-0:                 # failure to match an output should be an error
        label: other-display
        position: [1920, 0]
        inputs: []                   # TODO: disables input on this display (?)

    - card-id: 2                     # last resort matching
      # TODO: other means of matching a card?
  
    surfaces:
    # a list of surfaces matched by regular expressions on title, program (executable), client ID
    # only matching surfaces will get mapped, new surfaces replace old ones
  
    - title: ^The app$
      program: ^/snap/app/\d+/app$
      id: ^The client ID$
      # TODO: other means of matching a surface?
      position: [50, 50]             # [x, y] TODO: could either coordinate be optional?
      size: [200, 200]               # [w, h] TODO: could either size be optional?
      inputs: [all-keyboards]
    - program: ^/snap/other-app/\d+/other-app$
      display: my-display            # this surface will get forced to the geometry of "my-display",
                                     # takes precedence over position and size
      inputs: []                     # this surface receives no input
    
  all-off: []

xrandr

Borrowing from xrandr, here are all the options available there - any we should support?

usage: xrandr [options]
  where options are:
  --display <display> or -d <display>
  --help
  -o <normal,inverted,left,right,0,1,2,3>
            or --orientation <normal,inverted,left,right,0,1,2,3>
  -q        or --query
  -s <size>/<width>x<height> or --size <size>/<width>x<height>
  -r <rate> or --rate <rate> or --refresh <rate>
  -v        or --version
  -x        (reflect in x)
  -y        (reflect in y)
  --screen <screen>
  --verbose
  --current
  --dryrun
  --nograb
  --prop or --properties
  --fb <width>x<height>
  --fbmm <width>x<height>
  --dpi <dpi>/<output>
  --output <output>
      --auto
      --mode <mode>
      --preferred
      --pos <x>x<y>
      --rate <rate> or --refresh <rate>
      --reflect normal,x,y,xy
      --rotate normal,inverted,left,right
      --left-of <output>
      --right-of <output>
      --above <output>
      --below <output>
      --same-as <output>
      --set <property> <value>
      --scale <x>x<y>
      --scale-from <w>x<h>
      --transform <a>,<b>,<c>,<d>,<e>,<f>,<g>,<h>,<i>
      --off
      --crtc <crtc>
      --panning <w>x<h>[+<x>+<y>[/<track:w>x<h>+<x>+<y>[/<border:l>/<t>/<r>/<b>]]]
      --gamma <r>:<g>:<b>
      --brightness <value>
      --primary
  --noprimary
  --newmode <name> <clock MHz>
            <hdisp> <hsync-start> <hsync-end> <htotal>
            <vdisp> <vsync-start> <vsync-end> <vtotal>
            [flags...]
            Valid flags: +HSync -HSync +VSync -VSync
                         +CSync -CSync CSync Interlace DoubleScan
  --rmmode <name>
  --addmode <output> <name>
  --delmode <output> <name>
  --listproviders
  --setprovideroutputsource <prov-xid> <source-xid>
  --setprovideroffloadsink <prov-xid> <sink-xid>
  --listmonitors
  --listactivemonitors
  --setmonitor <name> {auto|<w>/<mmw>x<h>/<mmh>+<x>+<y>} {none|<output>,<output>,...}
  --delmonitor <name>

bed…

@AlanGriffiths
Copy link
Contributor Author

"First, implement a new capability in the simplest way you can think of that "could possibly work". Don't build a lot of amazing superstructure, don't do anything fancy, just put it in." - http://wiki.c2.com/?DoTheSimplestThingThatCouldPossiblyWork

@RAOF
Copy link
Contributor

RAOF commented Aug 21, 2018

I put in a vote for YAML; we can do the simplest thing there and incrementally expand it with minimal work.

@AlanGriffiths
Copy link
Contributor Author

@RAOF Just as a comment, what are your criteria for selecting from auto foo() -> int and int foo()?

If I actually type it then I use the newer -> format. But sometimes it gets written another way...

@Saviq
Copy link
Collaborator

Saviq commented Aug 23, 2018

I've had a play with this and, modulo me editing the wrong .yaml all working as expected. And I can confirm #556, too.

+many!

@AlanGriffiths
Copy link
Contributor Author

@Saviq "+many!"

Have patience, I've hacked this together to get some feedback on the config file. (Look how much that has changed!)

There are some internals to clean up now (and some tests to write).

{
Node name_node;
Node config_node;
// No 'default' but there is only one - use it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just error out if default layout wasn't there. But if you think that's better, WFM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Saviq I have been using it as easy-to-implement behaviour that is compatible with my test files. But I can drop it now if we don't like it.

@AlanGriffiths
Copy link
Contributor Author

Having "slept on it" about I'm favouring default-layout: default # Defaults to default

Vis:

default-layout: default # Defaults to default
layouts:
# keys here are layout labels (used for atomically switching between them)
# when enabling displays, surfaces should be matched in reverse recency order

  default:                         # the default layout

    cards:
    # a list of cards (currently matched by card-id)

    - card-id: 0
      VGA-1:
        # This output supports the following modes: 1920x1080@60.0, 1680x1050@60.0,
        # 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.8,
        # 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0,
        # 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2,
        # 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
        #
        # Uncomment the following to enforce the selected configuration.
        # Or amend as desired.
        #
        # state: disabled       # {enabled, disabled}, defaults to enabled
        # mode: 1920x1080@60.0  # Defaults to preferred mode
        # position: [0, 0]      # Defaults to [0, 0]
        # orientation: normal   # {normal, left, right, inverted}, defaults to normal

      HDMI-A-1:
        # This output supports the following modes: 1920x1080@60.0, 1680x1050@59.9,
        # 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x960@60.0, 1280x800@59.9,
        # 1152x864@75.0, 1280x720@60.0, 1024x768@75.0, 1024x768@70.1, 1024x768@60.0,
        # 832x624@74.5, 800x600@75.0, 800x600@72.2, 800x600@60.3, 800x600@56.2,
        # 640x480@75.0, 640x480@66.7, 640x480@59.9, 720x400@70.1
        #
        # Uncomment the following to enforce the selected configuration.
        # Or amend as desired.
        #
        # state: enabled        # {enabled, disabled}, defaults to enabled
        # mode: 1920x1080@60.0  # Defaults to preferred mode
        # position: [0, 0]      # Defaults to [0, 0]
        # orientation: normal   # {normal, left, right, inverted}, defaults to normal

      DisplayPort-1:
        # (disconnected)

      HDMI-A-2:
        # (disconnected)

      DisplayPort-2:
        # (disconnected)

@Saviq
Copy link
Collaborator

Saviq commented Aug 24, 2018

We'll need a tie-breaker then ;)

I really dislike this:

default-layout: default # Defaults to default
[...]
  default:                         # the default layout

My eyes are defaulting from this.

@AlanGriffiths
Copy link
Contributor Author

AlanGriffiths commented Aug 24, 2018

How about:

default-layout: docked # If omitted, there must be exactly one layout
layouts:
  undocked:
    [...]
  docked:
    [...]

@gerboland
Copy link
Contributor

Do we need "default-layout" specifier at all? Can we not require at least one layout called "default", which is the default layout Mir uses?

@AlanGriffiths
Copy link
Contributor Author

@gerboland Do we need "default-layout" specifier at all? Can we not require at least one layout called "default", which is the default layout Mir uses?

That's what @Saviq advocates. I'm thinking an explicit default that can be amended easily might be easier to manage.

@gerboland
Copy link
Contributor

It does make it easier to manage should there be a large number of layouts to choose from, but IMO most users will have max a couple of layouts.

We can always add "default-layout" later if we see the need for it...

@Saviq
Copy link
Collaborator

Saviq commented Aug 24, 2018

@gerboland @AlanGriffiths I would consider changing the default layout more of a runtime option, so something along the lines of
--display-config=static=/path/to/config.yaml[my-layout] or --display-layout=my-layout.

I would expect a grand majority to only ever have a single layout really.

@AlanGriffiths
Copy link
Contributor Author

AlanGriffiths commented Aug 24, 2018

Providing additional options at runtime would suggest considering alternatives (or additions) to extending --display-config with static... in miral::display_configuration_options(). (That's always felt a bit limited, but incorporated the existing options.)

A miral::DisplayConfiguration class that can be configured via the shell with select_config(string), load(istream), and apply() would allow these options to be set by the shell. The shell can use miral::CommandLineOption to allow the user to configure them (or use an arbitrary source).

@AlanGriffiths
Copy link
Contributor Author

@gerboland, @Saviq tests added, the implementation just has the stuff I think we've agreed: none of the stuff I want to add to set the default explicitly in the file. (If I get motivated I can PR that later.)

@AlanGriffiths
Copy link
Contributor Author

bors r=Saviq

bors bot added a commit that referenced this pull request Aug 24, 2018
551: Add a "--display-config static=<filename>" option r=Saviq a=AlanGriffiths

Add a "--display-config static=\<filename\>" option. (Fixes #495)

This reads the indicated file and tries to position the display outputs accordingly.
If the file does not exist, or if actual outputs are not specified defaults are applied.
The complete resulting layout is listed in the Mir "info" log.

Note: Specifying a non-existent file is an easy way to get a template for editing.

(As with other configuration options this can also be specified with environment variables or a config file.)

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
Co-authored-by: Alan Griffiths <alan.griffiths@canonical.com>
@bors
Copy link
Contributor

bors bot commented Aug 24, 2018

Build succeeded

@bors bors bot merged commit 8f0e7e5 into master Aug 24, 2018
@bors bors bot deleted the static-display-config branch August 24, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants