We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5393

Summary: KMSDRM: using atomic mode setting breaks GPU compatibility
Product: SDL Reporter: Substring <frog2wah>
Component: videoAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: critical    
Priority: P2 CC: sezeroz
Version: 2.0.13Keywords: target-2.0.14
Hardware: x86_64   
OS: Linux   
Attachments: test patch
test patch, v2
test patch, v3

Description Substring 2020-12-11 21:53:19 UTC
Hi,

Seeing the call for 2.0.14 and no attention to my forum post, I'm filing a bug.

I was trying the KMSDRM video backend with some very simple programs that were working ok on 2.0.12. The same code won’t work on the current dev branch and I get:

DEBUG: check_modesetting: probing “/dev/dri/card0”
DEBUG: /dev/dri/card0 connector, encoder and CRTC counts are: 4 5 6
DEBUG: check_modesetting: probing “/dev/dri/card0”
DEBUG: /dev/dri/card0 connector, encoder and CRTC counts are: 4 5 6
DEBUG: KMSDRM_VideoInit()
DEBUG: Opening device /dev/dri/card0
DEBUG: Opened DRM FD (3)
DEBUG: no atomic modesetting support.
DEBUG: Video subsystem has not been initialized
INFO: Using SDL video driver: (null)
DEBUG: Video subsystem has not been initialized

After carefully checking, the radeon driver doesn’t support atomic modesetting. That’s not the only problem : the same happens with the amdgpu driver if we disable Display Core (kernel parameter amdgpu.dc=0, which is required to get analogue outputs working).

This is a major regression in the KMSDRM driver.

Using atomic mode setting is great, but having no fallback to the "standard KMS" is bad.
Comment 1 Manuel Alfayate Corchete 2020-12-13 20:30:17 UTC
(In reply to Substring from comment #0)
> Hi,
> 
> Seeing the call for 2.0.14 and no attention to my forum post, I'm filing a
> bug.
> 
> I was trying the KMSDRM video backend with some very simple programs that
> were working ok on 2.0.12. The same code won’t work on the current dev
> branch and I get:
> 
> DEBUG: check_modesetting: probing “/dev/dri/card0”
> DEBUG: /dev/dri/card0 connector, encoder and CRTC counts are: 4 5 6
> DEBUG: check_modesetting: probing “/dev/dri/card0”
> DEBUG: /dev/dri/card0 connector, encoder and CRTC counts are: 4 5 6
> DEBUG: KMSDRM_VideoInit()
> DEBUG: Opening device /dev/dri/card0
> DEBUG: Opened DRM FD (3)
> DEBUG: no atomic modesetting support.
> DEBUG: Video subsystem has not been initialized
> INFO: Using SDL video driver: (null)
> DEBUG: Video subsystem has not been initialized
> 
> After carefully checking, the radeon driver doesn’t support atomic
> modesetting. That’s not the only problem : the same happens with the amdgpu
> driver if we disable Display Core (kernel parameter amdgpu.dc=0, which is
> required to get analogue outputs working).
> 
> This is a major regression in the KMSDRM driver.
> 
> Using atomic mode setting is great, but having no fallback to the "standard
> KMS" is bad.

Hi, thanks for the report

I may try to implement the legacy interface as a fallback sometime in the future, but just now I am swamped with another problem in the backend:
https://bugzilla.libsdl.org/show_bug.cgi?id=5350#add_comment
After finishing this, I won't have much time for some months.

So I think it's ATOMIC for now. I will try to implement the fallback as soon as possible.
To be honest, this surprises me (in a bad way). I went ATOMIC because I thought the legacy interface had it's days numbered, and that it was supported by all major drivers (AMDGPU, Intel, VC4, Freedreno...).
Looks like RADEON went outside my radar. Really sorry!
Comment 2 Manuel Alfayate Corchete 2020-12-13 20:33:59 UTC
Maybe it's just a matter of pointing to the legacy functions in KMSDRM_CreateDevice() when ATOMIC is not supported. But we are too near to release date I think. I don't know what could be done, it worries me that the next release has such a compatibility regression, but there's not much time...
Comment 3 Sam Lantinga 2020-12-14 17:07:44 UTC
If it's that simple of a fix, can you implement that and we can test it over the next couple of days?
Comment 4 Sam Lantinga 2020-12-15 20:24:33 UTC
Okay, I don't have a KMSDRM setup to test, but I added back the old KMSDRM driver as a fallback if the new one doesn't work:
https://hg.libsdl.org/SDL/rev/45c0170eec61

Can you try the latest snapshot and see if that a) builds, and b) works?
http://www.libsdl.org/tmp/SDL-2.0.zip

