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.

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.

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?

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. =)

@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.

Thanks for the nice PR! Could you rebase and resolve conflicts?

Thanks for looking at this. I've rebased locally but I'll need to set up a few things again before I can test. The conflicts revolve around the commit that adds --use-original-mode (e92a53e), which makes sense because they both deal with modesetting.

It is not sensible to supply a desired width/height if --use-original-mode is enabled, because they instruct kmscon to use potentially conflicting modes. The way I'm currently resolving the conflict in src/kmscon_seat.c keeps the primacy of --use-original-mode and adds the retry logic to the else case. This is the simplest resolution but I'm not sure it's the correct one.

if (seat->conf->use_original_mode)
	ret = uterm_display_activate(d->disp,
				     uterm_display_get_original(d->disp));
else {
	ret = uterm_display_activate(d->disp, NULL);
	if (ret == -EAGAIN)
		ret = uterm_display_activate(d->disp, NULL);
}

In principle, it should be an error to supply a desired width/height while --use-original-mode is enabled. These options contradict each other. But refusing to run could cause practical problems for users and this is a likely to happen to people because --use-original-mode defaults to true. That flag defaults to true so I think it would make sense to say that the desired width/height takes precedence. If the original mode takes precedence then a user who supplies a desired width/height with no other options would see no change in behavior which would be surprising. I would appreciate input on the most appropriate way to handle this contradiction.

I'm a little nervous that we could end up in some sort of convoluted infinite loop due to the retry logic in src/uterm_drm2d_video.c and src/uterm_drm3d_video.c basing its decision on whether or not the current mode matches the desired mode. I think this shouldn't happen because I had to take into account the case where there's not enough memory for the desired mode, but I wrote the patch with the assumption that nothing else would be trying to select a specific mode and that assumption no longer holds. I'll take a fresh look at the logic I added and test different scenarios in the near future.

I think it's fine to make it a hard error when a desired width/height is supplied while --use-original-mode is enabled, since the desired width/height arguments are newly introduced and don't have defautl values. We can mention in the help text and error message, that --use-original-mode must be turned off.

I have rebased and added an error message if --no-use-original-mode is not given. I added this check immediately after the check for conf->exit, which is what tells kmscon to quit when --help is given. This way we abort very early while still keeping the help output clean in case the user just adds the flag to the end of their command.

Thanks for rebasing your PR.

Instead of desired_width/desired_height, I would prefer one argument accepting a string, like:
--mode=1024x768
and we can optionally add refresh rate later
--mode=1024x768@60

I don't think we really need the retry logic.
In drm mode, you list the mode available from the device, so if one correspond to what you're looking for, then you shouldn't get an error when setting that mode. If this mode is not listed, then it will fail.

The error I ran into when setting that mode is that the kernel didn't allocate enough memory to the framebuffers; I had to additionally add "video=widthxheight" to the kernel's command line in order for it to work. Without this, I was simply presented with a blank screen. The retry logic allows us to display something to the user in this case even though it's not exactly what they asked for.

I can update the input format accordingly.

Ok, that's a bit surprising that you need the video= kernel argument, but as you mention it's in a VM, that can explain it.
So yes, you can keep the retry code then.

Out of curiosity, which driver are you using in your VM?
I think you shouldn't have this issue with virtio-gpu, so it's probably another one.

My responses might be a bit delayed, I'm working a shift job right now so my computer time during the week is limited.

I uploaded a new commit which uses the updated input format. If the format is incorrect then kmscon will add a warning to the log and ignore the argument.

As far as the driver goes, I think bochs-drm is what you're looking for? I'm running on top of Xen in HVM mode if that helps.

Thanks, your last upload looks good to me, I will merge it before the end of the week if no other comments.

Regarding bochs-drm driver, maybe this memory problem has been fixed in the kernel with https://lists.freedesktop.org/archives/dri-devel/2024-August/466783.html that should be in v6.13+