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

Draft: CRT SwitchRes Linux EDID PoC #17197

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

syboxez
Copy link

@syboxez syboxez commented Nov 24, 2024

Discussion about specific implementation ideas in switchres here: antonioginer/switchres#102

Guidelines

  1. Rebase before opening a pull request
  2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
  3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises

Description

Extremely hacky PoC for using CRT CwitchRes to generate a single-use EDID and apply that EDID as root to a specific display. Very inelegant, some hardcoding going on, overwrites KMS and X11 modesetting with EDID overwrites (to test the PoC in multiple environments, such as KMS, Wayland, and X11)

Related Issues

Currently, RetroArch does not support CRT SwitchRes in Wayland environments and may or may not have support for DRM/KMS environments.

Currently RetroArch will set standard video modes in KMS but will not generate new modes, and switchres has WIP support for DRM/KMS modeswitching that relies on a kernel patch.

This commit does not require a kernel patch and will use an external switchres binary to generate a single-use EDID binary containing a single modeline (switchres does not support generating EDID binaries when running as a library from what I've seen).

Related Pull Requests

Potentially a future PR for switchres to add support for generating an edid in the API

Reviewers

@hunterk @antonioginer @alphanu1

I am expecting to have to completely delete this commit and redo it with a much cleaner strategy. Would appreciate comments.

… a single-use EDID and apply that EDID to a specific display.
@LibretroAdmin
Copy link
Contributor

Sounds great, @alphanu1 @hunterk @antonioginer let us know what you think.

@zoltanvb
Copy link
Contributor

Looking forward to test this when I get the time. Some preliminary comments:

  • Wayland in general is not very keen on letting clients change resolution or refresh rate
  • it can be useful even without CRT, I'd like to see 70Hz DOS games displayed at 70Hz
  • EDID override is a really rude operation, even if it is successful in RA, there may be problems when RA ends and some desktop needs to be displayed, probably an EDID restore is needed
  • Switchres+RA combination could really benefit from a documentation update, including switchres.ini location and usage: Switchres has sorely inadequate documentation docs#946
  • the ultimate combination would be something where user would be able to configure all the display resolution/refresh related items in one place (Switchres, BFI, VRR, auto refresh rate switch etc.), and they would work together, or at least clearly indicate what does not work

@syboxez
Copy link
Author

syboxez commented Nov 27, 2024

Looking forward to test this when I get the time. Some preliminary comments:

* Wayland in general is not very keen on letting clients change resolution or refresh rate

* it can be useful even without CRT, I'd like to see 70Hz DOS games displayed at 70Hz

* EDID override is a really rude operation, even if it is successful in RA, there may be problems when RA ends and some desktop needs to be displayed, probably an EDID restore is needed

* Switchres+RA combination could really benefit from a documentation update, including switchres.ini location and usage: [Switchres has sorely inadequate documentation docs#946](https://github.com/libretro/docs/issues/946)

* the ultimate combination would be something where user would be able to configure all the display resolution/refresh related items in one place (Switchres, BFI, VRR, auto refresh rate switch etc.), and they would work together, or at least clearly indicate what does not work

EDID restore is easy enough, docs can absolutely be better, Freesync/GSync mostly solves the refresh rate issue on displays that support it.

@antonioginer
Copy link
Contributor

Currently RetroArch will set standard video modes in KMS but will not generate new modes, and switchres has WIP support for DRM/KMS modeswitching that relies on a kernel patch.

I'd say RA + Switchres + KMS doesn't rely on a kernel patch. In this case, Switchres acts as a modeline calculator, and these modelines are applied directly by RA's drm context.

Potentially a future PR for switchres to add support for generating an edid in the API

Yes, this would be better and totally feasible. Although the more correct approach would be to add a new Switchres backend that encapsulates the edid creation and cleanup, and uses the same display addressing logic, the way the rest of backends work.

If I understand this right, this is meant to target the VideoAMP, isn't it?

@substring
Copy link
Contributor

I'm confused with the benefits of it:

  • the current code breaks the current switchres behaviour on KMS, so we have no choice other than using EDIDs instead of the native KMS modeswitching we're doing right now
  • requires root/sudo to force the edid refresh 😨
  • too much manual configuration, although this can be improved

To continue on @antonioginer 's words, RA 's KMS backend is real bare metal, as low level as can be, and should work on anything without a patched kernel since switchres is just used as a modeline generator, and hopefully KMS allows creating framebuffers at the expected modeline. I did test it on lakka when preparing my PR for KMS modeswitching and it was working on Pi 4 without anything else than updating RA.

@syboxez
Copy link
Author

syboxez commented Nov 28, 2024

I'd say RA + Switchres + KMS doesn't rely on a kernel patch. In this case, Switchres acts as a modeline calculator, and these modelines are applied directly by RA's drm context.
Yes, but the code I read in RA's video_crt_switch.c (that I replaced here for testing purposes), RA only uses existing modelines supplied by the kernel and doesn't use switchres AFAIK. Ideally, switchres would be handling all of the resolution switching and calculations for DRM/KMS, X11, and Wayland contexts. Since EDID switching requires root access, my idea is to use polkit to ask for root permissions when starting RA with CRT Switchres enabled in situations where EDID switching is the only viable option (as opposed to switchres KMS mode or xrandr mode), and keep persistent root access exclusively for use with writing to debugfs and backing up the EDID if there is no way to do that rootless.

Potentially a future PR for switchres to add support for generating an edid in the API

Yes, this would be better and totally feasible. Although the more correct approach would be to add a new Switchres backend that encapsulates the edid creation and cleanup, and uses the same display addressing logic, the way the rest of backends work.

Yes. I just started a PR here due to ease of implementing a PoC, but switchres is probably a more appropriate location for these changes, and then this PR can just morph into implementing the API changes in switchres later on if/when that is merged.

If I understand this right, this is meant to target the VideoAMP, isn't it?

Not familiar with what VideoAMP is in this context.

@syboxez
Copy link
Author

syboxez commented Nov 28, 2024

I'm confused with the benefits of it:

* the current code breaks the current switchres behaviour on KMS, so we have no choice other than using EDIDs instead of the native KMS modeswitching we're doing right now

Yes. This is temporary code that is meant as a PoC and is nowhere near ready for actual merging. It's more meant as a way to open discussion for an actually proper way of implementing this idea, if it's even a good idea to begin with.

* requires root/sudo to force the edid refresh 😨

Yeah. Not ideal, but I can't figure out a way of doing it rootless, like with a udev rule or something. Suggestions are welcome.

* too much manual configuration, although this can be improved

Absolutely. My end-goal for granting root is to use polkit to ask for root access once when enabling CRT Switchres (or when starting RA if already enabled) and maintain persistence so there will not be repeat prompts every time there's a resolution switch.

To continue on @antonioginer 's words, RA 's KMS backend is real bare metal, as low level as can be, and should work on anything without a patched kernel since switchres is just used as a modeline generator, and hopefully KMS allows creating framebuffers at the expected modeline. I did test it on lakka when preparing my PR for KMS modeswitching and it was working on Pi 4 without anything else than updating RA.

I thought the Pi had a separate implementation specific to it. What I read in the code was RA using existing modelines for its KMS mode instead of generating a modeline or using switchres's existing KMS mode. The reason I overwrote all of the implementations here was just for testing purposes and is not meant to be merged.

@zoltanvb
Copy link
Contributor

zoltanvb commented Dec 1, 2024

Tried it on Pi4 with Ubuntu, it seems that the edid_override entry is there under /sys/kernel/debug, but trigger_hotplug is not, and without it the EDID will not take effect.

@antonioginer
Copy link
Contributor

I thought the Pi had a separate implementation specific to it.

That is true up to the Pi 3, starting from the Pi 4 the general Switchres (KMS) code is used.

What I read in the code was RA using existing modelines for its KMS mode instead of generating a modeline or using switchres's existing KMS mode. The reason I overwrote all of the implementations here was just for testing purposes and is not meant to be merged.

No, that's not the case. Switchres will calculate new (non-existing) modes and call RA's KMS code to set them.

Not familiar with what VideoAMP is in this context.

https://www.gamoover.net/Forums/index.php?topic=44224.0

VideoAMP is a board that has a flashable edid, I thought you were working to support that, that's why I suggested doing it into Switchres itself. If that's just meant to be an alternative method to KMS or X11 modesetting, I don't think that's going to provide any benefit over the existing solution.

@substring
Copy link
Contributor

There is anoher way to force refresh the EDID, which is through the status in /sys/class/drm/cardX-CCC-n/status, but it may temporarily turn off display. But this has worked with variable results over kernel versions and GPU drivers. Hopefully that interface is stable at least, and requires root, again.

But again: considering your only target is KMS so far, this won't work as good as the current modeswitching in RA because the various steps are way too long compared to the current implementation.

I may miss some steps, but that's how it goes with the EDID method:

  • backup the current EDID
  • invoke switchres externally and compute a new EDID (would mean shipping a side switchres to RA, not so elegant)
  • have root set that new EDID
  • have the kernel refresh the connector. It shouldn't probe the monitor hopefully (this is pretty long), but it will validate all modes in the EDID
  • have RA refresh the mode list (this is "long")
  • delete the current FB
  • create a new FB with the new resolution

Too much to be worth for roms that switch mode ingame (Sonic 2 on MD is a great test case). And in the meantime, you changed the EDID at the whole OS level, not just for RA ...

Compared to how it's done today:

  • internally call switchres to generate the modeline (Switchres is baked into RA, which makes this call blazing fast)
  • create a new FB with that modeline
  • swap FB at vsync

Way faster, can't be beaten.

@syboxez
Copy link
Author

syboxez commented Dec 9, 2024

There is anoher way to force refresh the EDID, which is through the status in /sys/class/drm/cardX-CCC-n/status, but it may temporarily turn off display. But this has worked with variable results over kernel versions and GPU drivers. Hopefully that interface is stable at least, and requires root, again.

Yeah, I have no problem switching to a more stable interface. The root requirement is my main issue with this implementation, but I think it's required.

But again: considering your only target is KMS so far, this won't work as good as the current modeswitching in RA because the various steps are way too long compared to the current implementation.

My main target is Wayland, not KMS. I overwrote KMS here for testing purposes only.

I may miss some steps, but that's how it goes with the EDID method:

* backup the current EDID

* invoke switchres externally and compute a new EDID (would mean shipping a side switchres to RA, not so elegant)

* have root set that new EDID

* have the kernel refresh the connector. It shouldn't probe the monitor hopefully (this is pretty long), but it will validate all modes in the EDID

* have RA refresh the mode list (this is "long")

* delete the current FB

* create a new FB with the new resolution

Yeah, basically, although I would like to implement the actual logic inside of switchres instead of RetroArch and make it available to the API for a more elegant solution.

Too much to be worth for roms that switch mode ingame (Sonic 2 on MD is a great test case). And in the meantime, you changed the EDID at the whole OS level, not just for RA ...

I like to test with Sonic 2, Silent Hill PS1, and 240p test suite. It's actually pretty fast when used on an NTSC CRT. A bit slower when using a DAC in my experience, though over native VGA.

Compared to how it's done today:

* internally call switchres to generate the modeline (Switchres _is_ baked into RA, which makes this call blazing fast)

* create a new FB with that modeline

* swap FB at vsync

Way faster, can't be beaten.

Ultimately the goal with this PR as stated above. The current state as of now is purely a PoC.

@LibretroAdmin
Copy link
Contributor

Hi there, is there any kind of agreed upon road forward for this PR? If some of the things in this PR are controversial to plainly merge, are there maybe other elements that both sides can agree on that we can put into a separate PR and then merged?

@syboxez
Copy link
Author

syboxez commented Dec 19, 2024

Hi there, is there any kind of agreed upon road forward for this PR? If some of the things in this PR are controversial to plainly merge, are there maybe other elements that both sides can agree on that we can put into a separate PR and then merged?

I don't know about agreed upon, but my plan is to completely scrap this PR, implement the logic in upstream switchres and open a PR there, and either convert this PR to be an implementation of that API call into new Wayland-specific logic or open a new PR to do the same thing.

Whether you want to keep this PR open or just have me open a new one when the API stuff is ready is up to you; I don't mind either way.

@antonioginer
Copy link
Contributor

We're interested in extending Switchres support to Wayland, but we'd rather investigate (with syboxez) a method that doesn't involve EDIDs, if there's any, and only consider EDIDs as a last resort. Being honest, it's hard for us to see how that idea could fit cleanly in Switchres, without taking the usual shortcuts we've worked so hard to avoid, and we wouldn't like to see a wasted effort on the wrong direction. Anyway we welcome the interest in the api and are open to new ideas.

@syboxez
Copy link
Author

syboxez commented Dec 20, 2024

Discussion about specific implementation ideas in switchres here: antonioginer/switchres#102

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.

5 participants