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

KMSDRM: using atomic mode setting breaks GPU compatibility #3920

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

KMSDRM: using atomic mode setting breaks GPU compatibility #3920

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: 2.0.13
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2020-12-11 21:53:19 +0000, Substring wrote:

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.

On 2020-12-13 20:30:17 +0000, Manuel Alfayate Corchete wrote:

(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!

On 2020-12-13 20:33:59 +0000, Manuel Alfayate Corchete wrote:

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

On 2020-12-14 17:07:44 +0000, Sam Lantinga wrote:

If it's that simple of a fix, can you implement that and we can test it over the next couple of days?

On 2020-12-15 20:24:33 +0000, Sam Lantinga wrote:

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!

On 2020-12-15 21:11:22 +0000, Ozkan Sezer wrote:

(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

On 2020-12-15 22:04:45 +0000, Sam Lantinga wrote:

Thanks!

On 2020-12-16 00:24:58 +0000, Ozkan Sezer wrote:

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

On 2020-12-17 22:20:46 +0000, Substring wrote:

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

On 2020-12-17 22:50:40 +0000, Ozkan Sezer wrote:

Created attachment 4585
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.

On 2020-12-18 00:30:10 +0000, Ozkan Sezer wrote:

Created attachment 4587
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.

On 2020-12-18 00:41:29 +0000, Ozkan Sezer wrote:

Created attachment 4588
test patch, v3

Revised, v3 patch. Same comments as above

On 2020-12-18 14:19:27 +0000, Substring wrote:

tested test_patch_v3.

It properly selects the right video backend between KMSDRM and KMSDRM_LEGACY.

On 2020-12-18 14:50:56 +0000, Ozkan Sezer wrote:

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

On 2020-12-18 22:29:49 +0000, Ozkan Sezer wrote:

This seems to have been fixed. Closing.

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

No branches or pull requests

1 participant