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

Allow to add additional modes to outputs #1095

Merged
merged 3 commits into from Jul 7, 2018

Conversation

Projects
None yet
6 participants
@agx
Copy link
Contributor

agx commented Jun 28, 2018

This adds support for adding additional modes to an output. This can be useful when a user wants to define custom modes (similar to xrandr --newmode).

This will become more even more interestin when we want to allow clients to add custom modes e.g. via
swaywm/wlr-protocols#15

Fixes #1080

@ascent12 ascent12 self-requested a review Jun 28, 2018

@emersion
Copy link
Member

emersion left a comment

-1 for wlr_output_add_mode. Use wlr_output_set_custom_mode instead.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 28, 2018

@emersion set_custom_mode seems to set the current mode. I'd like to be able to add several additional modes to the list of available modes and select from them at a later point:

This can be useful if you e.g. want to drive a projector at a conference. See e.g.:

https://wiki.debconf.org/wiki/DebConf19/Advice_for_presenters#xrandr_hints

for how to prepare for a talk. I'd be great if something like this would be possible with wlroots at some point.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 28, 2018

Hmm. I'm not sure I like this. What's the upside of doing this instead of setting the mode you want?

Also this modeline parsing is very meh. And: what should this do on non-DRM backends?

else if (strcasecmp(vsync, "-vsync") == 0)
mode->flags |= DRM_MODE_FLAG_NVSYNC;
else
return false;

This comment has been minimized.

@ddevault

ddevault Jun 28, 2018

Member

braces are mandatory

&mode->vdisplay,
&mode->vsync_start,
&mode->vsync_end,
&mode->vtotal, hsync, vsync) != 11)

This comment has been minimized.

@ddevault

ddevault Jun 28, 2018

Member

Here too

else
return false;