Thanks!
Comment 5 Ozkan Sezer 2020-12-15 21:11:22 UTC
(In reply to Sam Lantinga from comment #4)
> Okay, I don't have a KMSDRM setup to test, but I added back the old KMSDRM
> driver as a fallback if the new one doesn't work:
> https://hg.libsdl.org/SDL/rev/45c0170eec61

Seeing this is 2.0.12 code, I pushed two post-2.0.12 fixes to it:
https://hg.libsdl.org/SDL/rev/8151e3dbaeb5
Comment 6 Sam Lantinga 2020-12-15 22:04:45 UTC
Thanks!
Comment 7 Ozkan Sezer 2020-12-16 00:24:58 UTC
Pushed multiple build fixes to kmsdrm_legacy:
https://hg.libsdl.org/SDL/rev/0b115e06e6b4

Please review the commit carefully (easy to make a mistake
in that one.)
Comment 8 Substring 2020-12-17 22:20:46 UTC
(In reply to Sam Lantinga from comment #4)
> Okay, I don't have a KMSDRM setup to test, but I added back the old KMSDRM
> driver as a fallback if the new one doesn't work:
> https://hg.libsdl.org/SDL/rev/45c0170eec61
> 
> Can you try the latest snapshot and see if that a) builds, and b) works?
> http://www.libsdl.org/tmp/SDL-2.0.zip
> 
> Thanks!

As reported on the forum: it defaults to the KMSDRM driver and fails the same way. The only way round was to force the SDL_VIDEODRIVER to KMSDRM_LEGACY. So the workaround really close.
Comment 9 Ozkan Sezer 2020-12-17 22:50:40 UTC
Created attachment 4585 [details]
test patch

(In reply to Substring from comment #8)
> As reported on the forum: it defaults to the KMSDRM driver and fails the
> same way. The only way round was to force the SDL_VIDEODRIVER to
> KMSDRM_LEGACY. So the workaround really close.

Can we not add “Try ATOMIC compatibility” check from KMSDRM_VideoInit()
to check_modesetting(), so that KMSDRM_CreateDevice() fails and then
SDL_VideoInit() moves on to next bootstrap member which (the legacy one)?
I.e. something like in the attached test.patch

The patch is NOT even compile-tested. Must be tested to see, that:
- must be correct and must compile ;)
- the new atomic version works if appropriate drivers are available,
- must fallback to kmsdrm_legacy, if not.
Comment 10 Ozkan Sezer 2020-12-18 00:30:10 UTC
Created attachment 4587 [details]
test patch, v2

(In reply to Ozkan Sezer from comment #9)
> (In reply to Substring from comment #8)
> > As reported on the forum: it defaults to the KMSDRM driver and fails the
> > same way. The only way round was to force the SDL_VIDEODRIVER to
> > KMSDRM_LEGACY. So the workaround really close.
> 
> Can we not add “Try ATOMIC compatibility” check from KMSDRM_VideoInit()
> to check_modesetting(), so that KMSDRM_CreateDevice() fails and then
> SDL_VideoInit() moves on to next bootstrap member which (the legacy one)?
> I.e. something like in the attached test.patch

Arguably better patch, this time compile-tested. Must be run-tested
to see, that:
- the new atomic version works if appropriate drivers are available,
- must fallback to kmsdrm_legacy, if not.
Comment 11 Ozkan Sezer 2020-12-18 00:41:29 UTC
Created attachment 4588 [details]
test patch, v3

Revised, v3 patch. Same comments as above
Comment 12 Substring 2020-12-18 14:19:27 UTC
tested test_patch_v3.

It properly selects the right video backend between KMSDRM and KMSDRM_LEGACY.
Comment 13 Ozkan Sezer 2020-12-18 14:50:56 UTC
(In reply to Substring from comment #12)
> tested test_patch_v3.
> 
> It properly selects the right video backend between KMSDRM and KMSDRM_LEGACY.

Thanks. Patch applied as https://hg.libsdl.org/SDL/rev/a4eb1257a330

Manuel Alfayate Corchete: Can you give it a look and try to verify
nothing broke?  (Remember that the patch is applied to new kms/drm
code.)
Comment 14 Ozkan Sezer 2020-12-18 22:29:49 UTC
This seems to have been fixed.  Closing.