libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
8.85k stars 1.66k forks source link

X11: setting too low resolution crashes X #4840

Closed substring closed 2 years ago

substring commented 2 years ago

Hi,

Same issue as #4561 but in a different context. When playing SDL2 games on a CRT screen (with resolutions lower than 320x200) X fails with:

X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 7 (RRSetScreenSize) Value in failed request: 0x130 Serial number of failed request: 260 Current serial number in output stream: 261

This happens because the drivers have a resolution low limit, but this never happened prior to SDL 2.0.16. As the the issue mentionned above, this is triggered by calling RRSetScreenSize

Edit : changed the SDL2 problematic version from SDL 2.0.18 to 2.0.16, my bad

icculus commented 2 years ago

(Worth noting the patch in batocera-linux's repo was reverted.)

I think this is just a matter of our X11 catching this exception and returning an error (under the assumption the hardware or the X server lied about this being a valid display mode even though it fails when we try to set it).

substring commented 2 years ago

I'm maintaining an Archlinux based OS for retrogaming on PC + CRT arcade cabs https://github.com/substring/os, resolutions lower than 320x200 are definitely not seldom. My workaround for now is to revert SDL_x11modes.c to its state in 2.0.14 and recompile SDL2

icculus commented 2 years ago

So to be clear, the system definitely supports the resolution and we're just failing to set it?

substring commented 2 years ago

If my memory serves me right, the call to RRSetScreenSize didn't exist until https://github.com/libsdl-org/SDL/commit/16e3bfe807dfdd48b6833da4cd78e789a920e2ec The call to this function triggers some checks regarding the minimum width/height which is set at 320x200 on GPU drivers side, here are some references:

https://github.com/freedesktop/xorg-xf86-video-amdgpu/blob/65c127366a22c03d2ffcdcdf91eec28cac733e83/src/drmmode_display.c#L3369

https://github.com/freedesktop/xorg-xf86-video-ati/blob/5eba006e4129e8015b822f9e1d2f1e613e252cda/src/drmmode_display.c#L2857

The check from RRSetScreenSize : https://github.com/freedesktop/xorg-xserver/blob/aeed57d722f2eb978c17fd7e859334d34af38d05/randr/rrscreen.c#L257-L265

Not calling RRSetScreenSize doesn't trigger the check, so we can use odd 90s odd CRT resolutions, which worked fine till SDL 2.0.14

icculus commented 2 years ago

So I'm pretty confident this is a bug in x.org, since they hardcode a minimum size, but if all we need to do to make this work for your needs without changing this for everyone else is catch the error on RRSetScreenSize and ignore it, that seems like a reasonable workaround for now. I'm putting a patch in momentarily.

substring commented 2 years ago

I can test the patch before you commit it, because the error just kills the video. I don't know if it's just the connector or the whole GPU that is "disabled" but X is not anymore in a stable state.

icculus commented 2 years ago

@substring Try the latest in revision control and let me know if it works. If it doesn't, I'm just changing this to not call XRRSetScreenSize if the resolution is < 320x200.

icculus commented 2 years ago

Flibit just added the "waiting" label, but to be clear, we're trying to wrap up 2.0.22, so we really want your feedback on the patch as soon as you can try it, @substring.

Thanks!

substring commented 2 years ago

Doing my best to find some time, will test 1c7bf478ae9c7904c70509277b9a3ce4a2ca7ab2 asap and report back

substring commented 2 years ago

OK, took me some time to get the good environment to trigger the bug again. It was fixed with 2.0.18. I can confirm your patch makes no regression for the issue.

So I'm closing.