snprintf(mode->name, sizeof mode->name, "%dx%d@%.3f",

This comment has been minimized.

@ddevault

ddevault Jun 28, 2018

Member

sizeof(...)

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jun 28, 2018

Rather than trying to bubble this through the wlr_output interface I think it would be better for users hoping to set this to use grab drm.h and use some DRM-specific function.

@agx agx force-pushed the agx:output-add-mode branch from 101ac70 to 399d142 Jun 29, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 29, 2018

@emersion the upside of being able to add modes is that you can set them in advance (without running them) and a GUI can later on offer them for activation (see swaywm/wlr-protocols#15). It will also appear in the list of available modes returned by wl_output so even without it if we allow to switch a compositor to cycle through a list of available modes this would be in there (so e.g. a sysadmin can set the modes for a user to use at a later point - we'll likely want a wlr_output_remove_mode at a later point to remove a mode that is supported by an output from the list of available modes).

Non-drm backends can simply ignore the unused parameters and use x@ to add to the list of available modes.

I'm also not sure what's so bad about the modeline parsing. We're shifting around Vesa modes implicitly in drm_mode all the time. The alternative would be to fill drmModeModeInfo and pass that around and leave the parsing to the compositor.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 29, 2018

@SirCmpwn you mean adding a drm_connector_add_mode to drm.h ? We could do that but other backends would miss out then (see above) and I think we need add_mode / remove_mode functions functions to give fine enough control to the user.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 29, 2018

@emersion the upside of being able to add modes is that you can set them in advance (without running them) and a GUI can later on offer them for activation (see swaywm/wlr-protocols#15).

I really don't understand why adding mode is so important when you can set any custom mode. A client that lets the user choose a mode for an output can also offer to set a custom mode and remember a list of previously used custom modes.

You can also make your compositor do this job of remembering additional custom modes and exposing them through an output management protocol.

It will also appear in the list of available modes returned by wl_output so even without it if we allow to switch a compositor to cycle through a list of available modes this would be in there

Exposing modes in wl_output was a mistake in the Wayland protocol design - we only do so for user convenience and debugging. GNOME only exposes the current mode for instance.

Non-drm backends can simply ignore the unused parameters and use x@ to add to the list of available modes.

Remember that not all outputs support modes - all outputs except the DRM ones will have zero modes. This function will be a no-op on these backends, as adding a mode to the list would mark the output as supporting modes.

I'm also not sure what's so bad about the modeline parsing. We're shifting around Vesa modes implicitly in drm_mode all the time. The alternative would be to fill drmModeModeInfo and pass that around and leave the parsing to the compositor.

It's not convenient for an API. If you want to programmatically add a new mode you need to format a string that'll get parsed anyway right after. Strings are fine in the config file, not in a library API.

@SirCmpwn you mean adding a drm_connector_add_mode to drm.h ? We could do that but other backends would miss out then (see above) and I think we need add_mode / remove_mode functions functions to give fine enough control to the user.

I think he meant getting the DRM connector handle from the wlr_output to allow compositors to add modes themselves.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 29, 2018

I really don't understand why adding mode is so important when you can set any custom mode. A client
that
lets the user choose a mode for an output can also offer to set a custom mode and remember a list of
previously used custom modes.

Yeah I could do that but I think proper mode handling should be part of a compositor library. If that's not the case that's ok and every compositor can do this by itself but..

I think he meant getting the DRM connector handle from the wlr_output to allow compositors to add modes > themselves.

...that only works if I can rely on adding a mode always stays as simple as:

wl_list_insert(&conn->output.modes, &mode->wlr_mode.link);

with a filled in drm_mode. Is that the case?

Remember that not all outputs support modes
From what I have seen they always have at least one. Otherwisewlr_output_set_custom_mode would be a bad name.

Regarding modeline parsing

It's not convenient for an API. If you want to programmatically add a new mode you need to format a string > that'll get parsed anyway right after. Strings are fine in the config file, not in a library API.

I agree here and that's why I talked about drmModeModeInfo above. We can pass around the parameters as separate variables but I don't think that's a much better API either. Third way would be to just pass in width, height and refresh and then make up the rest to fill drmModeModeInfo.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jun 29, 2018

I think he meant getting the DRM connector handle from the wlr_output to allow compositors to add modes themselves.

That's not what I meant. @agx had the right idea, I want a function in drm.h which asserts the output passed is a DRM output and then does whatever it wants (up to and including modeline parsing). I think this is a pretty DRM specific feature, and other backends wouldn't benefit from it (at least not the ones we have today, save perhaps for headless, but w/e). Maybe the modeline parsing and adding of the mode can be separate functions and we can reuse them for headless in the future, idk.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 29, 2018

I want a function in drm.h which asserts the output passed is a DRM output and then does whatever it wants (up to and including modeline parsing).

I'm okay with the DRM-specific function. This function could take a drmModeModeInfo struct directly so that compositors don't have to format a modeline when calling it. The modeline parsing can be moved into rootston's config parsing.

Still not convinced it's really useful, but this design makes it acceptable to me.

we can reuse them for headless in the future

Some outputs are spec'ed to support modes, in which case the compositor will stick to the modes advertised by the output when modesetting. Some other outputs can have any size, thus don't support modes (ie. modes don't make sense on them): this is the case of all backends except DRM. Outputs support modes if the list of modes is not empty. Adding modes to the headless backend would break this design.

@martinetd

This comment has been minimized.

Copy link
Contributor

martinetd commented Jun 30, 2018

we can reuse them for headless in the future

Some outputs are spec'ed to support modes, in which case the compositor will stick to the modes advertised by the output when modesetting. Some other outputs can have any size, thus don't support modes (ie. modes don't make sense on them): this is the case of all backends except DRM. Outputs support modes if the list of modes is not empty. Adding modes to the headless backend would break this design.

FWIW headless currently defines and uses output_set_custom_mode at init (without checking the return value... and output_set_custom_mode can free the output that it didn't create? fun...!), and while I'm looking x11 and wayland backends define a static function with the same name that's never called and probably ought to be removed.
Shall I put that in an issue somewhere? It can wait until you folks agree on something here :P

@atomnuker
Copy link
Contributor

atomnuker left a comment

NAK, modeline strings are stupid and don't belong in any sort of a proper API!
Duplicate libdrm API if you have to, but seriously, modelines are legacy and should be eliminated. Leave parsing to whatever messed up tool wants to use string modelines.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 30, 2018

and output_set_custom_mode can free the output that it didn't create?

I don't get it. output_set_custom_mode doesn't free outputs. We should check its return value though.

and while I'm looking x11 and wayland backends define a static function with the same name that's never called and probably ought to be removed.

It's never called, but it's exposed via the output interface (.set_custom_mode = output_set_custom_mode). So it should stay there. This output function is used by rootston and sway fwiw.

@agx agx force-pushed the agx:output-add-mode branch from 399d142 to c23e1b5 Jul 3, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 3, 2018

Made this drm specific. I moved the modeline parsing to rootston so it does not clutter the wlroots API (yet?). However we could move parse_modeline into drm.c as well allowing compositors to parse modelines and get a xf86ModeModeInfo back.

If this looks better I'd have to guard the places with #ifdef linux in rooston which is the downside of not having it all in drm.c

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jul 3, 2018

Looking better.

If this looks better I'd have to guard the places with #ifdef linux in rooston which is the downside of not having it all in drm.c

Does BSD's drm implementation not support custom modes?

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 4, 2018

Ah...forgot about libdrm on FreeBSD. So it will have xf86ModeModeInfo as well.

}

assert(modeinfo);
memcpy (&mode->drm_mode, modeinfo, sizeof(*modeinfo));

This comment has been minimized.

@emersion

emersion Jul 5, 2018

Member

Style: no space before (

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 5, 2018

The DRM backend part LGTM.

@ascent12 Thoughts?

@ascent12
Copy link
Member

ascent12 left a comment

Aside for the extremely minor style things, overall it looks fine.
I'll try test this myself later today.

wl_list_insert(&conn->output.modes, &mode->wlr_mode.link);
return true;
}

This comment has been minimized.

@ascent12

ascent12 Jul 5, 2018

Member

There are extra blank lines on both sides of this function.

This comment has been minimized.

@ddevault
@@ -566,6 +566,31 @@ static bool drm_connector_set_mode(struct wlr_output *output,
return false;
}


bool wlr_drm_connector_add_mode(struct wlr_output *output, drmModeModeInfo *modeinfo) {

This comment has been minimized.

@ascent12

ascent12 Jul 5, 2018

Member

modeinfo can be const.

This comment has been minimized.

@ddevault

@agx agx force-pushed the agx:output-add-mode branch from c23e1b5 to bfc10ee Jul 6, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 6, 2018

@@ -841,6 +842,10 @@ void handle_new_output(struct wl_listener *listener, void *data) {
roots_config_get_output(config, wlr_output);
if (output_config) {
if (output_config->enable) {
struct roots_output_mode_config *mode_config;
wl_list_for_each(mode_config, &output_config->modes, link) {
wlr_drm_connector_add_mode(wlr_output, &mode_config->info);

This comment has been minimized.

@emersion

emersion Jul 6, 2018

Member

This needs to check that the wlr_output is a DRM output.

if (parse_modeline(value, &mode->info)) {
wl_list_insert(&oc->modes, &mode->link);
} else {
free (mode);

This comment has been minimized.

@emersion

emersion Jul 6, 2018

Member

Extra space here

@@ -566,6 +566,31 @@ static bool drm_connector_set_mode(struct wlr_output *output,
return false;
}


bool wlr_drm_connector_add_mode(struct wlr_output *output, drmModeModeInfo *modeinfo) {

This comment was marked as resolved.

@emersion

emersion Jul 6, 2018

Member

Maybe we should ensure that the type is DRM_MODE_TYPE_USERDEF?

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 6, 2018

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 6, 2018

We're defining tht mode so we're setting it as DRM_MODE_TYPE_USERDEF,
what would we check, that it doesn't match an existing mode?

You set DRM_MODE_TYPE_USERDEF in rootston. That means API users can add a mode with this flag set to something else. We should probably forbid that.

@agx agx force-pushed the agx:output-add-mode branch from bfc10ee to 8290acd Jul 6, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 6, 2018

struct wlr_drm_mode *mode = calloc(1, sizeof(*mode));

assert(modeinfo);
if (!mode || modeinfo->type != DRM_MODE_TYPE_USERDEF) {

This comment has been minimized.

@emersion

emersion Jul 6, 2018

Member

This leaks mode in case modeinfo->type is incorrect. We can probably move the modeinfo->type check before allocating.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 6, 2018

While doing that I'm wondering if it wouldn't be better to return 0 in case of success and < 0 in case of error so we can distinguish parsing errors from OOM?

I think using bools is fine

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 6, 2018

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 6, 2018

Anoher good point. We're not keeping track of that yet, do we? I'd rather not look at wlr_output->name.

We can probably use wlr_output_is_drm

@agx agx force-pushed the agx:output-add-mode branch from 8290acd to cc426da Jul 6, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 6, 2018

@emersion
Copy link
Member

emersion left a comment

LGTM

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jul 6, 2018

LGTM but I'd like to see @ascent12's comments addressed

wlr_drm_connector_add_mode(wlr_output, &mode_config->info);
}
} else {
if (wl_list_length(&output_config->modes)) {

This comment has been minimized.

@emersion

emersion Jul 6, 2018

Member

If you're going to update this PR, here's a nitpick: it's better to use wl_list_empty here instead of wl_list_length because it is constant time.

This comment has been minimized.

@emersion

emersion Jul 7, 2018

Member

Ping?

This comment has been minimized.

@agx

agx Jul 7, 2018

Author Contributor

Seems github hates me. Seems the PR updated now correctly.

@agx agx force-pushed the agx:output-add-mode branch from cc426da to a347462 Jul 7, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 7, 2018

@agx agx force-pushed the agx:output-add-mode branch from a347462 to e922d34 Jul 7, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 7, 2018

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 7, 2018

Hmm, are you missing a git push? It seems that the parameter is not const and wl_list_length is still used?

drm: Allow to add additional modes
This allows to add additional modes to the list of available video modes
using VESA Coordinated Video Timing information.

Closes #1080

@agx agx force-pushed the agx:output-add-mode branch from e922d34 to f9752db Jul 7, 2018

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 7, 2018

agx added some commits Jul 3, 2018

@agx agx force-pushed the agx:output-add-mode branch from f9752db to 60a83e9 Jul 7, 2018

@ddevault ddevault merged commit be54278 into swaywm:master Jul 7, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jul 7, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.