Add arguments to set the desired resolution.

[skyvine]

Hi @Aetf! I prepared this change for personal use because I wanted a larger resolution in my VM, and wanted to offer it to you because it seems generally useful (and as noted below, other users have had this problem). I saw your README stating that this is a personal fork just because the upstream repository has not been updated in a long time, so if you don't want to deal with extra random patches being added that makes perfect sense, but since you have a repository that has been updated more recently I thought it made sense to offer it to you before going elsewhere. Let me know what you think! =)

Problem Being Solved

It is currently not possible to set the resolution that kmscon displays at. This was a pain point for me, using kmscon instead of agetty in a VM, because the mode it selected used less than half of my screen space. The unmaintained repository has an issue open about this, so it is a pain point for others too.

Solution

This commit adds 2 new arguments to the command-line interface, which allow the user to specify their desired resolution. This only works in DRM mode, because kmscon does not support modesetting in fbdev:

kmscon/src/uterm_fbdev_video.c

Line 149 in 662884a

/* TODO: We do not support explicit modesetting in fbdev, so we require

Additionally, it only works if there is a mode which exactly matches the resolution specified. If no mode matches, or if fbdev is used, then the current behavior is preserved.

There are one root changes which facilitate this, another significant change which enables it, and some minor changes required for compatibility. The root change is that [the loop that selects a mode now checks if any mode matches the desired resolution, and sets it as the default if so]. This loop does not have direct access to the configuration data, so the desired width and height are stored in the uterm_video struct - this seemed like the least intrusive way to transfer the data, but I could update the commit to pass these values as separate arguments if desired. The other changes are related to making sure that the struct is fully initialized and updating the help output/man page.

Risks

These arguments can break kmscon if other system components are not configured correctly. In particular, on my VM kmscon initially output a blank screen due to an out of memory error. I had to pass video=1920x1080 as a kernel argument in order to resolve this issue. Restarting kmscon without specifying the resolution was sufficient to restore functionality.

I do not consider this commit to cause any regression because the behavior of the program is unchanged when the new arguments are not used.

Future Work

There are 2 items which I think are beyond the scope of this commit, but I will note as things I would like to add in the future (time allowing, a lot of things are up in the air right now for me).

First, using the mode that kmscon would previously select as a fallback mode when loading the desired resolution fails. In the case I encountered, it is easy to detect the fail state because the ioctl's that underlie either uterm_display_activate or uterm_display_swap return an out of memory error code. I tried adding simple logic to both of these functions to load the fallback mode, but this resulted in a screen that only rendered one line of output even though the correct mode was listed in the log output (and kmscon was clearly trying to print below the first line because the terminal didn't scroll after running commands, but the output was not rendered). As the fallback system is not needed to avoid a regression, I think it is fine for this to be considered out of scope for this commit.

Second, the test file does not allow for modesetting. I'm not sure what the scope of work required is here, since setting the mode after kmscon has initialized a display did not go well when I tried to do it for the fallback mode. I am less comfortable with having an untested feature than a less robust feature, if you happen to know what's needed here I'd love to hear about it.

Thank you for your consideration!

- Skyler

[skyvine]

I updated the commit to address the two issues described in "Future Work" from the initial post.

Testing

I looked at the test suite more closely. It looks like the build system only runs one test, from test_shl.c, and the functionality changed here would be part of test_output.c. The test just makes sure that it can set up a display and draw to it, so I'm not sure how this is more useful than running the main executable and checking that it works. A developer who is more familiar with the code might be able to poke at it in interesting ways, but I am not that developer. I did, however, update test_output.c to accept the --desired_width and --desired_height flags, with the same semantics as the main executable, in case it is useful to anyone.

Fallback mode

The new version of the commit keeps the previous semantics for the default_mode variable and adds a new member, desired_mode. The system attempts to use the desired mode, and if it runs into an error while page-swapping then it will retry with the default mode. This seems to work on my system, although it is possible that we want a similar check in other places.

I think it would be preferable to check how much memory is available before setting the mode and just do the right thing to begin with, but I could not find a good way of doing that. As far as I can tell, there is no uniform way to ask the system "how much memory is available for whatever backend you happen to be using". I would need to add a lot of code that is driver (and possibly kernel) specific. I think that it would be more appropriate for this kind of function to be in libdrm (if such a function makes sense at all), but it does not provide one. So I think recovery from failure is the most practical strategy for this situation.

Improvement Areas

I am not entirely comfortable adding desired_width and desired_height to the uterm_video struct. These are just random configuration options which are categorically different than everything else in that struct. The amount of data in there could quickly grow out of hand as more configuration options are added. I looked at what would be required to pass those values into bind_display separately, and it there were at least 3 different call stacks leading to bind_display, and it seems too disruptive to do that. I think it would make the most sense to just keep a pointer to the kmscon_conf_t in uterm_video, but that runs into a compatibility issue with test_output.c, which uses its own configuration structure and needs to create uterm_video instances.

On the topic of the test suite, I would expect that the ideal test suite would connect kmscon to mock/simulated displays to check that it does everything it is supposed to. However, I am suspicious that setting this up would require refactoring the API to take this into account. I expect it would be simple to make the above update to uterm_video if the test suite followed this strategy.

Finally, I am not sure if there are other aspects of the mode that users might want to configure and if so, what we would want the interface to look like there. Perhaps it would make sense to have a flag with an argument formatted similarly to the format of the kernel's video parameter. But I don't know if all of the options there make sense here. It might be that a flag like --mode={width}x{height} would be better for backwards compatibility, but on the other hand maybe it makes more sense to have different flags for different parts of the mode. It is not clear what future mode configuration options would look like, so I don't really have a basis for choosing between different formats. So I'm leaving the arguments as-is unless I find a compelling reason to change them.

The above work interests me, but I am not sure if/when I will have the time to act on it. It looks like this repository is unmaintained based on the age of some of the other pull requests and the most recent commit to this branch, so anything further I do will be uploaded to https://git.sr.ht/~skyvine/kmscon until there is a reply to this thread.

[skyvine]

Added one more commit to use retry logic when activating displays, otherwise there is undesirable behavior when switching between ttys on a system running multiple instances of kmscon. I assume there would be a similar issue if you run X or Wayland by default and switch over to kmscon.

[llitz]

hey @skyvine - can refresh rate be also added in here? I don't think it can be adjusted on fbdev, but shouldn't it be possible with drm?

[skyvine]

Sorry, I'm not sure. I prepared this change with no background knowledge, so I would have to do some research to understand how the refresh rates are handled. =)

[DanielMartensson]

@skyvine @llitz @Aetf
Hi!

I want to try this fork and see if this works for my ARM processor. Nice pull request by the way. I like your suggestion.